[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 for-xen-4.5 1/3] dpci: Replace tasklet with an softirq (v5)
>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |