RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[KY Srinivasan]

 -Original Message-
 From: Thomas Gleixner [mailto:t...@linutronix.de]
 Sent: Saturday, February 19, 2011 5:23 AM
 To: KY Srinivasan
 Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
 de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; 
 Hank
 Janssen
 Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
 
 On Tue, 15 Feb 2011, K. Y. Srinivasan wrote:
  -/* FIXME! We need to do this dynamically for PIC and APIC system */
  -#define VMBUS_IRQ  0x5
  -#define VMBUS_IRQ_VECTOR   IRQ5_VECTOR
  +static int vmbus_irq;
 
   /* Main vmbus driver data structure */
   struct vmbus_driver_context {
  @@ -57,6 +55,27 @@ struct vmbus_driver_context {
  struct vm_device device_ctx;
   };
 
  +/*
  + * Find an un-used IRQ that the VMBUS can use. If none is available;
  + * return -EBUSY.
  + */
  +static int vmbus_get_irq(void)
  +{
  +   unsigned int avail_irq_mask;
  +   int irq = -EBUSY;
  +
  +   /*
  +* Pick the first unused interrupt. HyperV can
  +* interrupt us on any interrupt line we specify.
  +*/
  +
  +   avail_irq_mask = probe_irq_on();
  +   if (avail_irq_mask != 0)
  +   irq = ffs(avail_irq_mask);
  +   probe_irq_off(avail_irq_mask);
  +   return irq;
 
 
 Please do not use probe_irq_on for dynamic irq allocation. Highjacking
 the lower PIC irqs is really not a good idea. Depending on when this
 runs, you might grab an irq required by a driver which gets loaded
 later.
 
 Could you please explain what you're trying to do here ?

The IRQ being allocated is for the VMBUS driver for Linux guests running on
a Windows virtualization platform (Hyper-V hypervisor).
The hypervisor is capable of notifying events on the VMBUS via
a guest specified interrupt line. Prior to this patch,
the code was statically selecting an interrupt line for
use by VMBUS. One of the long standing review comments
on  that code was to make this irq allocation dynamic and that
is what this patch does. For the Linux guest running as a VM
on Hyper-V, the concern you raise is not an issue.

Regards,

K. Y

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[KY Srinivasan]

 -Original Message-
 From: Thomas Gleixner [mailto:t...@linutronix.de]
 Sent: Saturday, February 19, 2011 10:12 AM
 To: KY Srinivasan
 Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
 de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; 
 Hank
 Janssen
 Subject: RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
 
 On Sat, 19 Feb 2011, KY Srinivasan wrote:
   From: Thomas Gleixner [mailto:t...@linutronix.de]
   Please do not use probe_irq_on for dynamic irq allocation. Highjacking
   the lower PIC irqs is really not a good idea. Depending on when this
   runs, you might grab an irq required by a driver which gets loaded
   later.
  
   Could you please explain what you're trying to do here ?
 
  The IRQ being allocated is for the VMBUS driver for Linux guests running on
  a Windows virtualization platform (Hyper-V hypervisor).
  The hypervisor is capable of notifying events on the VMBUS via
  a guest specified interrupt line. Prior to this patch,
  the code was statically selecting an interrupt line for
  use by VMBUS. One of the long standing review comments
  on  that code was to make this irq allocation dynamic and that
  is what this patch does. For the Linux guest running as a VM
  on Hyper-V, the concern you raise is not an issue.
 
 That patch does a whole lot of useless crap.
 
 When grabbing some random irq from the PIC is not an issue, then
 what's the point of this probing, retry loop and the comments about
 racing ? What races here? That does not make sense at all.

Like most virtualization platforms, Hyper-V also emulates the full PC 
platform. So, it is possible that the driver of some other emulated
devices might register for the IRQ line we might have selected. That 
is the race this code addresses. For performance reasons, we want
both storage and network traffic to go over the PV drivers. 

 
 I don't know why the previous reviewer wanted to have that
 dynamic. That just does not make sense to me.

Prior to this patch, we had a hard coded interrupt line for use by
this driver. If that line was already in use, the load of this driver 
would fail. This would be a fatal issue especially for distributions
that have embedded these PV drivers as part of their installation
media. This patch deals with such collisions in a more graceful way -
we would not bail until we have scanned all low interrupt lines.
 
 
 Btw, that whole interrupt handler with two tasklets, one of them
 scheduling work is just screaming threaded interrupt handler.

We are in the process of cleaning up these drivers; I am looking at
some of these and other issues.

Regards,

K. Y  

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization