xen-evtchn: Bind dyn evtchn:qemu-dm interrupt to next online CPU

xen-evtchn: Bind dyn evtchn:qemu-dm interrupt to next online CPU

[Anoob Soman]

Hi,

In our Xenserver testing, I have seen cases when we boot 50 windows VMs 
together, dom0 kernel softlocks up.

The following is a brief description of the problem of what caused 
soflockup detection code to kick. A HVM domain boot generates around 
200K (evtchn:qemu-dm xen-dyn) interrupts, in a short period of time. All 
these evtchn:qemu-dm are bound to VCPU 0, until irqbalance sees these 
IRQ and moves it to a different VCPU. In case of XenServer, irqbalance 
runs every 10 seconds, which means irqbalance doesn't get to see these 
burst of interrupts and doesn't re-balance interrupts most of the time, 
making all evtchn:qemu-dm to be processed by VCPU0. This cause VCPU0 to 
spend most of time processing hardirq and very little time on softirq. 
Moreover, in XenServer dom0 PREEMPTION is disabled, this means VCPU0 
never runs watchdog (process context), triggering a softlockup detection 
code to panic.

One way to solve this problem would be to bind evtchn:qemu-dm to next 
online VCPU, will spread hardirq processing evenly across different 
VCPU. Later, irqbalance will try to balance evtchn:qemu-dm, if required.

I am not sure if this is a sensible thing to do. I have a pseduo code 
(not the final patch) which tries to bind only 
IOCTL_EVTCHN_BIND_INTERDOMAIN (used by qemu-dm) to next.

  /* Rebind an evtchn so that it gets delivered to a specific cpu */
-static int rebind_irq_to_cpu(unsigned irq, unsigned tcpu)
+int xen_rebind_evtchn_to_cpu(int evtchn, unsigned tcpu)
  {
      struct evtchn_bind_vcpu bind_vcpu;
-    int evtchn = evtchn_from_irq(irq);
      int masked;

      if (!VALID_EVTCHN(evtchn))
@@ -1339,6 +1338,12 @@ static int rebind_irq_to_cpu(unsigned irq, 
unsigned tcpu)

      return 0;
  }
+EXPORT_SYMBOL_GPL(xen_rebind_evtchn_to_cpu);
+
+static int rebind_irq_to_cpu(unsigned irq, unsigned tcpu)
+{
+    return xen_rebind_evtchn_to_cpu(evtchn_from_irq(irq), tcpu);
+}

  static int set_affinity_irq(struct irq_data *data, const struct 
cpumask *dest,
                  bool force)
