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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.02.15 at 16:31, <konrad.wilk@xxxxxxxxxx> wrote:
> On Mon, Feb 02, 2015 at 03:19:33PM +0000, Jan Beulich wrote:
>> >>> On 02.02.15 at 15:29, <konrad.wilk@xxxxxxxxxx> wrote:
>> > --- a/xen/drivers/passthrough/io.c
>> > +++ b/xen/drivers/passthrough/io.c
>> > @@ -63,10 +63,37 @@ enum {
>> >  static void raise_softirq_for(struct hvm_pirq_dpci *pirq_dpci)
>> >  {
>> >      unsigned long flags;
>> > +    unsigned long old, new, val = pirq_dpci->state;
>> >  
>> > -    if ( test_and_set_bit(STATE_SCHED, &pirq_dpci->state) )
>> > -        return;
>> > +    /*
>> > +     * This cmpxch is a more clear version of:
>> > +     * if ( test_and_set_bit(STATE_SCHED, &pirq_dpci->state) )
>> > +     *         return;
>> > +     * since it also checks for STATE_RUN conditions.
>> > +     */
>> > +    for ( ;; )
>> > +    {
>> > +        new = 1 << STATE_SCHED;
>> > +        if ( val )
>> > +            new |= val;
>> 
>> Why the if()?
> 
> To 'or' the variable new with '1 << STATE_RUN' in case 'val' changed from
> the first read ('val = pirq_dpci->state') to the moment when
> we do the cmpxchg.

But if "val" is zero, the | simply will do nothing.

>> > +        old = cmpxchg(&pirq_dpci->state, val, new);
>> > +        switch ( old )
>> > +        {
>> > +        case (1 << STATE_SCHED):
>> > +        case (1 << STATE_RUN) | (1 << STATE_SCHED):
>> > +            /*
>> > +             * Whenever STATE_SCHED is set we MUST not schedule it.
>> > +             */
>> > +            return;
>> > +        }
>> > +        /*
>> > +         * If the 'state' is 0 or (1 << STATE_RUN) we can schedule it.
>> > +         */
>> 
>> Really? Wasn't the intention to _not_ schedule when STATE_RUN is
>> set? (Also the above is a only line comment, i.e. wants different style.)
> 
> I must confess I must have misread your last review. You said:
> 
>       > 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 ...
> 
>       > +    case (1 << STATE_RUN):
> 
>       ... in this case?
> 
> which to me implied you still want to schedule an 'dpci' when STATE_RUN is 
> set?
> 
> My thinking is that we should still schedule an 'dpci' when STATE_RUN is set
> as that is inline with what the old tasklet code did. It would
> schedule the tasklet the moment said tasklet was off the global list (but
> it would not add it in the global list - that would be the job of the
> tasklet function wrapper to detect and insert said tasklet back on
> the global list).

Didn't the original discussion (and issue) revolve around scheduling
while STATE_RUN was set? Hence the intention to wait for the flag
to clear - but preferably in an explicit rather than implicit (as your
current and previous patch do) manner.

Jan


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