: Staging: hv: Allocate the vmbus irq dynamically

PATCH In review K. Y. Srinivasan 4 participants 22 messages Feb 15, 2011 – Feb 23, 2011

Subsystems (2)

Staging virtio core

lore

Share & tools

lore

K. Y. Srinivasan

author

2011-02-15 19:47 UTC #0 lore raw

PATCH 1/1 : Staging: hv: Allocate the vmbus irq dynamically

Signed-off-by K. Y. Srinivasan <[email protected]>

Signed-off-by Haiyang Zhang <[email protected]>

Signed-off-by Hank Janssen <[email protected]>

---
 drivers/staging/hv/vmbus_drv.c |   72 +++++++++++++++++++++++++++++++---------
 1 files changed, 56 insertions(+), 16 deletions(-)

drivers/staging/hv/vmbus_drv.c +56 ~~-16~~

index 459c707..441ce85 100644
@@ -36,9 +36,7 @@
 #include "vmbus_private.h"
 
 
-/* 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;
+}
+
 static int vmbus_match(struct device *device, struct device_driver *driver);
 static int vmbus_probe(struct device *device);
 static int vmbus_remove(struct device *device);

@@ -80,7 +99,6 @@ EXPORT_SYMBOL(vmbus_loglevel);
 	/* (ALL_MODULES << 16 | DEBUG_LVL_ENTEREXIT); */
 	/* (((VMBUS | VMBUS_DRV)<<16) | DEBUG_LVL_ENTEREXIT); */
 
-static int vmbus_irq = VMBUS_IRQ;
 
 /* Set up per device attributes in /sys/bus/vmbus/devices/<bus device> */
 static struct device_attribute vmbus_device_attrs[] = {

@@ -467,6 +485,7 @@ static int vmbus_bus_init(void)
 	struct vm_device *dev_ctx = &vmbus_drv.device_ctx;
 	int ret;
 	unsigned int vector;
+	bool irq_assigned = false;
 
 	DPRINT_INFO(VMBUS, "+++++++ HV Driver version = %s +++++++",
 		    HV_DRV_VERSION);

@@ -517,20 +536,42 @@ static int vmbus_bus_init(void)
 		goto cleanup;
 	}
 
-	/* Get the interrupt resource */
-	ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
-			  driver->name, NULL);
-
-	if (ret != 0) {
-		DPRINT_ERR(VMBUS_DRV, "ERROR - Unable to request IRQ %d",
-			   vmbus_irq);
+	/*
+	 * Get an unused interrupt line and register our handler.
+	 * Since there is a window between getting an unused
+	 * interrupt line and registering our handler, we close
+	 * this window by repeating the sequence if we raced.
+	 * This loop will terminate under the following conditions:
+	 *
+	 * 1) If we succeed in registering our handler OR
+	 * 2) If there are no free interrupt lines for our use OR
+	 * 3) request_irq fails for reasons other than an irq collision
+	 */
+	while (!irq_assigned) {
+		vmbus_irq = vmbus_get_irq();
 
-		bus_unregister(&vmbus_drv_ctx->bus);
+		if (vmbus_irq < 0) {
+			bus_unregister(&vmbus_drv_ctx->bus);
+			ret = -EBUSY;
+			goto cleanup;
+		}
 
-		ret = -1;
-		goto cleanup;
+		ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
+			  driver->name, NULL);
+		switch (ret) {
+		case -EBUSY:
+			 /* We raced and lost; try again.*/
+			continue;
+		case 0:
+			irq_assigned = true;
+			break;
+		default:
+			/* Failed to request_irq; cleanup */
+			goto cleanup;
+		}
 	}
-	vector = VMBUS_IRQ_VECTOR;
+
+	vector = IRQ0_VECTOR + vmbus_irq;
 
 	DPRINT_INFO(VMBUS_DRV, "irq 0x%x vector 0x%x", vmbus_irq, vector);

@@ -1117,7 +1158,6 @@ MODULE_DEVICE_TABLE(pci, microsoft_hv_pci_table);
 
 MODULE_LICENSE("GPL");
 MODULE_VERSION(HV_DRV_VERSION);
-module_param(vmbus_irq, int, S_IRUGO);
 module_param(vmbus_loglevel, int, S_IRUGO);
 
 module_init(vmbus_init);

