Re: [PATCH v6 for-xen-4.5 3/3] dpci: Replace tasklet with an softirq (v6)

["Jan Beulich" <JBeulich@suse.com>]

>>> On 25.09.14 at 17:27, <konrad.wilk@oracle.com> wrote:
> On Thu, Sep 25, 2014 at 03:55:28PM +0100, Jan Beulich wrote:
>> >>> On 23.09.14 at 04:10, <konrad.wilk@oracle.com> wrote:
>> > +/*
>> > + * If we are racing with softirq_dpci (masked is still set) we return
>> > + * -EAGAIN. Otherwise we return 0.
>> > + *
>> > + *  If it is -EAGAIN, it is the callers responsibility to kick the softirq
>> > + *  (with the event_lock dropped).
>> 
>> But pt_pirq_cleanup_check() doesn't do this - is the comment
>> misleading or that particular call site reacting wrongly? Actually the
>> other call site doesn't kick any softirq either - what am I missing here?
> 
> The one call side that does is the 'pt_pirq_create..' which calls
> 'pt_pirq_reset'. The other ones:
>  a) domain_kill->domain_relinquish_resources->pci_release_devices->pci_clean_dpci_irq
>  b) pt_pirq_cleanup_check
> 
> are missing it. It is easy with a)- just add the process_pending_softirq()) in
> when we are not holding the lock. But b) is much harder as we would need to
> alter the whole 'pirq_cleanup_check' to return an error (as the callers of
> 'pirq_cleanup_check' are holding the lock) and perculate that up..

Hmm, perhaps I'm misunderstanding "kick" then: If all you want is
for it to be executed, you don't need to do anything on the -EAGAIN
way out of domain_relinquish_resources().

> One way to do this is by ignoring the 'pt_pirq_cleanup_check' case as
> the ramifications of that is that we would either re-use the 'pirq'
> in pt_irq_create_bind or pick 'pirq' up at pci_clean_dpci_irq and then
> remove it (and deal with the process_pending_softirq()).

As long as that's safe to do...

>> > +    if ( pt_pirq_reset(d, pirq_dpci) )
>> > +    {
>> > +        spin_unlock(&d->event_lock);
>> > +        process_pending_softirqs();
>> > +        if ( ( NOW() - start ) >> 30 )
>> > +            return -EAGAIN;
>> > +        goto restart;
>> > +    }
>> 
>> ... this still looks more like a hack, and I'm still not really certain
>> why between two uses (which is what I understand this is for) the
>> pIRQ (and hence it's softirq instance) won't be fully quiesced.
> 
> Just to make it clear - the 'pirq_guest_unbind' (which is called in the
> pt_irq_destroy_bind) will take care of removing the action. So no more
> __do_IRQ calls using the 'pirq' after that.
> 
> But we might have a pending softirq after we finished with 
> pt_irq_destroy_bind.
> And this loop will take care of waiting it out. This problem had
> existed prior to this patch - this wait loop was done inside the 
> 'tasklet_kill'.
> 
> I added the 1 second timeout as I am not a fan of unbound loops. But
> I can put it back in to make it simpler (and look less hacky).

If a softirq doesn't get run in a timely manner we're in bigger trouble
than what would warrant a timeout here. Perhaps simply put a
comment there referring to tasklet_kill() doing effectively the same
thing?

Jan