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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Thu, Sep 25, 2014 at 03:55:28PM +0100, Jan Beulich wrote:
> >>> On 23.09.14 at 04:10, <konrad.wilk@xxxxxxxxxx> 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..

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()).

That leaves a) case which is simple enough. I will add that in.
> 
> > @@ -104,9 +148,20 @@ void free_hvm_irq_dpci(struct hvm_irq_dpci *dpci)
> >   *
> >   * As such on every 'pt_irq_create_bind' call we MUST reset the values.
> >   */
> > -static void pt_pirq_reset(struct domain *d, struct hvm_pirq_dpci *dpci)
> > +static int pt_pirq_reset(struct domain *d, struct hvm_pirq_dpci *dpci)
> >  {
> > +    /*
> > +     * We MUST check for this condition as the softirq could be scheduled
> > +     * and hasn't run yet. As such we MUST delay this reset until it has
> > +     * completed its job.
> > +     */
> > +    if ( !pt_pirq_cleanup_check(dpci) )
> > +            return -EAGAIN;
> > +
> > +    INIT_LIST_HEAD(&dpci->softirq_list);
> 
> What is this good for? This isn't a list head, and simple list elements
> don't need resetting of their link fields.

True. Left-over.
> 
> > @@ -116,7 +171,9 @@ int pt_irq_create_bind(
> >      struct hvm_pirq_dpci *pirq_dpci;
> >      struct pirq *info;
> >      int rc, pirq = pt_irq_bind->machine_irq;
> > +    s_time_t start = NOW();
> >  
> > + restart:
> >      if ( pirq < 0 || pirq >= d->nr_pirqs )
> >          return -EINVAL;
> 
> I don't think this check needs re-doing on each restart.

OK.
> 
> > @@ -146,7 +203,17 @@ int pt_irq_create_bind(
> >          return -ENOMEM;
> >      }
> >      pirq_dpci = pirq_dpci(info);
> > -    pt_pirq_reset(d, pirq_dpci);
> > +    /* A crude 'while' loop with us dropping the spinlock and giving
> > +     * the softirq_dpci a chance to run. We do this up to one second
> > +     * at which point we give up. */
> 
> Please fix the comment style, but ...
> 
> > +    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).

> 
> > +static void dpci_softirq(void)
> > +{
> > +    struct hvm_pirq_dpci *pirq_dpci;
> > +    unsigned int cpu = smp_processor_id();
> > +    LIST_HEAD(our_list);
> > +
> > +    local_irq_disable();
> > +    list_splice_init(&per_cpu(dpci_list, cpu), &our_list);
> > +    local_irq_enable();
> > +
> > +    while ( !list_empty(&our_list) )
> > +    {
> > +        pirq_dpci = list_entry(our_list.next, struct hvm_pirq_dpci, 
> > softirq_list);
> > +        list_del(&pirq_dpci->softirq_list);
> > +
> > +        hvm_dirq_assist(pirq_dpci);
> > +
> > +        put_domain(pirq_dpci->dom);
> > +        pirq_dpci->masked = 0;
> > +    }
> > +}
> > +static int cpu_callback(
> 
> There is again/still a blank line missing here.

Grrr.

Thank you again for your excellent review!
> 
> Jan
> 

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