Re: [Xen-devel] [PATCH v3 2/2] IOMMU/spinlock: Make the pcidevs_lock a recursive one

["Xu, Quan" <quan.xu@xxxxxxxxx>]

On March 10, 2016 5:53pm, <March 10, 2016 5:53> wrote:
> >>> On 09.03.16 at 14:17, <quan.xu@xxxxxxxxx> wrote:
> > Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> > Acked-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> 
> The patch itself looks mostly fine now (see below for the minor left issues), 
> but
> the complete lack of a description (which should state why this change is 
> being
> done) makes this not ready to go in anyway.
> 

What about the following description:
--
The pcidevs_lock may be recursively taken for hiding ATS device,
when VT-d Device-TLB flush timed out.



> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -48,7 +48,7 @@ struct pci_seg {
> >      } bus2bridge[MAX_BUSES];
> >  };
> >
> > -spinlock_t pcidevs_lock = SPIN_LOCK_UNLOCKED;
> > +static spinlock_t _pcidevs_lock = SPIN_LOCK_UNLOCKED;
> 
> Why the renaming?
> 

The original name would cause compiler errors, as I also introduce a new helper 
with same name -- void pcidevs_lock(void).


> > @@ -103,6 +103,26 @@ static int pci_segments_iterate(
> >      return rc;
> >  }
> >
> > +void pcidevs_lock(void)
> > +{
> > +    spin_lock_recursive(&_pcidevs_lock);
> > +}
> > +
> > +void pcidevs_unlock(void)
> > +{
> > +    spin_unlock_recursive(&_pcidevs_lock);
> > +}
> > +
> > +int pcidevs_is_locked(void)
> 
> bool_t
> 
> > +{
> > +    return spin_is_locked(&_pcidevs_lock); }
> > +
> > +int pcidevs_trylock(void)
> 
> bool_t
> 
> (To avoid another round, please be aware that the underlying spin lock
> primitives still [wrongly] use "int", so to be fully correct you will need to 
> use !! in
> both return statements, unless you feel like (in another prereq patch) to 
> adjust
> those primitives too.
> 
Thanks for reminding.
I'd prefer to use !! in both return statements.
btw, could you help me check the code style?:

+bool_t pcidevs_is_locked(void)
+{
+    return !!(spin_is_locked(&_pcidevs_lock));
+}
+
+bool_t pcidevs_trylock(void)
+{
+    return !!(spin_trylock_recursive(&_pcidevs_lock));
+}

Is it right?

> > +{
> > +    return spin_trylock_recursive(&_pcidevs_lock);
> > +}
> 
> I also think that it would be a good idea to place these helpers and the lock
> definition next to each other.
> 
Agreed, make sense.

Quan



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