Re: [Xen-devel] [PATCH v5 5/7] VT-d: Refactor iommu_ops .map_page() and unmap_page()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.02.16 at 10:24, <quan.xu@xxxxxxxxx> wrote:
> On February 26, 2016 4:15pm, <JBeulich@xxxxxxxx> wrote:
>> >>> On 26.02.16 at 08:37, <quan.xu@xxxxxxxxx> wrote:
>> > On February 25, 2016 8:24pm, <JBeulich@xxxxxxxx> wrote:
>> >> >>> On 25.02.16 at 13:14, <quan.xu@xxxxxxxxx> wrote:
>> >> > On February 25, 2016 4:59pm, <JBeulich@xxxxxxxx> wrote:
>> >
>> >> >> However, the same effect could be achieved by making the lock a
>> >> >> recursive one, which would then seem to more conventional approach
>> >> >> (but requiring as much code to be touched).
>> >
>> > IMO, for v6, I'd better:
>> >   (1). replace all of 'spin_lock(&pcidevs_lock)' with
>> > 'spin_lock_recursive(&pcidevs_lock)',
>> >   (2). replace all of 'spin_unlock(&pcidevs_lock)' with
>> > 'spin_unlock_recursive(&pcidevs_lock)',
>> >   (3). _do_not_ touch 'spin_is_locked(&pcidevs_lock)'.
>> > Then, ignore v5 [patch4/7], [patch5/7], [patch6/7],  *_locked()
>> > related code,  and add a new patch for the above (1). (2). (3). Right?
>> 
>> Yes.
>> 
>> > BTW, .e.g. 'd->page_alloc_lock', I found the 'd->page_alloc_lock' is
>> > called by both spin_lock_recursive() and 'spin_lock()'.
>> > _If_  both spin_lock_recursive(&d->page_alloc_lock) and
>> > 'spin_lock(&d->page_alloc_lock)' are recursively called in same call
>> > tree as below, it might be a bug. Right?
>> >
>> >
>> > {
>> > ...
>> >     spin_lock()
>> >     ...
>> >        spin_lock_recursive()
>> >        ...
>> >        spin_unlock_recursive()  <--- (lock might be now free)
>> >     ...
>> >     spin_unlock()
>> > ...
>> > }
>> 
>> Well, such a use of course would be a bug. But using plain
>> spin_lock() on a path where recursive acquire can't occur is fine.
>> 
>> Nevertheless I'd recommend not mixing things for the pcidevs one, as its uses
>> are scattered around too much for it to be reasonably possible to prove
>> correctness if done that way.
> 
> 
> But I do _not_ mix things for the pcidevs one. As 
>   (1). replace ___all___ of 'spin_lock(&pcidevs_lock)' with 
> 'spin_lock_recursive(&pcidevs_lock)',
>   (2). replace ___all___ of 'spin_unlock(&pcidevs_lock)' with 
> 'spin_unlock_recursive(&pcidevs_lock)',

I am not saying you mix things, I'm just saying there is a risk you
do perhaps not even knowingly, e.g. when another patch goes in
about the same time as yours which adds another instance. Plus
doing what you state above is going to be close to unreviewable,
since the reviewer would have to check that indeed you caught
all instances. By removing global visibility of the lock variable both
issues are easily avoided, since without catching all cases a build
error would result.

Jan


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