Re: [Xen-devel] [RFC PATCH] dpci: Put the dpci back on the list if running on another CPU.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Mar 17, 2015 at 09:42:21AM +0100, Sander Eikelenboom wrote:
> 
> Tuesday, March 17, 2015, 9:18:32 AM, you wrote:
> 
> >>>> On 16.03.15 at 18:59, <konrad.wilk@xxxxxxxxxx> wrote:
> >> Hence was wondering if it would just be easier to put
> >> this patch in (see above) - with the benfit that folks have
> >> an faster interrupt passthrough experience and then I work on another
> >> variant of this with tristate cmpxchg and ->mapping atomic counter.
> 
> > Considering how long this issue has been pending I think we really
> > need to get _something_ in (or revert); if this something is the
> > patch in its most recent form, so be it (even if maybe not the
> > simplest of all possible variants). So please submit as a proper non-
> > RFC patch.
> 
> > Jan
> 
> I'm still running with this first simple stopgap patch from Konrad,
> and it has worked fine for me since.

I believe the patch that Sander and Malcom had been running is the best
candidate.

The other ones I had been fiddling with - such as the one attached
here - I cannot make myself comfortable that it will not hit
a dead-lock. On Intel hardware the softirq is called
from the vmx_resume - which means that the whole 'interrupt guest' and
deliever the event code happens during the VMEXIT to VMENTER time. But that
does not preclude another interrupt destined for this same vCPU
to come right in as we are progressing through the softirqs - and 
dead-lock: in the vmx_resume stack we are in hvm_dirq_assist
(called from dpci_softirq) and haven't cleared the STATE_SHED, while in
the IRQ stack we spin in the raise_sofitrq_for for the STATE_SCHED to be 
cleared.

An dead-lock avoidance could be added to save the CPU value of
the softirq that is executing the dpci. And then 'raise_softirq_for'
can check that and bail out if (smp_processor_id == dpci_pirq->cpu).
Naturlly this means being very careful _where_ we initialize
the 'cpu' to -1, etc - which brings back to carefully work
out the corner cases and make sure we do the right thing - which can
take time.

The re-using the 'dpci' on the per-cpu list is doing the same
exact thing that older tasklet code was doing. That is :
If the function assigned to the tasklet was running  - the softirq
that ran said function (hvm_dirq_assist) would be responsible for
putting the tasklet back on the per-cpu list. This would allow
to have an running tasklet and an 'to-be-scheduled' tasklet
at the same time. 

And that is what we need. I will post an proper patch
and also add Tested-by from Malcom and Sander on it - as it did
fix their test-cases and is unmodified (except an updated
comment) from what theytested in 2014.

> 
> I will see if this new one also "works-for-me", somewhere today :-)
> 
> --
> Sander
> 
> 
> 
> diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
> index ae050df..d1421b0 100644
> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/io.c
> @@ -804,7 +804,18 @@ static void dpci_softirq(void)
>          d = pirq_dpci->dom;
>          smp_mb(); /* 'd' MUST be saved before we set/clear the bits. */
>          if ( test_and_set_bit(STATE_RUN, &pirq_dpci->state) )
> -            BUG();
> +        {
> +            unsigned long flags;
> +
> +            /* Put back on the list and retry. */
> +            local_irq_save(flags);
> +            list_add_tail(&pirq_dpci->softirq_list, &this_cpu(dpci_list));
> +            local_irq_restore(flags);
> +
> +            raise_softirq(HVM_DPCI_SOFTIRQ);
> +            continue;
> +        }
> +
>          /*
>           * The one who clears STATE_SCHED MUST refcount the domain.
>           */
>

Attachment: 0001-dpci-when-scheduling-spin-until-STATE_RUN-or-STATE_S.patch
Description: Text document

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