Re: [PATCH v3 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3214 bytes --]

On Fri, 2016-03-11 at 09:41 -0500, Meng Xu wrote:
> Thank you very much for your explanation and education! They are
> really helpful! :-D
> 
> Let me summarize: ;-)
> > 
> >                                                      | spin_lock |
> spin_lock_irq | spin_lock_irqsave
> > 
> > Can run in irq context?                  | No            |  Yes
>         | Yes
> > 
> > Can run in irq disabled context?   |
> > No            |  No                | Yes
>
The table came out mangled, at least in my mail reader. For what I can
see, I think I see the point you're trying to make, and it's hard to
say whether the table itself is right or wrong...

Probably, a table like this, is just not the best way with which to try
to represent things.

For instance, you seem to be saying that spin_lock() can't be used if
interrupts are disabled, which is not true.

For instance, it is perfectly fine to do the following:

    CPU:
    .
    spin_lock_irq(l1);
    .
    .
[1] spin_lock(l2);
    .
    .
[2] spin_unlock(l2);
    .
    .
    spin_unlock_irq(l1);
    .

Even if l2 is also taken in an interrupt handler. In fact, what we want
is make sure that such an interrupt handler that may take l2, does not
interrupt CPU in the [1]--[2] time frame... But that can't happen
because interrupts are already disabled, so just using spin_lock() is
ok, and should be done, as that's cheaper than spin_lock_irqsave().

Fact is, what really matters is whether or not a lock is taken both
inside and outside of IRQ context. As a matter of fact, it is mostly
the case that, if a lock is ever taken inside an interrupt handler,
then spin_lock_irqsave() is what you want... but this does not justify
coming up with a table like the one above and claiming it to be
general.

> Why deadlock may occur if we mix the spin_lock and
> spin_lock_irq(save)?
> If we mix the spin_lock and spin_lock_irq(save), and a group of CPUs
> rendezvousing in an IPI handler, we will have deadlock. Because the
> CPU A that takes spin_lock will wait for the rendezvousing condition
> to be satisfied, while the CPU B that takes th spin_lock_irq(save)
> will not enter into the rendezvousing condition (since it disable the
> interrupt). Then,
> CPU A waits for CPU B to handle the IPI to get out of the
> rendezvousing condition (kind of synchrous point), which won't until
> it gets the spin_lock.
> CPU B waits for CPU A to release the spin_lock, which won't until it
> get out of the rendezvousing condition;
> 
> Are my understanding and summary correct?
> 
Yes, this looks a decent explanation of the rendezvous-related spinlock
problem... Although I prefer the wording that we do have already in
check_lock() and in start_secondary(). :-D

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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