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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

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

Jan


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