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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.11.14 at 17:44, <konrad.wilk@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxx> 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) )

With that you correct your earlier reply, but I don't see how this
answers my original question.

Jan


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