Signature · 1 line

1.5.5.6

Greg KH

2011-02-18 21:14 UTC #1 lore raw

Quoted 4 lines · K. Y. Srinivasan

Signed-off-by K. Y. Srinivasan <[email protected]>
Signed-off-by Haiyang Zhang <[email protected]>
Signed-off-by Hank Janssen <[email protected]>

You didn't run this through checkpatch.pl.

Please do so and fix the warning it gives you.

thanks,

greg k-h

KY Srinivasan

author

2011-02-18 22:00 UTC #2 lore raw

Quoted 12 lines

-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, February 18, 2011 4:14 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 1 line · K. Y. Srinivasan

Quoted 7 lines

Quoted 3 lines
Signed-off-by K. Y. Srinivasan <[email protected]>
Signed-off-by Haiyang Zhang <[email protected]>
Signed-off-by Hank Janssen <[email protected]>
You didn't run this through checkpatch.pl.
Please do so and fix the warning it gives you.

Greg, I did run the checkpatch script against this patch and the only complaint I got was with regards to the IRQF_SAMPLE_RANDOM flag that I pass. As a virtual machine, this is the only external event that the VM is going to see and so I chose to keep this flag. Is there something that would replace this flag; looking at the Xen drivers they do pass this flag.

Regards,

K. Y

Quoted 4 lines

thanks,
greg k-h

Greg KH

2011-02-18 22:07 UTC #3 lore raw

Quoted 18 lines · KY Srinivasan

Quoted 18 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, February 18, 2011 4:14 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 4 lines · K. Y. Srinivasan
Signed-off-by K. Y. Srinivasan <[email protected]>
Signed-off-by Haiyang Zhang <[email protected]>
Signed-off-by Hank Janssen <[email protected]>
You didn't run this through checkpatch.pl.

Quoted 7 lines

Quoted 1 line
Please do so and fix the warning it gives you.
Greg, I did run the checkpatch script against this patch and the only complaint I got was with regards to the IRQF_SAMPLE_RANDOM flag that I pass. As a virtual machine, this is the only external event that the VM is going to see and so I chose to keep this flag. Is there something that would replace this flag; looking at the Xen drivers they do pass this flag.

But that flag is going away, right? And this really can't be a valid source of entropy as the HV channel is pretty predictable.

If you are only using this because Xen does/did it, that's not a valid excuse :)

thanks,

greg k-h

KY Srinivasan

author

2011-02-18 22:16 UTC #4 lore raw

Quoted 37 lines

-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, February 18, 2011 5:07 PM
To: KY Srinivasan
Cc: Greg KH; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 18 lines · KY Srinivasan
Quoted 18 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, February 18, 2011 4:14 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 4 lines · K. Y. Srinivasan
Signed-off-by K. Y. Srinivasan <[email protected]>
Signed-off-by Haiyang Zhang <[email protected]>
Signed-off-by Hank Janssen <[email protected]>
You didn't run this through checkpatch.pl.
Quoted 7 lines
Quoted 1 line
Please do so and fix the warning it gives you.
Greg, I did run the checkpatch script against this patch and the only complaint I got was with regards to the IRQF_SAMPLE_RANDOM flag that I pass. As a virtual machine, this is the only external event that the VM is going to see and so I chose to keep this flag. Is there something that would replace this flag; looking at the Xen drivers they do pass this flag.

Quoted 2 lines

But that flag is going away, right? And this really can't be a valid source of entropy as the HV channel is pretty predictable.

Is it going away? What would replace this. Is all interrupt sources considered predictable? This is the only unpredictable thing happening in the VM and that is the reason I chose to keep the flag.

Quoted 3 lines

If you are only using this because Xen does/did it, that's not a valid excuse :)

Surely, you are joking. In any event I am sending you a new patch with that flag removed.

Regards,

K. Y

Quoted 4 lines

thanks,
greg k-h

Greg KH

2011-02-18 22:29 UTC #5 lore raw

Quoted 37 lines · KY Srinivasan

