Re: [Xen-devel] [PATCH v6 for-xen-4.5 1/3] dpci: Move from domain centric model to hvm_dirq_dpci model.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.10.14 at 17:40, <konrad.wilk@xxxxxxxxxx> wrote:
> @@ -65,10 +69,13 @@ static void schedule_softirq_for(struct hvm_pirq_dpci 
> *pirq_dpci)
>   */
>  int pt_pirq_softirq_active(struct hvm_pirq_dpci *pirq_dpci)
>  {
> -    if ( pirq_dpci->masked )
> -        return -EAGAIN;
> +    if ( test_bit(STATE_RUN, &pirq_dpci->masked) )
> +        return -ERESTART;
> +
> +    if ( test_bit(STATE_SCHED, &pirq_dpci->masked) )
> +        return -ERESTART;

Just check both in one go?

> @@ -230,7 +234,35 @@ int pt_irq_create_bind(
>              {
>                  pirq_dpci->gmsi.gflags = 0;
>                  pirq_dpci->gmsi.gvec = 0;
> -                pirq_dpci->dom = NULL;
> +
> +                /* The softirq has saved it so we are safe to reset it. */
> +                if ( test_bit(STATE_RUN, &pirq_dpci->masked) )
> +                {
> +                    pirq_dpci->dom = NULL;
> +                }
> +                else if ( test_and_clear_bit(STATE_SCHED, 
> &pirq_dpci->masked) )
> +                {
> +                    /* pirq_guest_unbind has made sure we won't be getting
> +                     * any more interrupts (no raise_softirq_for), so the 
> only
> +                     * ones that can be are the ones that got scheduled 
> _right_
> +                     * before the pirq_guest_unbind. If we can de-schedule 
> them
> +                     * that is good. The problem #1 is that the softirq 
> might be
> +                     * running and it has just set STATE_RUN while we cleared
> +                     * STATE_SCHED. That is OK, as right after the STATE_RUN 
> it
> +                     * will check the STATE_SCHED and if cleared it will 
> unclear
> +                     * STATE_RUN and ignore this pirq. We MUST put the 
> refcount
> +                     * down. */
> +                    put_domain(pirq_dpci->dom);
> +                    pirq_dpci->dom = NULL;
> +                }
> +                else
> +                {
> +                    /* !STATE_RUN (stale) and !STATE_SCHED.
> +                     * softirq will ignore this pirq, but we MUST put the 
> refcount
> +                     * down. */
> +                    put_domain(pirq_dpci->dom);
> +                    pirq_dpci->dom = NULL;
> +                }

First of all you don't seem to convert the setting of ->dom back to
what it was before (you said this is an incremental patch).

And then the else path is suspicious: Only when STATE_SCHED
is set there is a ref to be put. I.e. it always has to be the site
clearing that flag which also drops the ref. This is supported by
both else-if and else doing exactly the same (which can't really
be right).

> @@ -332,7 +364,7 @@ int pt_irq_create_bind(
>              {
>                  if ( pt_irq_need_timer(pirq_dpci->flags) )
>                      kill_timer(&pirq_dpci->timer);
> -                pirq_dpci->dom = NULL;
> +                /* TODO - function. pirq_dpci->dom = NULL; */

I wonder whether _here_ you really need this: Is it possible
for the softirq to get invoked when pirq_guest_bind() fails? If not,
only the other error path above would seem to need extra care.

> @@ -725,11 +754,24 @@ static void dpci_softirq(void)
>  
>      while ( !list_empty(&our_list) )
>      {
> +        struct domain *d;
> +
>          pirq_dpci = list_entry(our_list.next, struct hvm_pirq_dpci, 
> softirq_list);
>          list_del(&pirq_dpci->softirq_list);
>  
> -        hvm_dirq_assist(pirq_dpci);
> -        pirq_dpci->masked = 0;
> +        d = pirq_dpci->dom;
> +        barrier(); /* 'd' MUST be saved before we set/clear the bits. */

smp_mb() I think.

> +        if ( test_and_set_bit(STATE_RUN, &pirq_dpci->masked) )
> +            BUG();
> +        /* Asked to be descheduled. */
> +        if ( !test_and_clear_bit(STATE_SCHED, &pirq_dpci->masked) )

Invert the condition and ...

> +        {
> +            clear_bit(STATE_RUN, &pirq_dpci->masked);
> +            continue;
> +        }
> +        hvm_dirq_assist(d, pirq_dpci);
> +        put_domain(d);
> +        clear_bit(STATE_RUN, &pirq_dpci->masked);

... do the clear_bit() just once?

> --- a/xen/include/xen/hvm/irq.h
> +++ b/xen/include/xen/hvm/irq.h
> @@ -93,7 +93,7 @@ struct hvm_irq_dpci {
>  /* Machine IRQ to guest device/intx mapping. */
>  struct hvm_pirq_dpci {
>      uint32_t flags;
> -    bool_t masked;
> +    unsigned long masked;

Perhaps better "state" or some such?

Jan

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