diff --git a/drivers/xen/evtchn.c b/drivers/xen/evtchn.c
index 7efd1cb..bd34d3d 100644
--- a/drivers/xen/evtchn.c
+++ b/drivers/xen/evtchn.c
@@ -471,6 +471,7 @@ static long evtchn_ioctl(struct file *file,
      case IOCTL_EVTCHN_BIND_INTERDOMAIN: {
          struct ioctl_evtchn_bind_interdomain bind;
          struct evtchn_bind_interdomain bind_interdomain;
+        static unsigned int selected_cpu;

          rc = -EFAULT;
          if (copy_from_user(&bind, uarg, sizeof(bind)))
@@ -489,8 +490,13 @@ static long evtchn_ioctl(struct file *file,
              break;

          rc = evtchn_bind_to_user(u, bind_interdomain.local_port);
-        if (rc == 0)
+        if (rc == 0) {
              rc = bind_interdomain.local_port;
+            selected_cpu = cpumask_next(selected_cpu, cpu_online_mask);
+            if (selected_cpu >= nr_cpu_ids)
+                selected_cpu = cpumask_first(cpu_online_mask);
+            xen_rebind_evtchn_to_cpu(rc, selected_cpu);
+        }
          break;
      }

diff --git a/include/xen/events.h b/include/xen/events.h
index 88da2ab..f442ca5 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -58,6 +58,7 @@ void evtchn_put(unsigned int evtchn);

  void xen_send_IPI_one(unsigned int cpu, enum ipi_vector vector);
  void rebind_evtchn_irq(int evtchn, int irq);
+int xen_rebind_evtchn_to_cpu(int evtchn, unsigned tcpu);

  static inline void notify_remote_via_evtchn(int port)
  {

"selected_cpu" needs to be protected, but I would like to avoid taking a 
lock. One way to avoid taking lock (before xen_rebind_evtchn_to_cpu()) 
would be to use "local_port%num_present_cpus()" or " smp_processor_id()" 
as index into cpumask_next.

What do you guys suggest.

Thanks,

Anoob.


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

Re: xen-evtchn: Bind dyn evtchn:qemu-dm interrupt to next online CPU

[Boris Ostrovsky]

On 05/16/2017 01:15 PM, Anoob Soman wrote:
> Hi,
>
> In our Xenserver testing, I have seen cases when we boot 50 windows
> VMs together, dom0 kernel softlocks up.
>
> The following is a brief description of the problem of what caused
> soflockup detection code to kick. A HVM domain boot generates around
> 200K (evtchn:qemu-dm xen-dyn) interrupts, in a short period of time.
> All these evtchn:qemu-dm are bound to VCPU 0, until irqbalance sees
> these IRQ and moves it to a different VCPU. In case of XenServer,
> irqbalance runs every 10 seconds, which means irqbalance doesn't get
> to see these burst of interrupts and doesn't re-balance interrupts
> most of the time, making all evtchn:qemu-dm to be processed by VCPU0.
> This cause VCPU0 to spend most of time processing hardirq and very
> little time on softirq. Moreover, in XenServer dom0 PREEMPTION is
> disabled, this means VCPU0 never runs watchdog (process context),
> triggering a softlockup detection code to panic.
>
> One way to solve this problem would be to bind evtchn:qemu-dm to next
> online VCPU, will spread hardirq processing evenly across different
> VCPU. Later, irqbalance will try to balance evtchn:qemu-dm, if required.
>
> I am not sure if this is a sensible thing to do. I have a pseduo code
> (not the final patch) which tries to bind only
> IOCTL_EVTCHN_BIND_INTERDOMAIN (used by qemu-dm) to next.


I think it's reasonable. One thing to watch for though is VCPU offlining
while binding.


>  /* Rebind an evtchn so that it gets delivered to a specific cpu */
> -static int rebind_irq_to_cpu(unsigned irq, unsigned tcpu)
> +int xen_rebind_evtchn_to_cpu(int evtchn, unsigned tcpu)
>  {
>      struct evtchn_bind_vcpu bind_vcpu;
> -    int evtchn = evtchn_from_irq(irq);
>      int masked;
>
>      if (!VALID_EVTCHN(evtchn))
> @@ -1339,6 +1338,12 @@ static int rebind_irq_to_cpu(unsigned irq,
> unsigned tcpu)
>
>      return 0;
>  }
> +EXPORT_SYMBOL_GPL(xen_rebind_evtchn_to_cpu);
> +
> +static int rebind_irq_to_cpu(unsigned irq, unsigned tcpu)
> +{
> +    return xen_rebind_evtchn_to_cpu(evtchn_from_irq(irq), tcpu);
> +}
>
>  static int set_affinity_irq(struct irq_data *data, const struct
> cpumask *dest,
>                  bool force)
> diff --git a/drivers/xen/evtchn.c b/drivers/xen/evtchn.c
> index 7efd1cb..bd34d3d 100644
> --- a/drivers/xen/evtchn.c
> +++ b/drivers/xen/evtchn.c
> @@ -471,6 +471,7 @@ static long evtchn_ioctl(struct file *file,
>      case IOCTL_EVTCHN_BIND_INTERDOMAIN: {
>          struct ioctl_evtchn_bind_interdomain bind;
>          struct evtchn_bind_interdomain bind_interdomain;
> +        static unsigned int selected_cpu;
>
>          rc = -EFAULT;
>          if (copy_from_user(&bind, uarg, sizeof(bind)))
> @@ -489,8 +490,13 @@ static long evtchn_ioctl(struct file *file,
>              break;
>
>          rc = evtchn_bind_to_user(u, bind_interdomain.local_port);
> -        if (rc == 0)
> +        if (rc == 0) {
>              rc = bind_interdomain.local_port;
> +            selected_cpu = cpumask_next(selected_cpu, cpu_online_mask);
> +            if (selected_cpu >= nr_cpu_ids)
> +                selected_cpu = cpumask_first(cpu_online_mask);
> +            xen_rebind_evtchn_to_cpu(rc, selected_cpu);

Can you do proper assignment *instead of* binding to CPU0 as opposed to
rebinding the event channel later? Otherwise you are making an extra
hypercall.

You also probably want to look at current IRQ affinity mask instead of
cpu_online_mask.

> +        }
>          break;
>      }
>
> diff --git a/include/xen/events.h b/include/xen/events.h
> index 88da2ab..f442ca5 100644
> --- a/include/xen/events.h
> +++ b/include/xen/events.h
> @@ -58,6 +58,7 @@ void evtchn_put(unsigned int evtchn);
>
>  void xen_send_IPI_one(unsigned int cpu, enum ipi_vector vector);
>  void rebind_evtchn_irq(int evtchn, int irq);
> +int xen_rebind_evtchn_to_cpu(int evtchn, unsigned tcpu);
>
>  static inline void notify_remote_via_evtchn(int port)
>  {
>
> "selected_cpu" needs to be protected, but I would like to avoid taking
> a lock. One way to avoid taking lock (before
> xen_rebind_evtchn_to_cpu()) would be to use
> "local_port%num_present_cpus()" or " smp_processor_id()" as index into
> cpumask_next.

The latter sounds better to me --- there is a chance that the interrupts
will be processed locally.

-boris

>
> What do you guys suggest.
>
> Thanks,
>
> Anoob.
>


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

Re: xen-evtchn: Bind dyn evtchn:qemu-dm interrupt to next online CPU

[Anoob Soman]

On 16/05/17 20:02, Boris Ostrovsky wrote:
> On 05/16/2017 01:15 PM, Anoob Soman wrote:
>> Hi,
>>
>> In our Xenserver testing, I have seen cases when we boot 50 windows
>> VMs together, dom0 kernel softlocks up.
>>
>> The following is a brief description of the problem of what caused
>> soflockup detection code to kick. A HVM domain boot generates around
>> 200K (evtchn:qemu-dm xen-dyn) interrupts, in a short period of time.
>> All these evtchn:qemu-dm are bound to VCPU 0, until irqbalance sees
>> these IRQ and moves it to a different VCPU. In case of XenServer,
>> irqbalance runs every 10 seconds, which means irqbalance doesn't get
>> to see these burst of interrupts and doesn't re-balance interrupts
>> most of the time, making all evtchn:qemu-dm to be processed by VCPU0.
>> This cause VCPU0 to spend most of time processing hardirq and very
>> little time on softirq. Moreover, in XenServer dom0 PREEMPTION is
>> disabled, this means VCPU0 never runs watchdog (process context),
>> triggering a softlockup detection code to panic.
>>
>> One way to solve this problem would be to bind evtchn:qemu-dm to next
>> online VCPU, will spread hardirq processing evenly across different
>> VCPU. Later, irqbalance will try to balance evtchn:qemu-dm, if required.
>>
>> I am not sure if this is a sensible thing to do. I have a pseduo code
>> (not the final patch) which tries to bind only
>> IOCTL_EVTCHN_BIND_INTERDOMAIN (used by qemu-dm) to next.
>
> I think it's reasonable. One thing to watch for though is VCPU offlining
> while binding.
>

Let me see if I can manage to do take care of this scenario.

>>   /* Rebind an evtchn so that it gets delivered to a specific cpu */
>> -static int rebind_irq_to_cpu(unsigned irq, unsigned tcpu)
>> +int xen_rebind_evtchn_to_cpu(int evtchn, unsigned tcpu)
>>   {
>>       struct evtchn_bind_vcpu bind_vcpu;
>> -    int evtchn = evtchn_from_irq(irq);
>>       int masked;
>>
>>       if (!VALID_EVTCHN(evtchn))
>> @@ -1339,6 +1338,12 @@ static int rebind_irq_to_cpu(unsigned irq,
>> unsigned tcpu)
>>
>>       return 0;
>>   }
>> +EXPORT_SYMBOL_GPL(xen_rebind_evtchn_to_cpu);
>> +
>> +static int rebind_irq_to_cpu(unsigned irq, unsigned tcpu)
>> +{
>> +    return xen_rebind_evtchn_to_cpu(evtchn_from_irq(irq), tcpu);
>> +}
>>
>>   static int set_affinity_irq(struct irq_data *data, const struct
>> cpumask *dest,
>>                   bool force)
>> diff --git a/drivers/xen/evtchn.c b/drivers/xen/evtchn.c
>> index 7efd1cb..bd34d3d 100644
>> --- a/drivers/xen/evtchn.c
>> +++ b/drivers/xen/evtchn.c
>> @@ -471,6 +471,7 @@ static long evtchn_ioctl(struct file *file,
>>       case IOCTL_EVTCHN_BIND_INTERDOMAIN: {
>>           struct ioctl_evtchn_bind_interdomain bind;
>>           struct evtchn_bind_interdomain bind_interdomain;
>> +        static unsigned int selected_cpu;
>>
>>           rc = -EFAULT;
>>           if (copy_from_user(&bind, uarg, sizeof(bind)))
>> @@ -489,8 +490,13 @@ static long evtchn_ioctl(struct file *file,
>>               break;
>>
>>           rc = evtchn_bind_to_user(u, bind_interdomain.local_port);
>> -        if (rc == 0)
>> +        if (rc == 0) {
>>               rc = bind_interdomain.local_port;
>> +            selected_cpu = cpumask_next(selected_cpu, cpu_online_mask);
>> +            if (selected_cpu >= nr_cpu_ids)
>> +                selected_cpu = cpumask_first(cpu_online_mask);
>> +            xen_rebind_evtchn_to_cpu(rc, selected_cpu);
> Can you do proper assignment *instead of* binding to CPU0 as opposed to
> rebinding the event channel later? Otherwise you are making an extra
> hypercall.

Sure, make sense.

> You also probably want to look at current IRQ affinity mask instead of
> cpu_online_mask.

Sure, I will send a patch out with the changes, based on your comments.

>> +        }
>>           break;
>>       }
>>
>> diff --git a/include/xen/events.h b/include/xen/events.h
>> index 88da2ab..f442ca5 100644
>> --- a/include/xen/events.h
>> +++ b/include/xen/events.h
>> @@ -58,6 +58,7 @@ void evtchn_put(unsigned int evtchn);
>>
>>   void xen_send_IPI_one(unsigned int cpu, enum ipi_vector vector);
>>   void rebind_evtchn_irq(int evtchn, int irq);
>> +int xen_rebind_evtchn_to_cpu(int evtchn, unsigned tcpu);
>>
>>   static inline void notify_remote_via_evtchn(int port)
>>   {
>>
>> "selected_cpu" needs to be protected, but I would like to avoid taking
>> a lock. One way to avoid taking lock (before
>> xen_rebind_evtchn_to_cpu()) would be to use
>> "local_port%num_present_cpus()" or " smp_processor_id()" as index into
>> cpumask_next.
> The latter sounds better to me --- there is a chance that the interrupts
> will be processed locally.
>
> -boris

Did you mean using smp_processor_id() as index ?

>> What do you guys suggest.
>>
>> Thanks,
>>
>> Anoob.
>>


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

Re: xen-evtchn: Bind dyn evtchn:qemu-dm interrupt to next online CPU

[Boris Ostrovsky]

>>> "selected_cpu" needs to be protected, but I would like to avoid taking
>>> a lock. One way to avoid taking lock (before
>>> xen_rebind_evtchn_to_cpu()) would be to use
>>> "local_port%num_present_cpus()" or " smp_processor_id()" as index into
>>> cpumask_next.
>> The latter sounds better to me --- there is a chance that the interrupts
>> will be processed locally.
>>
>> -boris
>
> Did you mean using smp_processor_id() as index ?

I meant binding to smp_processor_id().

-boris


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

Re: xen-evtchn: Bind dyn evtchn:qemu-dm interrupt to next online CPU

[Anoob Soman]

On 16/05/17 20:02, Boris Ostrovsky wrote:

Hi Boris,

Sorry for the delay, I was out traveling.

>>           rc = evtchn_bind_to_user(u, bind_interdomain.local_port);
>> -        if (rc == 0)
>> +        if (rc == 0) {
>>               rc = bind_interdomain.local_port;
>> +            selected_cpu = cpumask_next(selected_cpu, cpu_online_mask);
>> +            if (selected_cpu >= nr_cpu_ids)
>> +                selected_cpu = cpumask_first(cpu_online_mask);
>> +            xen_rebind_evtchn_to_cpu(rc, selected_cpu);
> Can you do proper assignment *instead of* binding to CPU0 as opposed to
> rebinding the event channel later? Otherwise you are making an extra
> hypercall.

If I understood the code correctly, EVTCHNOP_bind_interdomain doesn't 
support sending in VCPU number, so I think we would require two 
hypercalls one for binding interdomain eventchn 
(EVTCHNOP_bind_interdomain) and another for binding it to a VCPU 
(EVTCHNOP_bind_vcpu). We can create EVTCHNOP_bind_interdomain_V2 sub-op, 
which can take in VCPU id, if we want to avoid making multiple hypercalls.

> You also probably want to look at current IRQ affinity mask instead of
> cpu_online_mask.
>

Do we need to look at IRQ affinity mask, if we are going to bind 
eventchannel to smp_processor_id(). If we definitely need to use IRQ 
affinity then binding to smp_processor_id() might not be the correct 
approach.

-Anoob



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

Re: xen-evtchn: Bind dyn evtchn:qemu-dm interrupt to next online CPU

[Boris Ostrovsky]

On 05/30/2017 11:17 AM, Anoob Soman wrote:
> On 16/05/17 20:02, Boris Ostrovsky wrote:
>
> Hi Boris,
>
> Sorry for the delay, I was out traveling.
>
>>>           rc = evtchn_bind_to_user(u, bind_interdomain.local_port);
>>> -        if (rc == 0)
>>> +        if (rc == 0) {
>>>               rc = bind_interdomain.local_port;
>>> +            selected_cpu = cpumask_next(selected_cpu,
>>> cpu_online_mask);
>>> +            if (selected_cpu >= nr_cpu_ids)
>>> +                selected_cpu = cpumask_first(cpu_online_mask);
>>> +            xen_rebind_evtchn_to_cpu(rc, selected_cpu);
>> Can you do proper assignment *instead of* binding to CPU0 as opposed to
>> rebinding the event channel later? Otherwise you are making an extra
>> hypercall.
>
> If I understood the code correctly, EVTCHNOP_bind_interdomain doesn't
> support sending in VCPU number, so I think we would require two
> hypercalls one for binding interdomain eventchn
> (EVTCHNOP_bind_interdomain) and another for binding it to a VCPU
> (EVTCHNOP_bind_vcpu). We can create EVTCHNOP_bind_interdomain_V2
> sub-op, which can take in VCPU id, if we want to avoid making multiple
> hypercalls.

This is not worth API change so I guess we are going to have to use
separate calls, as you originally proposed.


>
>> You also probably want to look at current IRQ affinity mask instead of
>> cpu_online_mask.
>>
>
> Do we need to look at IRQ affinity mask, if we are going to bind
> eventchannel to smp_processor_id(). If we definitely need to use IRQ
> affinity then binding to smp_processor_id() might not be the correct
> approach.

What if, for whatever reason, the current processor is not in the
affinity mask of the IRQ?

-boris


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

Re: xen-evtchn: Bind dyn evtchn:qemu-dm interrupt to next online CPU

[Anoob Soman]

On 30/05/17 18:42, Boris Ostrovsky wrote:
> This is not worth API change so I guess we are going to have to use
> separate calls, as you originally proposed.

Sure. I will stick to making two hypercalls.

>> Do we need to look at IRQ affinity mask, if we are going to bind
>> eventchannel to smp_processor_id(). If we definitely need to use IRQ
>> affinity then binding to smp_processor_id() might not be the correct
>> approach.
> What if, for whatever reason, the current processor is not in the
> affinity mask of the IRQ?

Yes, there is a possibility of that happening, but chances are really 
slim. I will drop the idea of binding evtchn to smp_processor_id and 
instead change the logic to use a per_cpu(int) to hold last used cpu and 
then use cpu_next_and(last_cpu, affinity, cpu_online_mask) to get next cpu.

-Anoob



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