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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> +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?

> +
> +    /* 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.

> @@ -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.

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

> @@ -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.

> +
> +        raise_softirq(HVM_DPCI_SOFTIRQ);
> +    }
> +}
> +static int cpu_callback(

Blank line missing.

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

unsigned int


> @@ -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.

Jan

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