Re: [PATCH v10 for-xen-4.5 2/2] dpci: Replace tasklet with an softirq

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

On Fri, Nov 14, 2014 at 11:11:46AM -0500, Konrad Rzeszutek Wilk wrote:
> On Fri, Nov 14, 2014 at 03:13:42PM +0000, Jan Beulich wrote:
> > >>> On 12.11.14 at 03:23, <konrad.wilk@oracle.com> wrote:
> > > +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, 1 << STATE_SCHED, 0) )
> > > +    {
> > > +    case (1 << STATE_SCHED):
> > > +        /*
> > > +         * We are going to try to de-schedule the softirq before it goes in
> > > +         * STATE_RUN. Whoever clears STATE_SCHED MUST refcount the 'dom'.
> > > +         */
> > > +        put_domain(d);
> > > +        /* fallthrough. */
> > 
> > Considering Sander's report, the only suspicious place I find is this
> > one: When the STATE_SCHED flag is set, pirq_dpci is on some
> > CPU's list. What guarantees it to get removed from that list before
> > getting inserted on another one?
> 
> None. The moment that STATE_SCHED is cleared, 'raise_softirq_for'
> is free to manipulate the list.

I was too quick to say this. A bit more inspection shows that while
'raise_softirq_for' is free to manipulate the list - it won't be called.

The reason is that the pt_pirq_softirq_reset is called _after_ the IRQ
action handler are removed for this IRQ. That means we will not receive
any interrupts for it and call 'raise_softirq_for'. At least until
'pt_irq_create_bind' is called. And said function has a check for
this too:

42      * A crude 'while' loop with us dropping the spinlock and giving            
243      * the softirq_dpci a chance to run.                                        
244      * We MUST check for this condition as the softirq could be scheduled       
245      * and hasn't run yet. Note that this code replaced tasklet_kill which      
246      * would have spun forever and would do the same thing (wait to flush out   
247      * outstanding hvm_dirq_assist calls.                                       
248      */                                                                         
249     if ( pt_pirq_softirq_active(pirq_dpci) )          

Hence the patch below is not needed.