Re: [Xen-devel] [PATCH v8 for-xen-4.5 2/2] dpci: Replace tasklet with an softirq (v8)

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.10.14 at 03:58, <konrad.wilk@xxxxxxxxxx> wrote:
> On Thu, Oct 23, 2014 at 10:36:07AM +0100, Jan Beulich wrote:
>> >>> On 21.10.14 at 19:19, <konrad.wilk@xxxxxxxxxx> wrote:
> Was not sure whether you prefer 'true'
> or 'false' instead of numbers - decided on numbers since most of the code-base
> uses numbers.

So far we don't use "true" and "false" in hypervisor code at all (or if
you spotted any such use, it slipped in by mistake). We ought to
consider switching to bool/true/false though.

> +/*
> + * Reset the pirq_dpci->dom parameter to NULL.
> + *
> + * This function checks the different states to make sure it can do
> + * at the right time and if unschedules the softirq before it has
> + * run it also refcounts (which is what the softirq would have done).
> + */
> +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));
> +    switch ( cmpxchg(&pirq_dpci->state, STATE_SCHED, 0) )
> +    {
> +        /*
> +         * We are going to try to de-schedule the softirq before it goes in
> +         * STATE_RUN. Whoever clears STATE_SCHED MUST refcount the 'dom'.
> +         */
> +        case STATE_SCHED:
> +            put_domain(d);
> +            /* fallthrough. */
> +        /*
> +         * The reason it is OK to reset 'dom' when STATE_RUN bit is set is 
> due
> +         * to a shortcut the 'dpci_softirq' implements. It stashes the 'dom' 
> in
> +         * a local variable before it sets STATE_RUN - and therefore will not
> +         * dereference '->dom' which would result in a crash.
> +        */
> +        case STATE_RUN:
> +            pirq_dpci->dom = NULL;
> +            break;
> +        default:
> +            break;
> +    }

Apart from the indentation being wrong (case labels having the same
indentation as the switch()'s opening brace), this doesn't seem to be a
proper equivalent of the former code: There you cleared ->dom when
STATE_RUN regardless of STATE_SCHED. Leaving out the comments
I'd suggest

    switch ( cmpxchg(&pirq_dpci->state, STATE_SCHED, 0) )
    {
    case STATE_SCHED:
        put_domain(d);
    case STATE_RUN: case STATE_SCHED|STATE_RUN:
        pirq_dpci->dom = NULL;
        break;
    default:
        BUG();
    case 0:
        break;
    }

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -767,40 +767,51 @@ static int pci_clean_dpci_irq(struct domain *d,
>          xfree(digl);
>      }
>  
> -    tasklet_kill(&pirq_dpci->tasklet);
> -
> -    return 0;
> +    return pt_pirq_softirq_active(pirq_dpci);

This function returns a bool_t now, but the (indirect) caller of this
function expects -ERESTART.

Jan


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