passthrough: improve interrupt injection locking

PATCH In review David Vrabel 4 participants 13 messages Oct 23, 2015 – Oct 30, 2015

Share & tools

David Vrabel

author

2015-10-23 11:05 UTC #0 lore raw

When injecting an interrupt for a passthrough device into a guest, the per-domain event_lock is held, reducing performance when a guest has many VCPUs and high interrupt rates.

By using a per-interrupt lock in the hot paths, this contention is eliminated and performance improves (a bit).

For testing, a 32 VCPU guest with an NVME device assigned to it was used. Continual reads with small (512 B) blocks were performed on all 32 hardware queues simultaneously.

* Lock profiling:

Before (elapsed: 60 s):

(XEN) [ 3321.143155] Domain 1 event_lock:
(XEN) [ 3321.143158] lock: 14411627(00000005:90714AEF),

                      block:     6658599(00000003:709F82BD)

After (elapsed: 60 s):

(XEN) [ 1253.921427] Domain 2 event_lock:
(XEN) [ 1253.921429] lock: 8287(00000000:01AE517C),

                      block:          67(00000000:000D4C3A)

* Aggregate performance:

MB/s
Before 60.8
After 68.4

David

David Vrabel

author

2015-10-23 11:05 UTC #1 lore raw

PATCH 1/2 passthrough: use per-interrupt lock when injecting an interrupt

The use of the per-domain event_lock in hvm_dirq_assist() does not scale with many VCPUs or interrupts.

Add a per-interrupt lock to reduce contention. When a interrupt for a passthrough device is being setup or teared down, we must take both the event_lock and this new lock.

Signed-off-by David Vrabel <[email protected]>

---
 xen/drivers/passthrough/io.c | 34 +++++++++++++++++++++++-----------
 xen/include/xen/hvm/irq.h    |  1 +
 2 files changed, 24 insertions(+), 11 deletions(-)

xen/drivers/passthrough/io.c +23 ~~-11~~