Quoted 37 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, February 18, 2011 5:07 PM
To: KY Srinivasan
Cc: Greg KH; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 25 lines · KY Srinivasan
Quoted 12 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, February 18, 2011 4:14 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 1 line · K. Y. Srinivasan
Quoted 7 lines
Quoted 3 lines
Signed-off-by K. Y. Srinivasan <[email protected]>
Signed-off-by Haiyang Zhang <[email protected]>
Signed-off-by Hank Janssen <[email protected]>
You didn't run this through checkpatch.pl.
Please do so and fix the warning it gives you.
Greg, I did run the checkpatch script against this patch and the only complaint I got was with regards to the IRQF_SAMPLE_RANDOM flag that I pass. As a virtual machine, this is the only external event that the VM is going to see and so I chose to keep this flag. Is there something that would replace this flag; looking at the Xen drivers they do pass this flag.

Quoted 5 lines

Quoted 2 lines
But that flag is going away, right? And this really can't be a valid source of entropy as the HV channel is pretty predictable.
Is it going away? What would replace this. Is all interrupt sources considered predictable?

Did you read the file that the checkpatch script told you to about this entry?

Quoted 2 lines

This is the only unpredictable thing happening in the VM and that is the reason I chose to keep the flag.

If you remove it, do we loose all entropy for the VM?

Quoted 3 lines

Quoted 2 lines
If you are only using this because Xen does/did it, that's not a valid excuse :)
Surely, you are joking.

Not at all.

Quoted 1 line

In any event I am sending you a new patch with that flag removed.

Have you tested to see if you now loose all entropy, and it causes problems or not?

thanks,

greg k-h

KY Srinivasan

author

2011-02-19 00:56 UTC #6 lore raw

Quoted 48 lines

-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, February 18, 2011 5:29 PM
To: KY Srinivasan
Cc: Greg KH; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 37 lines · KY Srinivasan
Quoted 37 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, February 18, 2011 5:07 PM
To: KY Srinivasan
Cc: Greg KH; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 25 lines · KY Srinivasan
Quoted 12 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, February 18, 2011 4:14 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 1 line · K. Y. Srinivasan
Quoted 7 lines
Quoted 3 lines
Signed-off-by K. Y. Srinivasan <[email protected]>
Signed-off-by Haiyang Zhang <[email protected]>
Signed-off-by Hank Janssen <[email protected]>
You didn't run this through checkpatch.pl.
Please do so and fix the warning it gives you.
Greg, I did run the checkpatch script against this patch and the only complaint I got was with regards to the IRQF_SAMPLE_RANDOM flag that I pass. As a virtual machine, this is the only external event that the VM is going to see and so I chose to keep this flag. Is there something that would replace this flag; looking at the Xen drivers they do pass this flag.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
But that flag is going away, right? And this really can't be a valid source of entropy as the HV channel is pretty predictable.
Is it going away? What would replace this. Is all interrupt sources considered predictable?
Did you read the file that the checkpatch script told you to about this entry?

It is only after reading the document, I decided to keep that flag. Please note this is not a question of some interrupt sources not being
a good source of entropy; for this VM this is the only source of interrupts. The document on this flag talked about how people were incorrectly marking their interrupt as an entropy source; in this case there is not much of a choice.

Quoted 11 lines

Quoted 3 lines
This is the only unpredictable thing happening in the VM and that is the reason I chose to keep the flag.
If you remove it, do we loose all entropy for the VM?
Quoted 3 lines
Quoted 2 lines
If you are only using this because Xen does/did it, that's not a valid excuse :)
Surely, you are joking.
Not at all.

To set the record straight here, this flag is in the existing code. After I ran checkpatch, I toyed with the idea of getting rid of this. Then I decided to keep it for all the reasons I mentioned earlier.

Quoted 5 lines

Quoted 2 lines
In any event I am sending you a new patch with that flag removed.
Have you tested to see if you now loose all entropy, and it causes problems or not?

I am glad you asked me to test it. When I remove this flag, the entropy goes down significantly and this is not surprising. Looking at /proc/sys/kernel/entropy/entropy_avail, with the original patch after a couple of compiles the number would be in thousands. With that flag removed, I have the VM up for about an hour, even after a couple of compiles, the entropy number is yet to crack 200.

Let me know how you want to proceed here.

Regards,

K. Y

Greg KH

2011-02-19 01:02 UTC #7 lore raw

Quoted 86 lines · KY Srinivasan

