Re: [Xen-devel] [PATCHv1] xen/events/fifo: Handle linked events when closing a PIRQ port

[linux@xxxxxxxxxxxxxx]

On 2015-08-10 16:24, David Vrabel wrote:
> Commit fcdf31a7c162de0c93a2bee51df4688ab0a348f8 (xen/events/fifo:
> Handle linked events when closing a port) did not handle closing a
> port bound to a PIRQ because these are closed from shutdown_pirq()
> which is called with interrupts disabled.
>
> Defer the close to a work queue where we can safely spin waiting for
> the LINKED bit to clear.  For simplicity, the close is always deferred
> even if it is not required (i.e., we're already in process context).
>
> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> Cc: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> ---
> Cc: Sander Eikelenboom <linux@xxxxxxxxxxxxxx>
Hi David,

Tested your patch, don't know for sure but this doesn't seem to work 
out.
I end up with this event channel error on dom0 boot.

Which ends in state:
Name                                        ID   Mem VCPUs      State   Time(s)
(null)                                       0  1536     6     r-----    
 183.8
--
Sander

(XEN) [2015-08-10 16:35:34.584] PCI add device 0000:0d:00.0
(XEN) [2015-08-10 16:35:34.891] PCI add device 0000:0c:00.0
(XEN) [2015-08-10 16:35:35.123] PCI add device 0000:0b:00.0
(XEN) [2015-08-10 16:35:35.325] PCI add device 0000:0a:00.0
(XEN) [2015-08-10 16:35:35.574] PCI add device 0000:09:00.0
(XEN) [2015-08-10 16:35:35.642] PCI add device 0000:09:00.1
(XEN) [2015-08-10 16:35:35.872] PCI add device 0000:05:00.0
(XEN) [2015-08-10 16:35:36.044] PCI add device 0000:06:01.0
(XEN) [2015-08-10 16:35:36.109] PCI add device 0000:06:02.0
(XEN) [2015-08-10 16:35:36.293] PCI add device 0000:08:00.0
(XEN) [2015-08-10 16:35:36.603] PCI add device 0000:07:00.0
(XEN) [2015-08-10 16:35:36.906] PCI add device 0000:04:00.0
(XEN) [2015-08-10 16:35:37.074] PCI add device 0000:03:06.0
(XEN) [2015-08-10 16:35:39.456] PCI: Using MCFG for segment 0000 bus 
00-ff
(XEN) [2015-08-10 16:35:49.623] d0: Forcing read-only access to MFN 
fed00
(XEN) [2015-08-10 16:35:51.374] event_channel.c:472:d0v0 EVTCHNOP 
failure: error -17


> ---
>  drivers/xen/events/events_2l.c       | 10 +++++++
>  drivers/xen/events/events_base.c     | 13 +--------
>  drivers/xen/events/events_fifo.c     | 52 
> +++++++++++++++++++++++++++---------
>  drivers/xen/events/events_internal.h |  5 ++--
>  4 files changed, 53 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/xen/events/events_2l.c 
> b/drivers/xen/events/events_2l.c
> index 7dd4631..82c90de 100644
> --- a/drivers/xen/events/events_2l.c
> +++ b/drivers/xen/events/events_2l.c
> @@ -354,6 +354,15 @@ static void evtchn_2l_resume(void)
>                                 EVTCHN_2L_NR_CHANNELS/BITS_PER_EVTCHN_WORD);
>  }
>
> +static void evtchn_2l_close(unsigned int port, unsigned int cpu)
> +{
> +       struct evtchn_close close;
> +
> +       close.port = port;
> +       if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)
> +               BUG();
> +}
> +
>  static const struct evtchn_ops evtchn_ops_2l = {
>         .max_channels      = evtchn_2l_max_channels,
>         .nr_channels       = evtchn_2l_max_channels,
> @@ -366,6 +375,7 @@ static const struct evtchn_ops evtchn_ops_2l = {
>         .unmask            = evtchn_2l_unmask,
>         .handle_events     = evtchn_2l_handle_events,
>         .resume            = evtchn_2l_resume,
> +       .close             = evtchn_2l_close,
>  };
>
>  void __init xen_evtchn_2l_init(void)
> diff --git a/drivers/xen/events/events_base.c 
> b/drivers/xen/events/events_base.c
> index 1495ecc..e3f0049 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -452,17 +452,6 @@ static void xen_free_irq(unsigned irq)
>         irq_free_desc(irq);
>  }
>
> -static void xen_evtchn_close(unsigned int port, unsigned int cpu)
> -{
> -       struct evtchn_close close;
> -
> -       xen_evtchn_op_close(port, cpu);
> -
> -       close.port = port;
> -       if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)
> -               BUG();
> -}
> -
>  static void pirq_query_unmask(int irq)
>  {
>         struct physdev_irq_status_query irq_status;
> @@ -546,7 +535,7 @@ out:
>
>  err:
>         pr_err("irq%d: Failed to set port to irq mapping (%d)\n", irq, rc);
> -       xen_evtchn_close(evtchn, NR_CPUS);
> +       xen_evtchn_close(evtchn, 0);
>         return 0;
>  }
>
> diff --git a/drivers/xen/events/events_fifo.c 
> b/drivers/xen/events/events_fifo.c
> index 6df8aac..149e1e9 100644
> --- a/drivers/xen/events/events_fifo.c
> +++ b/drivers/xen/events/events_fifo.c
> @@ -40,6 +40,7 @@
>  #include <linux/smp.h>
>  #include <linux/percpu.h>
>  #include <linux/cpu.h>
> +#include <linux/slab.h>
>
>  #include <asm/sync_bitops.h>
>  #include <asm/xen/hypercall.h>
> @@ -385,24 +386,51 @@ static void evtchn_fifo_resume(void)
>         event_array_pages = 0;
>  }
>
> +struct close_work {
> +       struct work_struct work;
> +       unsigned int port;
> +};
> +
> +static void evtchn_fifo_close_work(struct work_struct *work)
> +{
> +       struct close_work *cw = container_of(work, struct close_work, work);
> +       struct evtchn_close close;
> +
> +       while (evtchn_fifo_is_linked(cw->port))
> +               cpu_relax();
> +
> +       close.port = cw->port;
> +       if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)
> +               BUG();
> +
> +       kfree(cw);
> +}
> +
>  static void evtchn_fifo_close(unsigned port, unsigned int cpu)
>  {
> -       if (cpu == NR_CPUS)
> -               return;
> +       struct close_work *cw;
>
> -       get_online_cpus();
> -       if (cpu_online(cpu)) {
> -               if (WARN_ON(irqs_disabled()))
> -                       goto out;
> +       /*
> +        * A port cannot be closed until the LINKED bit is clear.
> +        *
> +        * Reusing an already linked event may: a) cause the new event
> +        * to be raised on the wrong VCPU; or b) cause the event to be
> +        * lost (if the old VCPU is offline).
> +        *
> +        * If the VCPU is offline, its queues must be drained before
> +        * spinning for LINKED to be clear.
> +        */
>
> -               while (evtchn_fifo_is_linked(port))
> -                       cpu_relax();
> -       } else {
> +       if (!cpu_online(cpu))
>                 __evtchn_fifo_handle_events(cpu, true);
> -       }
>
> -out:
> -       put_online_cpus();
> +       cw = kzalloc(sizeof(*cw), GFP_ATOMIC);
> +       if (!cw)
> +               return;
> +       INIT_WORK(&cw->work, evtchn_fifo_close_work);
> +       cw->port = port;
> +
> +       schedule_work_on(cpu, &cw->work);
>  }
>
>  static const struct evtchn_ops evtchn_ops_fifo = {
> diff --git a/drivers/xen/events/events_internal.h
> b/drivers/xen/events/events_internal.h
> index d18e123..017cc22 100644
> --- a/drivers/xen/events/events_internal.h
> +++ b/drivers/xen/events/events_internal.h
> @@ -146,10 +146,9 @@ static inline void xen_evtchn_resume(void)
>                 evtchn_ops->resume();
>  }
>
> -static inline void xen_evtchn_op_close(unsigned port, unsigned cpu)
> +static inline void xen_evtchn_close(unsigned port, unsigned cpu)
>  {
> -       if (evtchn_ops->close)
> -               return evtchn_ops->close(port, cpu);
> +       evtchn_ops->close(port, cpu);
>  }
>
>  void xen_evtchn_2l_init(void);
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel