Re: [Xen-devel] [PATCH v6 3/5] IOMMU: Make the pcidevs_lock a recursive one

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

On March 04, 2016 9:59pm, <dario.faggioli@xxxxxxxxxx> wrote:
> On Fri, 2016-03-04 at 11:54 +0000, Xu, Quan wrote:
> > On March 04, 2016 5:29pm, <JBeulich@xxxxxxxx> wrote:
> > > On March 04, 2016 7:59am, <dario.faggioli@xxxxxxxxxx> wrote:
> > >
> > > > Also I'd highlight the below modification:
> > > > -    if ( !spin_trylock(&pcidevs_lock) )
> > > > -        return -ERESTART;
> > > > -
> > > > +    pcidevs_lock();
> > > >
> > > > IMO, it is right too.
> > > Well, I'll have to see where exactly this is (pulling such out of
> > > context is pretty unhelpful), but I suspect it can't be replaced
> > > like this.
> > >
> > Jan, I am looking forward to your review.
> > btw, It is in the assign_device(), in the
> > xen/drivers/passthrough/pci.c file.
> >
> Mmm... If multiple cpus calls assign_device(), and the calls race, the 
> behavior
> between before and after the patch looks indeed different to me.
> 
> In fact, in current code, the cpus that find the lock busy already, would 
> quit the
> function immediately and a continuation is created. On the other hand, with 
> the
> patch, they would spin and actually get the lock, one after the other (if 
> there's
> more of them) at some point.
> 
> Please, double check my reasoning, but I looks to me that it is indeed 
> different
> what happens when the hypercall is restarted (i.e., in current code) and what
> happens if we just let others take the lock and execute the function (i.e., 
> with
> the patch applied).
> 
> I suggest you try to figure out whether that is actually the case. Once you've
> done, feel free to report here and ask for help for finding a solution, if 
> you don't
> see one.
> 

Good idea.
For multiple cpus calls assign_device(), Iet's assume that there are 3 calls in 
parallel:
  (p1). xl pci-attach TestDom 0000:81:00.0
  (p2). xl pci-attach TestDom 0000:81:00.0
  (p3). xl pci-attach TestDom 0000:81:00.0
 
Furthermore, p1 and p2 run on pCPU1, and p3 runs on pCPU2.

After my patch,
__IIUC__ , the invoker flow might be as follow:
pCPU1                                               pCPU2
 .                                                     .
 .                                                     .
 assign_device_1()
  {                                                    .
   spin_lock_r(lock)                                      .
   .                                                   assign_device_3() 
spin_lock_r(lock) <-- blocks
   assign_device_2()
     {                                                 x <-- spins
       spin_lock_r(lock) <-- can continue                     x <-- spins
       spin_unlock_r(lock) <-- *doesn't* release lock           x <-- spins
     }                                                 x <-- spins
     .                                                 x <-- spins
  }                                                    x <-- spins
   .                                                   x <-- spins
   spin_unlock_r(lock) <-- release lock ----------->. ....... 
...............<--assign_device_3() continue, with lock held
   .                                                   .
   .                                                   .
   .                                                   spin_unlock_r(lock) 
<--lock is now free


Befer my patch,
The invoker flower might return at the point of assign_device_2() / 
assign_device_3().

So, yes, If multiple cpus calls assign_device(), and the calls race, the 
behavior between before and after the patch looks indeed different.

I try to fix it with follow:
--------patch >> -------- 

--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -118,6 +118,11 @@ int pcidevs_is_locked(void)
     return spin_is_locked(&_pcidevs_lock);
 }

+int pcidevs_trylock(void)
+{
+    return spin_trylock_recursive(&_pcidevs_lock);
+}
+
 void __init pt_pci_init(void)
 {
     radix_tree_init(&pci_segments);
@@ -1365,7 +1370,7 @@ static int assign_device(struct domain *d, u16 seg, u8 
bus, u8 devfn, u32 flag)
              p2m_get_hostp2m(d)->global_logdirty)) )
         return -EXDEV;

-    if ( !spin_trylock(&pcidevs_lock) )
+    if ( !pcidevs_trylock() )
         return -ERESTART;

     rc = iommu_construct(d);
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 017aa0b..b87571d 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -97,6 +97,7 @@ struct pci_dev {
 void pcidevs_lock(void);
 void pcidevs_unlock(void);
 int pcidevs_is_locked(void);
+int pcidevs_trylock(void);

 bool_t pci_known_segment(u16 seg);
 bool_t pci_device_detect(u16 seg, u8 bus, u8 dev, u8 func);

--------patch << -------- 

A quick question, is it '-ERESTART', instead of '-EBUSY' ?

There is also a similar case, cpu_hotplug:
   $cpu_up()--> cpu_hotplug_begin()-->get_cpu_maps()--> 
spin_trylock_recursive(&cpu_add_remove_lock)

Feel free to share your idea, and correct me if I'm wrong.

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