Quoted 48 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, February 18, 2011 5:29 PM
To: KY Srinivasan
Cc: Greg KH; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 37 lines · KY Srinivasan
Quoted 37 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, February 18, 2011 5:07 PM
To: KY Srinivasan
Cc: Greg KH; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 25 lines · KY Srinivasan
Quoted 12 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, February 18, 2011 4:14 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 1 line · K. Y. Srinivasan
Quoted 7 lines
Quoted 3 lines
Signed-off-by K. Y. Srinivasan <[email protected]>
Signed-off-by Haiyang Zhang <[email protected]>
Signed-off-by Hank Janssen <[email protected]>
You didn't run this through checkpatch.pl.
Please do so and fix the warning it gives you.
Greg, I did run the checkpatch script against this patch and the only complaint I got was with regards to the IRQF_SAMPLE_RANDOM flag that I pass. As a virtual machine, this is the only external event that the VM is going to see and so I chose to keep this flag. Is there something that would replace this flag; looking at the Xen drivers they do pass this flag.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
But that flag is going away, right? And this really can't be a valid source of entropy as the HV channel is pretty predictable.
Is it going away? What would replace this. Is all interrupt sources considered predictable?
Did you read the file that the checkpatch script told you to about this entry?
It is only after reading the document, I decided to keep that flag. Please note this is not a question of some interrupt sources not being
a good source of entropy; for this VM this is the only source of interrupts. The document on this flag talked about how people were incorrectly marking their interrupt as an entropy source; in this case there is not much of a choice.
Quoted 11 lines
Quoted 3 lines
This is the only unpredictable thing happening in the VM and that is the reason I chose to keep the flag.
If you remove it, do we loose all entropy for the VM?
Quoted 3 lines
Quoted 2 lines
If you are only using this because Xen does/did it, that's not a valid excuse :)
Surely, you are joking.
Not at all.
To set the record straight here, this flag is in the existing code. After I ran checkpatch, I toyed with the idea of getting rid of this. Then I decided to keep it for all the reasons I mentioned earlier.
Quoted 5 lines
Quoted 2 lines
In any event I am sending you a new patch with that flag removed.
Have you tested to see if you now loose all entropy, and it causes problems or not?

Quoted 8 lines

I am glad you asked me to test it. When I remove this flag, the entropy goes down significantly and this is not surprising. Looking at /proc/sys/kernel/entropy/entropy_avail, with the original patch after a couple of compiles the number would be in thousands. With that flag removed, I have the VM up for about an hour, even after a couple of compiles, the entropy number is yet to crack 200.
Let me know how you want to proceed here.

Ok, my fault, let's keep the original flag. Care to resend the patch you had originally sent?

thanks,

greg k-h

KY Srinivasan

author

2011-02-19 01:19 UTC #8 lore raw

Quoted 107 lines

-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, February 18, 2011 8:03 PM
To: KY Srinivasan
Cc: Greg KH; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 38 lines · KY Srinivasan
Quoted 38 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, February 18, 2011 5:29 PM
To: KY Srinivasan
Cc: Greg KH; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 27 lines · KY Srinivasan
Quoted 27 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, February 18, 2011 5:07 PM
To: KY Srinivasan
Cc: Greg KH; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 16 lines · KY Srinivasan
Quoted 16 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, February 18, 2011 4:14 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 4 lines · K. Y. Srinivasan
Signed-off-by K. Y. Srinivasan <[email protected]>
Signed-off-by Haiyang Zhang <[email protected]>
Signed-off-by Hank Janssen <[email protected]>
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
You didn't run this through checkpatch.pl.
Please do so and fix the warning it gives you.
Greg, I did run the checkpatch script against this patch and the only complaint I got was with regards to the IRQF_SAMPLE_RANDOM flag
that I
Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
pass. As a virtual machine, this is the only external event that the VM is going to see and so I chose to keep this flag. Is there something that would replace this flag; looking at the Xen drivers they do pass this flag.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
But that flag is going away, right? And this really can't be a valid source of entropy as the HV channel is pretty predictable.
Is it going away? What would replace this. Is all interrupt sources considered predictable?
Did you read the file that the checkpatch script told you to about this entry?
Quoted 9 lines
It is only after reading the document, I decided to keep that flag. Please note this is not a question of some interrupt sources not being
a good source of entropy; for this VM this is the only source of interrupts. The document on this flag talked about how people were incorrectly marking their interrupt as an entropy source; in this case there is not much of a choice.
Quoted 2 lines
Quoted 2 lines
This is the only unpredictable thing happening in the VM and that is the
reason
Quoted 15 lines
Quoted 9 lines
Quoted 1 line
I chose to keep the flag.
If you remove it, do we loose all entropy for the VM?
Quoted 3 lines
Quoted 2 lines
If you are only using this because Xen does/did it, that's not a valid excuse :)
Surely, you are joking.
Not at all.
To set the record straight here, this flag is in the existing code. After I ran checkpatch, I toyed with the idea of getting rid of this. Then I decided to keep it for all the reasons I mentioned earlier.
Quoted 1 line
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
In any event I am sending you a new patch with that flag removed.
Have you tested to see if you now loose all entropy, and it causes problems or not?
I am glad you asked me to test it. When I remove this flag, the entropy goes
down
Quoted 6 lines
significantly and this is not surprising. Looking at /proc/sys/kernel/entropy/entropy_avail, with the original patch after a couple of compiles the number would be in thousands. With that flag removed, I have the VM up for about an hour, even after a couple of compiles, the entropy number is yet to crack 200.

