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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Jan 23, 2015 at 09:37:53AM +0000, Jan Beulich wrote:
> >>> On 23.01.15 at 02:44, <konrad.wilk@oracle.com> wrote:
> > Here is a patch that does this. I don't yet have an setup to test
> > the failing scenario (working on that). I removed the part in
> > the softirq because with this patch I cannot see a way it would
> > ever get there (in the softirq hitting the BUG). 
> 
> Hmm, but how do you now schedule the second instance that needed ...
> 
> > --- a/xen/drivers/passthrough/io.c
> > +++ b/xen/drivers/passthrough/io.c
> > @@ -64,16 +64,24 @@ static void raise_softirq_for(struct hvm_pirq_dpci 
> > *pirq_dpci)
> >  {
> >      unsigned long flags;
> >  
> > -    if ( test_and_set_bit(STATE_SCHED, &pirq_dpci->state) )
> > +    switch ( cmpxchg(&pirq_dpci->state, 0, 1 << STATE_SCHED) )
> > +    {
> > +    case (1 << STATE_SCHED):
> > +    case (1 << STATE_RUN):
> 
> ... in this case?

We do need that. I was under the false understanding that the old
code would just drop it if it was running. However the old code
(tasklet only) followed the logic that:
 a) If it is scheduled (on the list), then don't put it there.
 b) If it is running (t->running), then don't put it there. However
    once the function is done (in the do_tasklet_work)  we detect
    that we tried to schedule it (t->scheduled_on is set to non-zero value)
    - and we put it back on the list.

I need to think about this some more.
> 
> >> Additionally I think it should be considered whether the bitmap
> >> approach of interpreting ->state is the right one, and we don't
> >> instead want a clean 3-state (idle, sched, run) model.
> > 
> > Could you elaborate a bit more please? As in three different unsigned int
> > (or bool_t) that set in what state we are in?
> 
> An enum { STATE_IDLE, STATE_SCHED, STATE_RUN }. Especially
> if my comment above turns out to be wrong, you'd have no real
> need for the SCHED and RUN flags to be set at the same time.

Seems we do need these four states: scheduled; running; schedulding while running; and idle.

> 
> Jan
>