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

[Sander Eikelenboom <linux@eikelenboom.it>]

Monday, January 12, 2015, 5:45:30 PM, you wrote:

> There is race when we clear the STATE_SCHED in the softirq
> - which allows the 'raise_softirq_for' to progress and
> schedule another dpci. During that time the other CPU could
> receive an interrupt and calls 'raise_softirq_for' and put
> the dpci on its per-cpu list. There would be two 'dpci_softirq'
> running at the same time (on different CPUs) where the
> dpci state is STATE_RUN (and STATE_SCHED is cleared). This
> ends up hitting:

>  if ( test_and_set_bit(STATE_RUN, &pirq_dpci->state) )
>         BUG()

> Instead of that put the dpci back on the per-cpu list to deal
> with later.

> The reason we can get his with this is when an interrupt
> affinity is set over multiple CPUs.

> Another potential fix would be to add a guard in the raise_softirq_for
> to check for 'STATE_RUN' bit being set and not schedule the
> dpci until that bit has been cleared.

> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Reported-by: Malcolm Crossley <malcolm.crossley@citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thanks again for the quick fix Konrad !

Tested it for the last half hour and everything seems fine,
so you can also stick on a tested-by if you like.

--
Sander



> ---
>  xen/drivers/passthrough/io.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)

> diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
> index ae050df..9b77334 100644
> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/io.c
> @@ -804,7 +804,17 @@ 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.
>           */