Re: [Xen-devel] [PATCH v6 3/5] IOMMU: Make the pcidevs_lock a recursive one

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Fri, 2016-03-04 at 11:54 +0000, Xu, Quan wrote:
> On March 04, 2016 5:29pm, <JBeulich@xxxxxxxx> wrote:
> > On March 04, 2016 7:59am, <dario.faggioli@xxxxxxxxxx> wrote:
> > 
> > > Also I'd highlight the below modification:
> > > -    if ( !spin_trylock(&pcidevs_lock) )
> > > -        return -ERESTART;
> > > -
> > > +    pcidevs_lock();
> > > 
> > > IMO, it is right too.
> > Well, I'll have to see where exactly this is (pulling such out of
> > context is pretty
> > unhelpful), but I suspect it can't be replaced like this.
> > 
> Jan, I am looking forward to your review.
> btw, It is in the assign_device(), in the
> xen/drivers/passthrough/pci.c file.
> 
Mmm... If multiple cpus calls assign_device(), and the calls race, the
behavior between before and after the patch looks indeed different to
me.

In fact, in current code, the cpus that find the lock busy already,
would quit the function immediately and a continuation is created. On
the other hand, with the patch, they would spin and actually get the
lock, one after the other (if there's more of them) at some point.

Please, double check my reasoning, but I looks to me that it is indeed
different what happens when the hypercall is restarted (i.e., in
current code) and what happens if we just let others take the lock and
execute the function (i.e., with the patch applied).

I suggest you try to figure out whether that is actually the case. Once
you've done, feel free to report here and ask for help for finding a
solution, if you don't see one.

> > > > but I'd add a mention of this in the changelog.
> > > In git log?
> > To be honest, changes like this would better not be buried in a big
> > rework like
> > the one here. Make it a prereq patch, where you then will kind of
> > automatically
> > describe why it's correct. (I agree current code is bogus, and
> > we're not hitting
> > the respective
> > BUG_ON() in check_lock() only because spin_debug gets set to true
> > only after
> > most __init code has run.)
> Agreed, I would make a prereq patch in v7.
> 
Ok. In general, I agree with Jan.

In this case, I suggested just mentioning in changelog as we curently
basically have a bug, and I think it would be fine to have something
like "Doing what we do also serves as a fix for a bug found in xxx
yyy".

But it's indeed Jan's call, and his solution is certainly cleaner.
Not to mention that, in the case of assign_device(), fixing that would
most likely require being done in a patch on its own anyway.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel