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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Fri, 2016-03-11 at 12:36 +0000, Xu, Quan wrote:
> On March 11, 2016 6:36pm, <Dario Faggioli> wrote:
> > On Fri, 2016-03-11 at 06:54 +0000, Xu, Quan wrote:
> > > 
> > So, now, if we know for sure that a lock is _never_ever_ever_ taken
> > from
> > interrupt context, can we mix spin_lock() and spin_lock_irq() on it
> > (for whatever
> > reason)? Well, as far as the above reasoning is concerned, yes.
> > 
> I appreciate Dario's explanation.
> btw, be careful of spin_lock_irq(), which includes
> 'ASSERT(local_irq_is_enabled()'..
> 
It does. What about it? That is exactly what marks the difference
between spin_lock_irq() and spin_lock_irqsave(). In fact, the former
should not be used if interrupts are already disabled because then,
when calling the corresponding spin_unlock_irq(), they'd be re-enabled
while instead they shouldn't.

But as said, from the point of view of preventing deadlock, for locks
taken both from inside and outside IRQ context, they're equivalent.

> > 
> > In fact, the deadlock arises because IRQs interrupt asynchronously
> > what the
> > CPU is doing, and that can happen when the CPU has taken the lock
> > already. But
> > if the 'asynchronous' part goes away, we really don't care whether
> > a lock is take
> > at time t1 with IRQ enabled, and at time t2 with IRQ disabled,
> > don't you think?
> > 
> Yes.
> Consistency may be helpful to avoid some easy-to-avoid lock errors.
> Moreover, without my fix, I think it would not lead dead lock, as the
> pcidevs_lock is not being taken
> In IRQ context. Right? 
> 
> 
> For deadlock, I think the key problems are:
>   - A lock can be acquired from IRQ context
>   -The interrupt is delivered to the _same_CPU_ that already holds
> the lock.
> 
In your case, pcidevs_lock is certainly not being taken from both
inside and outside IRQ context. If it where, using spin_lock() --as it
happens basically everywhere in the code-- would be wrong, and using
spin_lock_irq[save]() --as it happens in the only case you're patching-
- would be what would be right!

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