index bda9374..7c86c20 100644
@@ -106,7 +106,7 @@ static void pt_pirq_softirq_reset(struct hvm_pirq_dpci *pirq_dpci)
 {
     struct domain *d = pirq_dpci->dom;
 
-    ASSERT(spin_is_locked(&d->event_lock));
+    ASSERT(spin_is_locked(&pirq_dpci->lock));
 
     switch ( cmpxchg(&pirq_dpci->state, 1 << STATE_SCHED, 0) )
     {

@@ -209,7 +209,6 @@ int pt_irq_create_bind(
     if ( pirq < 0 || pirq >= d->nr_pirqs )
         return -EINVAL;
 
- restart:
     spin_lock(&d->event_lock);
 
     hvm_irq_dpci = domain_get_irq_dpci(d);

@@ -237,6 +236,8 @@ int pt_irq_create_bind(
     }
     pirq_dpci = pirq_dpci(info);
 
+    spin_lock(&pirq_dpci->lock);
+
     /*
      * A crude 'while' loop with us dropping the spinlock and giving
      * the softirq_dpci a chance to run.

@@ -245,11 +246,11 @@ int pt_irq_create_bind(
      * would have spun forever and would do the same thing (wait to flush out
      * outstanding hvm_dirq_assist calls.
      */
-    if ( pt_pirq_softirq_active(pirq_dpci) )
+    while ( pt_pirq_softirq_active(pirq_dpci) )
     {
-        spin_unlock(&d->event_lock);
+        spin_unlock(&pirq_dpci->lock);
         cpu_relax();
-        goto restart;
+        spin_lock(&pirq_dpci->lock);
     }
 
     switch ( pt_irq_bind->irq_type )

@@ -301,6 +302,7 @@ int pt_irq_create_bind(
                 pirq_dpci->dom = NULL;
                 pirq_dpci->flags = 0;
                 pirq_cleanup_check(info, d);
+                spin_unlock(&pirq_dpci->lock);
                 spin_unlock(&d->event_lock);
                 return rc;
             }

@@ -311,6 +313,7 @@ int pt_irq_create_bind(
 
             if ( (pirq_dpci->flags & mask) != mask )
             {
+                spin_unlock(&pirq_dpci->lock);
                 spin_unlock(&d->event_lock);
                 return -EBUSY;
             }

@@ -331,6 +334,7 @@ int pt_irq_create_bind(
         dest_mode = !!(pirq_dpci->gmsi.gflags & VMSI_DM_MASK);
         dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
         pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id;
+        spin_unlock(&pirq_dpci->lock);
         spin_unlock(&d->event_lock);
         if ( dest_vcpu_id >= 0 )
             hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]);

@@ -351,6 +355,7 @@ int pt_irq_create_bind(
 
         if ( !digl || !girq )
         {
+            spin_unlock(&pirq_dpci->lock);
             spin_unlock(&d->event_lock);
             xfree(girq);
             xfree(digl);

@@ -412,6 +417,7 @@ int pt_irq_create_bind(
                 hvm_irq_dpci->link_cnt[link]--;
                 pirq_dpci->flags = 0;
                 pirq_cleanup_check(info, d);
+                spin_unlock(&pirq_dpci->lock);
                 spin_unlock(&d->event_lock);
                 xfree(girq);
                 xfree(digl);

@@ -419,6 +425,7 @@ int pt_irq_create_bind(
             }
         }
 
+        spin_unlock(&pirq_dpci->lock);
         spin_unlock(&d->event_lock);
 
         if ( iommu_verbose )

@@ -430,6 +437,7 @@ int pt_irq_create_bind(
     }
 
     default:
+        spin_unlock(&pirq_dpci->lock);
         spin_unlock(&d->event_lock);
         return -EOPNOTSUPP;
     }

@@ -481,6 +489,8 @@ int pt_irq_destroy_bind(
     pirq = pirq_info(d, machine_gsi);
     pirq_dpci = pirq_dpci(pirq);
 
+    spin_lock(&pirq_dpci->lock);
+
     if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_MSI )
     {
         unsigned int bus = pt_irq_bind->u.pci.bus;

@@ -549,6 +559,7 @@ int pt_irq_destroy_bind(
         pirq_cleanup_check(pirq, d);
     }
 
+    spin_unlock(&pirq_dpci->lock);
     spin_unlock(&d->event_lock);
 
     if ( what && iommu_verbose )

@@ -566,6 +577,7 @@ int pt_irq_destroy_bind(
 
 void pt_pirq_init(struct domain *d, struct hvm_pirq_dpci *dpci)
 {
+    spin_lock_init(&dpci->lock);
     INIT_LIST_HEAD(&dpci->digl_list);
     dpci->gmsi.dest_vcpu_id = -1;
 }

@@ -621,7 +633,7 @@ int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq)
     return 1;
 }
 
-/* called with d->event_lock held */
+/* called with pirq_dhci->lock held */
 static void __msi_pirq_eoi(struct hvm_pirq_dpci *pirq_dpci)
 {
     irq_desc_t *desc;

@@ -675,7 +687,7 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
 {
     ASSERT(d->arch.hvm_domain.irq.dpci);
 
-    spin_lock(&d->event_lock);
+    spin_lock(&pirq_dpci->lock);
     if ( test_and_clear_bool(pirq_dpci->masked) )
     {
         struct pirq *pirq = dpci_pirq(pirq_dpci);

@@ -687,7 +699,7 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
 
             if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI )
             {
-                spin_unlock(&d->event_lock);
+                spin_unlock(&pirq_dpci->lock);
                 return;
             }
         }

@@ -695,7 +707,7 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
         if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI )
         {
             vmsi_deliver_pirq(d, pirq_dpci);
-            spin_unlock(&d->event_lock);
+            spin_unlock(&pirq_dpci->lock);
             return;
         }

@@ -709,7 +721,7 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
         {
             /* for translated MSI to INTx interrupt, eoi as early as possible */
             __msi_pirq_eoi(pirq_dpci);
-            spin_unlock(&d->event_lock);
+            spin_unlock(&pirq_dpci->lock);
             return;
         }

@@ -723,7 +735,7 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
         ASSERT(pt_irq_need_timer(pirq_dpci->flags));
         set_timer(&pirq_dpci->timer, NOW() + PT_IRQ_TIME_OUT);
     }
-    spin_unlock(&d->event_lock);
+    spin_unlock(&pirq_dpci->lock);
 }
 
 static void __hvm_dpci_eoi(struct domain *d,

xen/include/xen/hvm/irq.h +1 -0

index 4c9cb20..8b8e461 100644
@@ -91,6 +91,7 @@ struct hvm_irq_dpci {
 
 /* Machine IRQ to guest device/intx mapping. */
 struct hvm_pirq_dpci {
+    spinlock_t lock;
     uint32_t flags;
     unsigned int state;
     bool_t masked;

Signature · 1 line

2.1.4

Jan Beulich

2015-10-27 11:56 UTC #2 lore raw

Re: [PATCHv1 1/2] passthrough: use per-interrupt lock when injecting an interrupt

Quoted 9 lines

Quoted 1 line
Quoted 1 line · On 23.10.15 at 13:05, <[email protected]> wrote:
The use of the per-domain event_lock in hvm_dirq_assist() does not scale with many VCPUs or interrupts.
Add a per-interrupt lock to reduce contention. When a interrupt for a passthrough device is being setup or teared down, we must take both the event_lock and this new lock.
Signed-off-by David Vrabel <[email protected]>

Konrad, could you please take a look too?

xen/drivers/passthrough/io.c int pt_irq_create_bind( · line 254

Quoted 13 lines

xen/drivers/passthrough/io.c +3 -3

@@ -245,11 +246,11 @@ int pt_irq_create_bind(
      * would have spun forever and would do the same thing (wait to flush out
      * outstanding hvm_dirq_assist calls.
      */
-    if ( pt_pirq_softirq_active(pirq_dpci) )
+    while ( pt_pirq_softirq_active(pirq_dpci) )
     {
-        spin_unlock(&d->event_lock);
+        spin_unlock(&pirq_dpci->lock);
         cpu_relax();
-        goto restart;
+        spin_lock(&pirq_dpci->lock);
     }

The comment ahead of pt_pirq_softirq_active() (mentioning event_lock) will thus need updating.

xen/drivers/passthrough/io.c int pt_irq_destroy_bind( · line 492

Quoted 5 lines

xen/drivers/passthrough/io.c +1 -0

@@ -481,6 +489,8 @@ int pt_irq_destroy_bind(
     pirq = pirq_info(d, machine_gsi);
     pirq_dpci = pirq_dpci(pirq);
 
+    spin_lock(&pirq_dpci->lock);

Considering that code further down in this function checks pirq_dpci to be non-NULL, this doesn't look safe (or else those checks should go away, as after this addition they would be likely to trigger e.g. Coverity warnings).

xen/drivers/passthrough/io.c int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq) · line 637

Quoted 7 lines

xen/drivers/passthrough/io.c +1 -1

@@ -621,7 +633,7 @@ int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq)
     return 1;
 }
 
-/* called with d->event_lock held */
+/* called with pirq_dhci->lock held */
 static void __msi_pirq_eoi(struct hvm_pirq_dpci *pirq_dpci)

The fact that this is a safe change to the locking model imo needs to be spelled out explicitly in the commit message. Afaict it's safe only because desc_guest_eoi() uses pirq for nothing else than to (atomically!) clear pirq->masked.

xen/drivers/passthrough/io.c static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) · line 693

Quoted 9 lines

xen/drivers/passthrough/io.c +1 -1

@@ -675,7 +687,7 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
 {
     ASSERT(d->arch.hvm_domain.irq.dpci);
 
-    spin_lock(&d->event_lock);
+    spin_lock(&pirq_dpci->lock);
     if ( test_and_clear_bool(pirq_dpci->masked) )
     {
         struct pirq *pirq = dpci_pirq(pirq_dpci);

Along the same lines - it's not obvious that the uses of pirq here are safe with event_lock no longer held. In particular I wonder about send_guest_pirq() - despite the other use in __do_IRQ_guest() not doing any locking either I'm not convinced this is correct.

Jan

Konrad Rzeszutek Wilk

2015-10-28 20:19 UTC #3 lore raw

Re: [PATCHv1 1/2] passthrough: use per-interrupt lock when injecting an interrupt

.snip..

Quoted 10 lines

Review int pt_irq_destroy_bind( · line 492
Quoted 5 lines
(quoted diff) +1 -0
@@ -481,6 +489,8 @@ int pt_irq_destroy_bind(
     pirq = pirq_info(d, machine_gsi);
     pirq_dpci = pirq_dpci(pirq);
 
+    spin_lock(&pirq_dpci->lock);
Considering that code further down in this function checks pirq_dpci to be non-NULL, this doesn't look safe (or else those checks should go away, as after this addition they would be likely to trigger e.g. Coverity warnings).

? The checks are for pirq_dpci->dom.

Quoted 18 lines

Review int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq) · line 637
Quoted 8 lines
(quoted diff) +1 -1
@@ -621,7 +633,7 @@ int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq)
     return 1;
 }
 
-/* called with d->event_lock held */
+/* called with pirq_dhci->lock held */
 static void __msi_pirq_eoi(struct hvm_pirq_dpci *pirq_dpci)
The fact that this is a safe change to the locking model imo needs to be spelled out explicitly in the commit message. Afaict it's safe only because desc_guest_eoi() uses pirq for nothing else than to (atomically!) clear pirq->masked.
Quoted 4 lines
(quoted diff) +0 -0
@@ -675,7 +687,7 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
 {
     ASSERT(d->arch.hvm_domain.irq.dpci);
 

Quoted 10 lines

Review static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) · line 693
Quoted 5 lines
(quoted diff) +1 -1
-    spin_lock(&d->event_lock);
+    spin_lock(&pirq_dpci->lock);
     if ( test_and_clear_bool(pirq_dpci->masked) )
     {
         struct pirq *pirq = dpci_pirq(pirq_dpci);
Along the same lines - it's not obvious that the uses of pirq here are safe with event_lock no longer held. In particular I wonder about send_guest_pirq() - despite the other use in __do_IRQ_guest() not doing any locking either I'm not convinced this is correct.

It seems that the event channel mechanism only uses the event channel lock when expanding and initializing (FIFO). For the old mechanism it was for binding, closing (uhuh), status, reset, and set_priority.

The 'closing' is interesting. What if the guest was closing the port while we are to notify it of an interrupt (so inside hvm_dirq_assist).

evtchn_close will take the event_lock call pirq_cleanup_check and calls pt_pirq_cleanup_check. We check for STATE_RUN (still set as dpci_softirq hasn't finished calling hvm_dirq_assist) so it returns zero .. and does not call radix_tree_delete.

Then evtchn_close goes to 'unlink_pirq_port' and then follows up with 'unmap_domain_pirq_emuirq'. That sets emuirq to IRQ_UNBOUND.

OK, now hvm_dirq_assist is now inside the first 'if' and checks:

684 if ( hvm_domain_use_pirq(d, pirq) )

which would have been true, but now is false (as emuirq is now IRQ_UNBOUND).

Lets also assume this is for an MSI.

The 'pt_irq_need_timer' returns 0:

142 return !(flags & (HVM_IRQ_DPCI_GUEST_MSI | HVM_IRQ_DPCI_TRANSLATE));

which hits this:

723 ASSERT(pt_irq_need_timer(pirq_dpci->flags));

and blows up.

If we are not compiled with 'debug=y' then we would set a timer which 8 ms later calls 'pt_irq_time_out'. It then clears the STATE_RUN and the softirq is done.

So lets switch back to 'evtchn_close'.. which unlocks the event_lock and is done.

The 'pt_irq_time_out' is pretty much a nop - takes the event_lock, does not iterate over the list as it is an MSI one so no legacy interrupts. Calls pt_irq_guest_eoi over the radix tree - and pt_irq_guest_eoi does nothing as the _HVM_IRQ_DPCI_EOI_LATCH_SHIFT is not set.

I think that is the only issue - when using the legacy event channels and the guest fiddling with the pirq. I suppose that means anybody using 'unmap_domain_pirq_emuirq' will hit this ASSERT(). That means:

arch_domain_soft_reset
physdev_unmap_pirq
evtchn_close

Thought you could 'fix' this by doing:

xen/drivers/passthrough/io.c +2 -2

index bda9374..6ea59db 100644
@@ -720,8 +720,8 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
          * guest will never deal with the irq, then the physical interrupt line
          * will never be deasserted.
          */
-        ASSERT(pt_irq_need_timer(pirq_dpci->flags));
-        set_timer(&pirq_dpci->timer, NOW() + PT_IRQ_TIME_OUT);
+        if (pt_irq_need_timer(pirq_dpci->flags))
+            set_timer(&pirq_dpci->timer, NOW() + PT_IRQ_TIME_OUT);
     }
     spin_unlock(&d->event_lock);
 }

Quoted 8 lines

Jan
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Jan Beulich

2015-10-29 09:11 UTC #4 lore raw

Re: [PATCHv1 1/2] passthrough: use per-interrupt lock when injecting an interrupt

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines · On 28.10.15 at 21:18, <[email protected]> wrote:
(quoted diff) +0 -0
@@ -481,6 +489,8 @@ int pt_irq_destroy_bind(
     pirq = pirq_info(d, machine_gsi);
     pirq_dpci = pirq_dpci(pirq);
 

Quoted 8 lines

Quoted 6 lines
Review int pt_irq_destroy_bind( · line 492
Quoted 1 line
(quoted diff) +1 -0
+    spin_lock(&pirq_dpci->lock);
Considering that code further down in this function checks pirq_dpci to be non-NULL, this doesn't look safe (or else those checks should go away, as after this addition they would be likely to trigger e.g. Coverity warnings).
? The checks are for pirq_dpci->dom.

What about

        /* clear the mirq info */
        if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )

and

    if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) &&
         list_empty(&pirq_dpci->digl_list) )

? In fact I can't spot any access to pirq_dpci->dom in this function.

Quoted 16 lines

Quoted 4 lines
Quoted 4 lines
(quoted diff) +0 -0
@@ -675,7 +687,7 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
 {
     ASSERT(d->arch.hvm_domain.irq.dpci);
 
Quoted 10 lines
Review static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) · line 693
Quoted 5 lines
(quoted diff) +1 -1
-    spin_lock(&d->event_lock);
+    spin_lock(&pirq_dpci->lock);
     if ( test_and_clear_bool(pirq_dpci->masked) )
     {
         struct pirq *pirq = dpci_pirq(pirq_dpci);
Along the same lines - it's not obvious that the uses of pirq here are safe with event_lock no longer held. In particular I wonder about send_guest_pirq() - despite the other use in __do_IRQ_guest() not doing any locking either I'm not convinced this is correct.

Quoted 3 lines

It seems that the event channel mechanism only uses the event channel lock when expanding and initializing (FIFO). For the old mechanism it was for binding, closing (uhuh), status, reset, and set_priority.

Well, the event lock is also used for some pIRQ management iirc.

Jan

Konrad Rzeszutek Wilk

2015-10-30 14:45 UTC #5 lore raw

Re: [PATCHv1 1/2] passthrough: use per-interrupt lock when injecting an interrupt

Quoted 16 lines · Jan Beulich

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines · On 28.10.15 at 21:18, <[email protected]> wrote:
(quoted diff) +0 -0
@@ -481,6 +489,8 @@ int pt_irq_destroy_bind(
     pirq = pirq_info(d, machine_gsi);
     pirq_dpci = pirq_dpci(pirq);
 
Quoted 8 lines
Quoted 6 lines
Review int pt_irq_destroy_bind( · line 492
Quoted 1 line
(quoted diff) +1 -0
+    spin_lock(&pirq_dpci->lock);
Considering that code further down in this function checks pirq_dpci to be non-NULL, this doesn't look safe (or else those checks should go away, as after this addition they would be likely to trigger e.g. Coverity warnings).
? The checks are for pirq_dpci->dom.
What about

Quoted 7 lines

        /* clear the mirq info */
        if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )

and

    if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) &&
         list_empty(&pirq_dpci->digl_list) )

Yes. I was looking at the function code (pt_irq_create_bind) not the pt_irq_destroy_bind!

Quoted 19 lines

? In fact I can't spot any access to pirq_dpci->dom in this function.
Quoted 16 lines
Quoted 14 lines
Review static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) · line 693
Quoted 9 lines
(quoted diff) +1 -1
@@ -675,7 +687,7 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
 {
     ASSERT(d->arch.hvm_domain.irq.dpci);
 
-    spin_lock(&d->event_lock);
+    spin_lock(&pirq_dpci->lock);
     if ( test_and_clear_bool(pirq_dpci->masked) )
     {
         struct pirq *pirq = dpci_pirq(pirq_dpci);
Along the same lines - it's not obvious that the uses of pirq here are safe with event_lock no longer held. In particular I wonder about send_guest_pirq() - despite the other use in __do_IRQ_guest() not doing any locking either I'm not convinced this is correct.

Quoted 5 lines

Quoted 3 lines
It seems that the event channel mechanism only uses the event channel lock when expanding and initializing (FIFO). For the old mechanism it was for binding, closing (uhuh), status, reset, and set_priority.
Well, the event lock is also used for some pIRQ management iirc.

Correct - pirq_guest_bind, pirq_guest_unbind, map_domain_pirq, map_domain_emuirq_pirq, unmap_domain_pirq_emuirq, and so on.

Quoted 3 lines

Jan

David Vrabel

author

2015-10-23 11:05 UTC #6 lore raw

PATCH 2/2 passthrough: improve locking when iterating over interrupts bound to VMs

In reply to David Vrabel

radix_tree_gang_lookup() only requires a RCU read lock, not the per-domain event_lock.

Introduce a new RCU read lock and take the per-interrupt lock before calling the callback instead.

This eliminates all contention on the event_lock when injecting interrupts from passthrough devices.

Signed-off-by David Vrabel <[email protected]>

---
 xen/drivers/passthrough/io.c | 10 +++++++---
 xen/include/xen/sched.h      |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

xen/drivers/passthrough/io.c +7 -3

index 7c86c20..9b51ef0 100644
@@ -601,7 +601,7 @@ int pt_pirq_iterate(struct domain *d,
     unsigned int pirq = 0, n, i;
     struct pirq *pirqs[8];
 
-    ASSERT(spin_is_locked(&d->event_lock));
+    rcu_read_lock(&d->pirq_rcu_lock);
 
     do {
         n = radix_tree_gang_lookup(&d->pirq_tree, (void **)pirqs, pirq,

@@ -610,12 +610,18 @@ int pt_pirq_iterate(struct domain *d,
         {
             struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirqs[i]);
 
+            spin_lock(&pirq_dpci->lock);
+
             pirq = pirqs[i]->pirq;
             if ( (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
                 rc = cb(d, pirq_dpci, arg);
+
+            spin_unlock(&pirq_dpci->lock);
         }
     } while ( !rc && ++pirq < d->nr_pirqs && n == ARRAY_SIZE(pirqs) );
 
+    rcu_read_unlock(&d->pirq_rcu_lock);
+
     return rc;
 }

@@ -678,9 +684,7 @@ void hvm_dpci_msi_eoi(struct domain *d, int vector)
     if ( !iommu_enabled || !d->arch.hvm_domain.irq.dpci )
        return;
 
-    spin_lock(&d->event_lock);
     pt_pirq_iterate(d, _hvm_dpci_msi_eoi, (void *)(long)vector);
-    spin_unlock(&d->event_lock);
 }
 
 static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)

xen/include/xen/sched.h +1 -0

index 3729b0f..ae98c1e 100644
@@ -355,6 +355,7 @@ struct domain
      */
     struct radix_tree_root pirq_tree;
     unsigned int     nr_pirqs;
+    rcu_read_lock_t pirq_rcu_lock;
 
     enum guest_type guest_type;

Signature · 1 line

2.1.4

Jan Beulich

2015-10-27 12:45 UTC #7 lore raw

Re: [PATCHv1 2/2] passthrough: improve locking when iterating over interrupts bound to VMs

Quoted 3 lines

Quoted 1 line
Quoted 1 line · On 23.10.15 at 13:05, <[email protected]> wrote:
radix_tree_gang_lookup() only requires a RCU read lock, not the per-domain event_lock.

... when not caring about a consistent snapshot.

xen/drivers/passthrough/io.c void hvm_dpci_msi_eoi(struct domain *d, int vector) · line 688

Quoted 8 lines

xen/drivers/passthrough/io.c +0 -2

@@ -678,9 +684,7 @@ void hvm_dpci_msi_eoi(struct domain *d, int vector)
     if ( !iommu_enabled || !d->arch.hvm_domain.irq.dpci )
        return;
 
-    spin_lock(&d->event_lock);
     pt_pirq_iterate(d, _hvm_dpci_msi_eoi, (void *)(long)vector);
-    spin_unlock(&d->event_lock);
 }

How about other callers of pt_pirq_iterate()?

Jan

David Vrabel

author

2015-10-27 13:11 UTC #8 lore raw

Re: [PATCHv1 2/2] passthrough: improve locking when iterating over interrupts bound to VMs

Quoted 10 lines · Jan Beulich

Quoted 3 lines
Quoted 1 line
Quoted 1 line · On 23.10.15 at 13:05, <[email protected]> wrote:
radix_tree_gang_lookup() only requires a RCU read lock, not the per-domain event_lock.
... when not caring about a consistent snapshot.
Quoted 4 lines
xen/drivers/passthrough/io.c +0 -0
@@ -678,9 +684,7 @@ void hvm_dpci_msi_eoi(struct domain *d, int vector)
     if ( !iommu_enabled || !d->arch.hvm_domain.irq.dpci )
        return;
 

Quoted 6 lines

xen/drivers/passthrough/io.c void hvm_dpci_msi_eoi(struct domain *d, int vector) · line 688
Quoted 4 lines
xen/drivers/passthrough/io.c +0 -2
-    spin_lock(&d->event_lock);
     pt_pirq_iterate(d, _hvm_dpci_msi_eoi, (void *)(long)vector);
-    spin_unlock(&d->event_lock);
 }
How about other callers of pt_pirq_iterate()?

They weren't hot so I didn't look at them.

David

Konrad Rzeszutek Wilk

2015-10-28 20:42 UTC #9 lore raw

Re: [PATCHv1 2/2] passthrough: improve locking when iterating over interrupts bound to VMs

In reply to David Vrabel

Quoted 2 lines · David Vrabel

radix_tree_gang_lookup() only requires a RCU read lock, not the per-domain event_lock.

Don't you need to make some form of 'spin_lock_init' call?

Quoted 6 lines

Introduce a new RCU read lock and take the per-interrupt lock before calling the callback instead.
This eliminates all contention on the event_lock when injecting interrupts from passthrough devices.

This pt_pirq_iterate is called from:

hvm_migrate_pirqs (which only uses irq_desc lock) - so you could also drop the event_lock there.

pt_irq_time_out (which is only called for legacy interrupts and only at 8ms). Uses event_lock.

pci_clean_dpci_irqs (during domain shutdown, uses event_lock).

Also should this rcu lock be put in 'pirq_guest_unmask' (which oddly enough has no lock?!)?

Reading the radix-tree.h t says: "
..
The notable exceptions to this rule are the following functions:
...
radix_tree_gang_lookup

The first 7 functions are able to be called locklessly, using RCU. The caller must ensure calls to these functions are made within rcu_read_lock() regions."

Great, so that is what your patch does. Thought it would imply that everybody using radix_tree_gang_lookup on pirq_tree needs to use RCU?

Quoted 69 lines

Signed-off-by David Vrabel <[email protected]>

---
 xen/drivers/passthrough/io.c | 10 +++++++---
 xen/include/xen/sched.h      |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

xen/drivers/passthrough/io.c +7 -3

index 7c86c20..9b51ef0 100644
@@ -601,7 +601,7 @@ int pt_pirq_iterate(struct domain *d,
     unsigned int pirq = 0, n, i;
     struct pirq *pirqs[8];
 
-    ASSERT(spin_is_locked(&d->event_lock));
+    rcu_read_lock(&d->pirq_rcu_lock);
 
     do {
         n = radix_tree_gang_lookup(&d->pirq_tree, (void **)pirqs, pirq,

@@ -610,12 +610,18 @@ int pt_pirq_iterate(struct domain *d,
         {
             struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirqs[i]);
 
+            spin_lock(&pirq_dpci->lock);
+
             pirq = pirqs[i]->pirq;
             if ( (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
                 rc = cb(d, pirq_dpci, arg);
+
+            spin_unlock(&pirq_dpci->lock);
         }
     } while ( !rc && ++pirq < d->nr_pirqs && n == ARRAY_SIZE(pirqs) );
 
+    rcu_read_unlock(&d->pirq_rcu_lock);
+
     return rc;
 }

@@ -678,9 +684,7 @@ void hvm_dpci_msi_eoi(struct domain *d, int vector)
     if ( !iommu_enabled || !d->arch.hvm_domain.irq.dpci )
        return;
 
-    spin_lock(&d->event_lock);
     pt_pirq_iterate(d, _hvm_dpci_msi_eoi, (void *)(long)vector);
-    spin_unlock(&d->event_lock);
 }
 
 static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)

xen/include/xen/sched.h +1 -0

index 3729b0f..ae98c1e 100644
@@ -355,6 +355,7 @@ struct domain
      */
     struct radix_tree_root pirq_tree;
     unsigned int     nr_pirqs;
+    rcu_read_lock_t pirq_rcu_lock;
 
     enum guest_type guest_type;

Signature · 7 lines

2.1.4


_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Ian Campbell

2015-10-23 12:41 UTC #10 lore raw

In reply to David Vrabel

Quoted 3 lines · David Vrabel

When injecting an interrupt for a passthrough device into a guest, the per-domain event_lock is held, reducing performance when a guest has many VCPUs and high interrupt rates.

Did you CC me due to a possible impact on ARM? If so then I think since ARM lacks this "dpci" stuff none of these changes should have any impact on that arch.

If you think I've missed something or you CCd me for some other reason please let me know.

Thanks,
Ian.

Quoted 30 lines

By using a per-interrupt lock in the hot paths, this contention is eliminated and performance improves (a bit).
For testing, a 32 VCPU guest with an NVME device assigned to it was used. Continual reads with small (512 B) blocks were performed on all 32 hardware queues simultaneously.
* Lock profiling:
Before (elapsed: 60 s):
(XEN) [ 3321.143155] Domain 1 event_lock:
(XEN) [ 3321.143158] lock: 14411627(00000005:90714AEF),
                      block:     6658599(00000003:709F82BD)
After (elapsed: 60 s):
(XEN) [ 1253.921427] Domain 2 event_lock:
(XEN) [ 1253.921429] lock: 8287(00000000:01AE517C),
                      block:          67(00000000:000D4C3A)
* Aggregate performance:
MB/s
Before 60.8
After 68.4
David

David Vrabel

author

2015-10-23 12:39 UTC #11 lore raw

Quoted 11 lines · Ian Campbell

Quoted 3 lines · David Vrabel
When injecting an interrupt for a passthrough device into a guest, the per-domain event_lock is held, reducing performance when a guest has many VCPUs and high interrupt rates.
Did you CC me due to a possible impact on ARM? If so then I think since ARM lacks this "dpci" stuff none of these changes should have any impact on that arch.
If you think I've missed something or you CCd me for some other reason please let me know.

This series seems to fall into "THE REST" category.

David

Ian Campbell

2015-10-23 12:45 UTC #12 lore raw

Quoted 12 lines · David Vrabel

Quoted 11 lines · Ian Campbell
Quoted 4 lines · David Vrabel
When injecting an interrupt for a passthrough device into a guest, the
per-domain event_lock is held, reducing performance when a guest has many VCPUs and high interrupt rates.
Did you CC me due to a possible impact on ARM? If so then I think since ARM
lacks this "dpci" stuff none of these changes should have any impact on that arch.

Quoted 4 lines

Quoted 2 lines
If you think I've missed something or you CCd me for some other reason please let me know.
This series seems to fall into "THE REST" category.

Ah, only Jan and I were CCd so I didn't think of that.

I think despite the paths this ends up being x86 specific, so I'll leave it to Jan.

Ian.