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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.10.15 at 13:05, <david.vrabel@xxxxxxxxxx> 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 <david.vrabel@xxxxxxxxxx>

Konrad, could you please take a look too?

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

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

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

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


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