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

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

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)',



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