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

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

On Mon, Sep 22, 2014 at 12:58:57PM +0100, Jan Beulich wrote:
> >>> On 19.09.14 at 20:51, <konrad.wilk@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/irq.c
> > +++ b/xen/arch/x86/irq.c
> > @@ -1337,6 +1337,12 @@ void (pirq_cleanup_check)(struct pirq *pirq, struct 
> > domain *d)
> >              return;
> >          if ( !pt_pirq_cleanup_check(&pirq->arch.hvm.dpci) )
> >              return;
> > +        /*
> > +         * Inhibit deletion from 'pirq_tree' so that 'pci_clean_dpci_irq'
> > +         * can still properly call 'dpci_kill'.
> > +         */
> > +        if ( dpci_kill(&pirq->arch.hvm.dpci) )
> > +            return;
> 
> If this is really necessary / The Right Thing To Do, it would clearly
> belong into pt_pirq_cleanup_check() rather than here, since it's the
> purpose of that function to deal with all pass-through related
> aspects of pIRQ cleanup.

OK. Will do.
> 
> > +int dpci_kill(struct hvm_pirq_dpci *pirq_dpci)
> > +{
> > +    if ( test_and_set_bit(STATE_SCHED, &pirq_dpci->state) )
> > +        return -EAGAIN;
> > +
> > +    if ( test_bit(STATE_RUN, &pirq_dpci->state) )
> > +        return -EAGAIN;
> > +
> > +    clear_bit(STATE_SCHED, &pirq_dpci->state);
> 
> So why do you first set and the immediately clear STATE_SCHED?

To make sure that we do not race with another
'schedule_for_dpci'/'dpci_softirq'.

Granted this is all 'if somebody did extend this in the future'
type code. That is not happening right now with all of this
and we could as well just depend on 'dpic->masked' being set.

> 
> > +
> > +    /* In case we turn around and call 'schedule_dpci_for' right away. */
> > +    INIT_LIST_HEAD(&pirq_dpci->softirq_list);
> 
> You needing this seems to hint at a problem elsewhere.

That actually is not happening at all. I added that code in case
somebody _did_ do this in the future. Perhaps a better approach
would be to mention this in the function header?

> 
> > @@ -131,6 +194,16 @@ int pt_irq_create_bind(
> >      }
> >      pirq_dpci = pirq_dpci(info);
> >  
> > +    /* Something is horrible wrong if it has been scheduled or is running. 
> > */
> > +    ASSERT(pirq_dpci->state == 0);
> > +
> > +    /* Guest unbinds (pt_irq_destroy_bind) and rebinds it back. */
> > +    if ( !pirq_dpci->dom )
> > +        pirq_dpci->dom = d;
> 
> And this looks more like a hack too. I think there should be exactly
> one place setting this field, and one or two (the second possibly in
> some error path following the setting of it) clearing it.

Right. The second patch (which I was thinking to squash in
but figured as a seperate patch might make it easier to understand)
does away with this and just does:

        pirq_dpci->dom = d;

here.

> 
> > @@ -156,7 +229,9 @@ int pt_irq_create_bind(
> >              {
> >                  pirq_dpci->gmsi.gflags = 0;
> >                  pirq_dpci->gmsi.gvec = 0;
> > +                pirq_dpci->dom = NULL;
> >                  pirq_dpci->flags = 0;
> > +                dpci_kill(pirq_dpci);
> >                  pirq_cleanup_check(info, d);
> 
> Now these dpci_kill() calls you insert not just here right before the
> pirq_cleanup_check() ones put an even bigger question mark on
> your change to pirq_cleanup_check().

The 'pirq_cleanup_check' macro calls the 'pirq_cleanup_check' only if
the event value is set. At this stage the event is not set (it is
called later by the OS to bind the pirq to the event channel -
map_domain_pirq) so it would never call 'dpci_kill' at this juncture.
Unless somebody setup an event channel first and then called this?

Inserting 'dpci_kill' was the best way to make sure we would still call it
and set the fields in the right state. However as patch #2 demonstrates
there is a potential race.

Perhaps the best way to do this is to put 'dpci_kill' in:

 1) At the start of this function (as patch #2 does), or
    'pt_irq_destroy_bind'. Your call in which function it should go.

 2) When the domain is being destroyed (in case we setup an MSI but
    never use them). Already implemented in this patch.

 3). In 'pt_pirq_cleanup_check' if the flags field is set to zero.

    (And we need to do that because if we remove the 'struct pirq'
     from the 'pirq_tree' radix tree, we will never be able to
     call 'dpci_kill' on it from 'pci_clean_dpci_irqs' as the
     field is gone).

And remove the various 'dpci_kill's in the error paths.
> 
> > @@ -625,3 +703,80 @@ void hvm_dpci_eoi(struct domain *d, unsigned int 
> > guest_gsi,
> >  unlock:
> >      spin_unlock(&d->event_lock);
> >  }
> > +
> > +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);
> > +
> > +        if ( !test_and_set_bit(STATE_RUN, &pirq_dpci->state) )
> > +        {
> > +            if ( !test_and_clear_bit(STATE_SCHED, &pirq_dpci->state) )
> > +                BUG();
> > +            hvm_dirq_assist(pirq_dpci);
> > +            put_domain(pirq_dpci->dom);
> > +            clear_bit(STATE_RUN, &pirq_dpci->state);
> > +            continue;
> > +        }
> > +
> > +        local_irq_disable();
> > +        list_add_tail(&pirq_dpci->softirq_list, &per_cpu(dpci_list, cpu));
> > +        local_irq_enable();
> 
> Can't all this be simplified quite a bit now that the code is no longer
> domain-centric? I can't really see who the above setting of
> STATE_RUN can race with for example. And should that no longer
> be needed it would seem that re-inserting the list entry wouldn't
> then be either.

It can race with 'dpci_kill.'

But I can simplify this by just using 'mapping' and having just one
state - set or unset. Thought that will require fiddling with
'hvm_dirq_assist'. I think to preserve git bisection no-break rules
that needs to be done as a seperate patch (after the #3 patch which
squashes __hvm_dirq_assist in hvm_dirq_assist).

> 
> > +
> > +        raise_softirq(HVM_DPCI_SOFTIRQ);
> > +    }
> > +}
> > +static int cpu_callback(
> 
> Blank line missing.

Aye!
> 
> > +static int __init setup_dpci_softirq(void)
> > +{
> > +    int cpu;
> 
> unsigned int

Shame on me. You mentioned that in the previous review and I lost that
change!
> 
> 
> > @@ -108,7 +109,7 @@ int pt_pirq_iterate(struct domain *d,
> >                      int (*cb)(struct domain *,
> >                                struct hvm_pirq_dpci *, void *arg),
> >                      void *arg);
> > -
> > +int dpci_kill(struct hvm_pirq_dpci *);
> >  /* Modify state of a PCI INTx wire. */
> >  void hvm_pci_intx_assert(
> >      struct domain *d, unsigned int device, unsigned int intx);
> 
> Please don't drop useful blank lines like this.

Yes. That was an oversight.
Will of course fix it.
> 
> Jan

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