Quoted 4 lines

Quoted 1 line
Let me know how you want to proceed here.
Ok, my fault, let's keep the original flag. Care to resend the patch you had originally sent?

Thanks Greg; I just sent it. I will deal with irq balancing issue in a separate patch.

Regards,

K. Y

Thomas Gleixner

2011-02-19 10:23 UTC #9 lore raw

In reply to K. Y. Srinivasan

Quoted 20 lines · K. Y. Srinivasan

drivers/staging/hv/vmbus_drv.c +10 -3

-/* 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;
+

drivers/staging/hv/vmbus_drv.c struct vmbus_driver_context { · line 76

Quoted 10 lines

drivers/staging/hv/vmbus_drv.c +10 -0

+	/*
+	 * 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 ?

Thanks,

tglx

KY Srinivasan

author

2011-02-19 14:34 UTC #10 lore raw

Quoted 49 lines

-----Original Message-----
From: Thomas Gleixner [mailto:[email protected]]
Sent: Saturday, February 19, 2011 5:23 AM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Haiyang Zhang; Hank Janssen
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 20 lines · K. Y. Srinivasan
drivers/staging/hv/vmbus_drv.c +10 -3
-/* 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;
+
drivers/staging/hv/vmbus_drv.c struct vmbus_driver_context { · line 76
Quoted 10 lines
drivers/staging/hv/vmbus_drv.c +10 -0
+	/*
+	 * 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.

Quoted 1 line

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

Thomas Gleixner

2011-02-19 15:12 UTC #11 lore raw

Quoted 8 lines · KY Srinivasan

Quoted 7 lines
From: Thomas Gleixner [mailto:[email protected]]
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 ?

Quoted 9 lines

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.

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

Btw, that whole interrupt handler with two tasklets, one of them scheduling work is just screaming threaded interrupt handler.

Thanks,

tglx

KY Srinivasan

author

2011-02-19 16:46 UTC #12 lore raw

Quoted 30 lines

-----Original Message-----
From: Thomas Gleixner [mailto:[email protected]]
Sent: Saturday, February 19, 2011 10:12 AM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Haiyang Zhang; Hank Janssen
Subject: RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 8 lines · KY Srinivasan
Quoted 7 lines
From: Thomas Gleixner [mailto:[email protected]]
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 ?
Quoted 9 lines
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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 3 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

Thomas Gleixner

2011-02-20 16:15 UTC #13 lore raw

Quoted 9 lines · KY Srinivasan

Quoted 3 lines
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.

So in case your driver gets the interrupt line first, which the other driver wants to acquire as well, then what? Do you want to do that probe magic in the other driver as well? What if this is a regular device driver which gets its irq number from ACPI/PCI or whatever. Then that driver simply wont work as it's interrupt line is busy.

Quoted 10 lines

Quoted 3 lines
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.

So you trade breaking the PV stuff against breaking random other drivers? That doesn't sound like a brilliant idea.

There are various ways to solve that proper.

- You can provide the interrupt number from ACPI/PCI or whatever your HV provides as enumeration.

- Use a fixed vector like XEN does for the event channel

- Use dynamic allocation in the IOAPIC space like the kernel does for MSI(-X)

Thanks,

tglx

KY Srinivasan

author

2011-02-21 03:43 UTC #14 lore raw

Quoted 48 lines

-----Original Message-----
From: Thomas Gleixner [mailto:[email protected]]
Sent: Sunday, February 20, 2011 11:16 AM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Haiyang Zhang; Hank Janssen
Subject: RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 9 lines · KY Srinivasan
Quoted 3 lines
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.
So in case your driver gets the interrupt line first, which the other driver wants to acquire as well, then what? Do you want to do that probe magic in the other driver as well? What if this is a regular device driver which gets its irq number from ACPI/PCI or whatever. Then that driver simply wont work as it's interrupt line is busy.
Quoted 10 lines
Quoted 3 lines
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.
So you trade breaking the PV stuff against breaking random other drivers? That doesn't sound like a brilliant idea.
There are various ways to solve that proper.
- You can provide the interrupt number from ACPI/PCI or whatever your HV provides as enumeration.

Quoted 8 lines

- Use a fixed vector like XEN does for the event channel
- Use dynamic allocation in the IOAPIC space like the kernel does for MSI(-X)
Thanks,
tglx

I am not claiming that what I have done here is the best possible solution. However, I will submit to you that it is better than what we had here prior to this patch. I will address this and a whole lot of other issues in future patches.

Regards,

K. Y

Greg KH

2011-02-21 03:50 UTC #15 lore raw

Quoted 4 lines · KY Srinivasan

I am not claiming that what I have done here is the best possible solution. However, I will submit to you that it is better than what we had here prior to this patch. I will address this and a whole lot of other issues in future patches.

Care to document it in the TODO file so it doesn't get forgotten?

thanks,

greg k-h

Thomas Gleixner

2011-02-21 11:02 UTC #16 lore raw

In reply to KY Srinivasan

Quoted 37 lines · KY Srinivasan

Quoted 37 lines
Quoted 5 lines
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.
So in case your driver gets the interrupt line first, which the other driver wants to acquire as well, then what? Do you want to do that probe magic in the other driver as well? What if this is a regular device driver which gets its irq number from ACPI/PCI or whatever. Then that driver simply wont work as it's interrupt line is busy.
Quoted 10 lines
Quoted 3 lines
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.
So you trade breaking the PV stuff against breaking random other drivers? That doesn't sound like a brilliant idea.
There are various ways to solve that proper.
- You can provide the interrupt number from ACPI/PCI or whatever your HV provides as enumeration.
- Use a fixed vector like XEN does for the event channel
- Use dynamic allocation in the IOAPIC space like the kernel does for MSI(-X)

Quoted 8 lines

Quoted 3 lines
Thanks,
tglx
I am not claiming that what I have done here is the best possible solution. However, I will submit to you that it is better than what we had here prior to this patch. I will address this and a whole lot of other issues in future patches.

No, it's _NOT_ better in any way. You trade breaking your PV thing against breaking random other drivers. Care to explain why you think that's better ?

Thanks,

tglx

KY Srinivasan

author

2011-02-21 14:40 UTC #17 lore raw

Quoted 53 lines

-----Original Message-----
From: Thomas Gleixner [mailto:[email protected]]
Sent: Monday, February 21, 2011 6:03 AM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Haiyang Zhang; Hank Janssen
Subject: RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 41 lines · KY Srinivasan
Quoted 40 lines
Quoted 5 lines
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.
So in case your driver gets the interrupt line first, which the other driver wants to acquire as well, then what? Do you want to do that probe magic in the other driver as well? What if this is a regular device driver which gets its irq number from ACPI/PCI or whatever. Then that driver simply wont work as it's interrupt line is busy.
Quoted 10 lines
Quoted 3 lines
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.
So you trade breaking the PV stuff against breaking random other drivers? That doesn't sound like a brilliant idea.
There are various ways to solve that proper.
- You can provide the interrupt number from ACPI/PCI or whatever your HV provides as enumeration.
- Use a fixed vector like XEN does for the event channel
- Use dynamic allocation in the IOAPIC space like the kernel does for MSI(-X)
Thanks,
tglx

Quoted 8 lines

Quoted 4 lines
I am not claiming that what I have done here is the best possible solution. However, I will submit to you that it is better than what we had here prior to this patch. I will address this and a whole lot of other issues in future patches.
No, it's _NOT_ better in any way. You trade breaking your PV thing against breaking random other drivers. Care to explain why you think that's better ?

The root device for the VM is bound to the PV driver on some distributions. So, if we cannot load the PV drivers, we do not have a system that boots. In general, the system performance without these PV drivers is so poor that for all practical purposes, having the PV drivers is almost a requirement for having a useable system. While the platform supports configuration of the VM with some emulated devices, it is not a recommended configuration (because of performance reasons) for Linux virtual machines on the Hyper-V platform.

I have spent significantly more time debating this patch than developing this patch that I still think improves the current driver. I will leave it to Greg and other powers that be to decide if this patch will be accepted. Let me know what your verdict is.

Regards,

K. Y

Thomas Gleixner

2011-02-21 14:51 UTC #18 lore raw

Quoted 23 lines · KY Srinivasan

Quoted 14 lines
Quoted 14 lines
Quoted 13 lines
There are various ways to solve that proper.
- You can provide the interrupt number from ACPI/PCI or whatever your HV provides as enumeration.
- Use a fixed vector like XEN does for the event channel
- Use dynamic allocation in the IOAPIC space like the kernel does for MSI(-X)
Thanks,
tglx
Quoted 8 lines
Quoted 4 lines
I am not claiming that what I have done here is the best possible solution. However, I will submit to you that it is better than what we had here prior to this patch. I will address this and a whole lot of other issues in future patches.
No, it's _NOT_ better in any way. You trade breaking your PV thing against breaking random other drivers. Care to explain why you think that's better ?

Quoted 7 lines

The root device for the VM is bound to the PV driver on some distributions. So, if we cannot load the PV drivers, we do not have a system that boots. In general, the system performance without these PV drivers is so poor that for all practical purposes, having the PV drivers is almost a requirement for having a useable system. While the platform supports configuration of the VM with some emulated devices, it is not a recommended configuration (because of performance reasons) for Linux virtual machines on the Hyper-V platform.

It does not matter whether it's recommended or not. If it results in a non usable emulated device it's broken. Just imagine you grab the serial interrupt, which results in not having a debug console. Not at all acceptable.

Quoted 4 lines

I have spent significantly more time debating this patch than developing this patch that I still think improves the current driver. I will leave it to Greg and other powers that be to decide if this patch will be accepted. Let me know what your verdict is.

As I said, any other clean solution (and I pointed out 3) over this hack.

Thanks,

tglx

Greg KH

2011-02-21 15:43 UTC #19 lore raw

Quoted 37 lines · Thomas Gleixner

Quoted 23 lines · KY Srinivasan
Quoted 14 lines
Quoted 14 lines
Quoted 13 lines
There are various ways to solve that proper.
- You can provide the interrupt number from ACPI/PCI or whatever your HV provides as enumeration.
- Use a fixed vector like XEN does for the event channel
- Use dynamic allocation in the IOAPIC space like the kernel does for MSI(-X)
Thanks,
tglx
Quoted 8 lines
Quoted 4 lines
I am not claiming that what I have done here is the best possible solution. However, I will submit to you that it is better than what we had here prior to this patch. I will address this and a whole lot of other issues in future patches.
No, it's _NOT_ better in any way. You trade breaking your PV thing against breaking random other drivers. Care to explain why you think that's better ?
Quoted 7 lines
The root device for the VM is bound to the PV driver on some distributions. So, if we cannot load the PV drivers, we do not have a system that boots. In general, the system performance without these PV drivers is so poor that for all practical purposes, having the PV drivers is almost a requirement for having a useable system. While the platform supports configuration of the VM with some emulated devices, it is not a recommended configuration (because of performance reasons) for Linux virtual machines on the Hyper-V platform.
It does not matter whether it's recommended or not. If it results in a non usable emulated device it's broken. Just imagine you grab the serial interrupt, which results in not having a debug console. Not at all acceptable.

Quoted 7 lines

Quoted 4 lines
I have spent significantly more time debating this patch than developing this patch that I still think improves the current driver. I will leave it to Greg and other powers that be to decide if this patch will be accepted. Let me know what your verdict is.
As I said, any other clean solution (and I pointed out 3) over this hack.

Yeah, that sounds like the best solution, let's not cause accidental breakage of systems that were working already.

KY, care to fix this up properly now? There's no rush to take your previous patch, as that is working ok for now, right?

thanks,

greg k-h

Greg KH

2011-02-23 19:16 UTC #20 lore raw

In reply to Thomas Gleixner

Quoted 37 lines · Thomas Gleixner

Quoted 23 lines · KY Srinivasan
Quoted 14 lines
Quoted 14 lines
Quoted 13 lines
There are various ways to solve that proper.
- You can provide the interrupt number from ACPI/PCI or whatever your HV provides as enumeration.
- Use a fixed vector like XEN does for the event channel
- Use dynamic allocation in the IOAPIC space like the kernel does for MSI(-X)
Thanks,
tglx
Quoted 8 lines
Quoted 4 lines
I am not claiming that what I have done here is the best possible solution. However, I will submit to you that it is better than what we had here prior to this patch. I will address this and a whole lot of other issues in future patches.
No, it's _NOT_ better in any way. You trade breaking your PV thing against breaking random other drivers. Care to explain why you think that's better ?
Quoted 7 lines
The root device for the VM is bound to the PV driver on some distributions. So, if we cannot load the PV drivers, we do not have a system that boots. In general, the system performance without these PV drivers is so poor that for all practical purposes, having the PV drivers is almost a requirement for having a useable system. While the platform supports configuration of the VM with some emulated devices, it is not a recommended configuration (because of performance reasons) for Linux virtual machines on the Hyper-V platform.
It does not matter whether it's recommended or not. If it results in a non usable emulated device it's broken. Just imagine you grab the serial interrupt, which results in not having a debug console. Not at all acceptable.

Quoted 7 lines

Quoted 4 lines
I have spent significantly more time debating this patch than developing this patch that I still think improves the current driver. I will leave it to Greg and other powers that be to decide if this patch will be accepted. Let me know what your verdict is.
As I said, any other clean solution (and I pointed out 3) over this hack.

I agree, KY, please fix this up properly, I've dropped this patch from my queue now.

thanks,

greg k-h

KY Srinivasan

author

2011-02-23 19:22 UTC #21 lore raw

Quoted 55 lines

-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Wednesday, February 23, 2011 2:16 PM
To: Thomas Gleixner
Cc: KY Srinivasan; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Quoted 4 lines · Thomas Gleixner
Quoted 3 lines · KY Srinivasan
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
There are various ways to solve that proper.
- You can provide the interrupt number from ACPI/PCI or whatever
your HV
Quoted 20 lines
Quoted 20 lines
Quoted 19 lines
Quoted 7 lines
Quoted 7 lines
provides as enumeration.
- Use a fixed vector like XEN does for the event channel
- Use dynamic allocation in the IOAPIC space like the kernel does for MSI(-X)
Quoted 8 lines
Quoted 3 lines
Thanks,
tglx
I am not claiming that what I have done here is the best possible solution. However, I will submit to you that it is better than what we had here prior to this patch. I will address this and a whole lot of other issues in future patches.
No, it's _NOT_ better in any way. You trade breaking your PV thing against breaking random other drivers. Care to explain why you think that's better ?
Quoted 5 lines
Quoted 5 lines
The root device for the VM is bound to the PV driver on some distributions. So, if we cannot load the PV drivers, we do not have a system that boots. In general, the system performance without these PV drivers is so poor that for all practical purposes, having the PV drivers is almost a requirement for having a useable system. While the platform supports configuration of the
VM with
Quoted 13 lines
Quoted 2 lines
some emulated devices, it is not a recommended configuration (because of performance reasons) for Linux virtual machines on the Hyper-V platform.
It does not matter whether it's recommended or not. If it results in a non usable emulated device it's broken. Just imagine you grab the serial interrupt, which results in not having a debug console. Not at all acceptable.
Quoted 4 lines
I have spent significantly more time debating this patch than developing this patch that I still think improves the current driver. I will leave it to Greg and other powers that be to decide if this patch will be accepted. Let me know what your verdict is.

Quoted 5 lines

Quoted 2 lines
As I said, any other clean solution (and I pointed out 3) over this hack.
I agree, KY, please fix this up properly, I've dropped this patch from my queue now.

Ok; will do.

K. Y

Quoted 4 lines

thanks,
greg k-h