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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.10.14 at 22:11, <konrad.wilk@xxxxxxxxxx> wrote:
> Or do you want me to remove the 'goto restart' loop as it
> is unlikely to ever be triggered because the softirq would be
> executed right away?

No, that one definitely needs to stay.

> +static void pt_pirq_softirq_reset(struct hvm_pirq_dpci *pirq_dpci)
> +{
> +    struct domain *d = pirq_dpci->dom;
> +
> +    ASSERT(spin_is_locked(&d->event_lock));
> +
> +    switch ( cmpxchg(&pirq_dpci->state, 1 << STATE_SCHED, 0) )
> +    {
> +            /*
> +             * We are going to try to de-schedule the softirq before it goes 
> in
> +             * STATE_RUN. Whoever clears STATE_SCHED MUST refcount the 'dom'.
> +             */
> +        case (1 << STATE_SCHED):
> +            put_domain(d);
> +            /* fallthrough. */
> +            /*
> +             * The reason it is OK to reset 'dom' when STATE_RUN bit is set 
> is
> +             * due to a shortcut the 'dpci_softirq' implements. It stashes 
> the
> +             * a 'dom' in local variable before it sets STATE_RUN - and
> +             * therefore will not dereference '->dom' which would crash.
> +             */
> +        case (1 << STATE_RUN):
> +        case (1 << STATE_RUN) | (1 << STATE_SCHED):
> +            pirq_dpci->dom = NULL;
> +            break;
> +        default:
> +            break;

Indentation is still wrong. Also I think the comments are a little
odd in their placement. In particular, a fall-through comment should
imo immediately precede the subsequent case label(s). Hence I think
the larger comment would better go after the two labels involving
STATE_RUN.

> @@ -165,8 +287,14 @@ int pt_irq_create_bind(
>              {
>                  pirq_dpci->gmsi.gflags = 0;
>                  pirq_dpci->gmsi.gvec = 0;
> -                pirq_dpci->dom = NULL;
>                  pirq_dpci->flags = 0;
> +                /*
> +                 * Between the 'pirq_guest_bind' and before 
> 'pirq_guest_unbind'
> +                 * an interrupt can be scheduled. No more of them are going 
> to
> +                 * be scheduled but we must deal with the one that is in the
> +                 * queue.
> +                 */
> +                pt_pirq_softirq_reset(pirq_dpci);
>                  pirq_cleanup_check(info, d);
>                  spin_unlock(&d->event_lock);
>                  return rc;

I wonder whether that wouldn't better be moved into the conditional
where pirq_guest_unbind() gets called, while - along the lines of the
subsequent change - handling failure of pirq_guest_bind() the more
straightforward way.

> +static int cpu_callback(
> +    struct notifier_block *nfb, unsigned long action, void *hcpu)
> +{
> +    unsigned int cpu = (unsigned long)hcpu;
> +
> +    switch ( action )
> +    {
> +    case CPU_UP_PREPARE:
> +        INIT_LIST_HEAD(&per_cpu(dpci_list, cpu));
> +        break;
> +    case CPU_UP_CANCELED:
> +    case CPU_DEAD:
> +        /*
> +         * On CPU_DYING this callback is called (on the CPU that is dying)
> +         * with an possible HVM_DPIC_SOFTIRQ pending - at which point we can
> +         * clear out any outstanding domains (by the virtue of the idle loop
> +         * calling the softirq later). In CPU_DEAD case the CPU is deaf and
> +         * there are no pending softirqs for us to handle so we can chill.
> +         */
> +        ASSERT(list_empty(&per_cpu(dpci_list, cpu)));
> +        break;
> +    default:
> +        break;

Could I additionally talk you into omitting unnecessary default cases
like this one?

> -void pci_release_devices(struct domain *d)
> +int pci_release_devices(struct domain *d)
>  {
>      struct pci_dev *pdev;
>      u8 bus, devfn;
> +    int ret;
>  
>      spin_lock(&pcidevs_lock);
> -    pci_clean_dpci_irqs(d);
> +    ret = pci_clean_dpci_irqs(d);
> +    if ( ret == -ERESTART )
> +    {
> +        spin_unlock(&pcidevs_lock);
> +        return ret;
> +    }
>      while ( (pdev = pci_get_pdev_by_domain(d, -1, -1, -1)) )
>      {
>          bus = pdev->bus;

Even if errors other than -ERESTART aren't possible right now, the
code now looks like it is ignoring such. I think it would be better if
you simply dropped the special casing of -ERESTART and propagated
all errors here.

Jan

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