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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.09.14 at 16:31, <konrad.wilk@xxxxxxxxxx> wrote:
> On Mon, Sep 22, 2014 at 12:58:57PM +0100, Jan Beulich wrote:
>> >>> On 19.09.14 at 20:51, <konrad.wilk@xxxxxxxxxx> wrote:
>> > +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?

Yeah, if you could make the patch not more difficult to understand
than it needs to be by leaving out pieces helpful only to an abstract
future usage model (hinting at eventual needs in comments is
certainly fine).

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

I don't really mind either way - all that confuses me is that the way
it's done now makes it at least look like the function can get called
twice in a row, which surely isn't what we want/need.

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

Sounds like a reasonable alternative.

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

I was actually more thinking that perhaps patch 3 could be moved
first (i.e. together with the elimination of the domain centric
implementation, but without the tasklet->softirq conversion).

Jan

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