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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.03.16 at 12:27, <quan.xu@xxxxxxxxx> wrote:
> 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.

s/may/is going to/ (since right now that isn't the case yet)

>> > --- 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).

Ah, of course.

>> > @@ -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?

The outermost parentheses aren't needed, and harm readability.

Jan


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