RE: [PATCH 2/3]: Staging: hv: Use native wait primitives
-Original Message- From: Jiri Slaby [mailto:jirisl...@gmail.com] Sent: Tuesday, February 15, 2011 4:21 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives On 02/11/2011 06:59 PM, K. Y. Srinivasan wrote: In preperation for getting rid of the osd layer; change the code to use native wait interfaces. As part of this, fixed the buggy implementation in the osd_wait_primitive where the condition was cleared potentially after the condition was signalled. ... @@ -566,7 +567,11 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer, } } - osd_waitevent_wait(msginfo-waitevent); + wait_event_timeout(msginfo-waitevent, + msginfo-wait_condition, + msecs_to_jiffies(1000)); + BUG_ON(msginfo-wait_condition == 0); The added BUG_ONs all over the code look scary. These shouldn't be BUG_ONs at all. You should maybe warn and bail out, but not kill the whole machine. This is Linux code running as a guest on a Windows host; and so the guest cannot tolerate a failure of the host. In the cases where I have chosen to BUG_ON, there is no reasonable recovery possible when the host is non-functional (as determined by a non-responsive host). And looking at the code, more appropriate would be completion instead of wait events. And msecs_to_jiffies(1000) == HZ. Agreed. In this first round of cleanup, I chose to keep the primitives as they were in osd.c. Greg, if it is ok with you, I will send you a patch that fixes these issues on top of the patches I have already sent. Regards, K. Y @@ -689,7 +693,8 @@ static void vmbus_ongpadl_torndown( memcpy(msginfo-response.gpadl_torndown, gpadl_torndown, sizeof(struct vmbus_channel_gpadl_torndown)); - osd_waitevent_set(msginfo-waitevent); + msginfo-wait_condition = 1; + wake_up(msginfo-waitevent); break; } } @@ -730,7 +735,8 @@ static void vmbus_onversion_response( memcpy(msginfo-response.version_response, version_response, sizeof(struct vmbus_channel_version_response)); - osd_waitevent_set(msginfo-waitevent); + msginfo-wait_condition = 1; + wake_up(msginfo-waitevent); } } spin_unlock_irqrestore(vmbus_connection.channelmsg_lock, flags); regards, -- js ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCH ]:Staging: hv: Allocate the vmbus irq dynamically
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/vmbus_drv.c | 49 +++
1 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 459c707..f279e6a 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -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,25 @@ struct vmbus_driver_context {
struct vm_device device_ctx;
};
+/*
+ * Find an un-used IRQ that the VMBUS can use. If none is available; return -1.
+ */
+
+static int vmbus_get_irq(void)
+{
+ unsigned int avail_irq_mask;
+ int irq = -1;
+ /*
+* 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 +97,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[] = {
@@ -466,7 +482,7 @@ static int vmbus_bus_init(void)
struct hv_driver *driver = vmbus_drv.drv_obj;
struct vm_device *dev_ctx = vmbus_drv.device_ctx;
int ret;
- unsigned int vector;
+ unsigned int vector, prev_irq = ~0;
DPRINT_INFO(VMBUS, +++ HV Driver version = %s +++,
HV_DRV_VERSION);
@@ -518,19 +534,23 @@ static int vmbus_bus_init(void)
}
/* 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_irq_again:
+ vmbus_irq = vmbus_get_irq();
+ if ((vmbus_irq 0) || (prev_irq == vmbus_irq)) {
+ printk(KERN_WARNING VMBUS_DRV: Failed to allocate IRQ\n);
bus_unregister(vmbus_drv_ctx-bus);
-
ret = -1;
goto cleanup;
}
- vector = VMBUS_IRQ_VECTOR;
+ prev_irq = vmbus_irq;
+
+ ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
+ driver-name, NULL);
+
+ if (ret != 0)
+ goto get_irq_again;
+
+ vector = IRQ0_VECTOR + vmbus_irq;
DPRINT_INFO(VMBUS_DRV, irq 0x%x vector 0x%x, vmbus_irq, vector);
@@ -1117,7 +1137,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);
--
1.5.5.6
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/3]: Staging: hv: Use native wait primitives
On Tue, Feb 15, 2011 at 01:35:56PM +, KY Srinivasan wrote: -Original Message- From: Jiri Slaby [mailto:jirisl...@gmail.com] Sent: Tuesday, February 15, 2011 4:21 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives On 02/11/2011 06:59 PM, K. Y. Srinivasan wrote: In preperation for getting rid of the osd layer; change the code to use native wait interfaces. As part of this, fixed the buggy implementation in the osd_wait_primitive where the condition was cleared potentially after the condition was signalled. ... @@ -566,7 +567,11 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer, } } - osd_waitevent_wait(msginfo-waitevent); + wait_event_timeout(msginfo-waitevent, + msginfo-wait_condition, + msecs_to_jiffies(1000)); + BUG_ON(msginfo-wait_condition == 0); The added BUG_ONs all over the code look scary. These shouldn't be BUG_ONs at all. You should maybe warn and bail out, but not kill the whole machine. This is Linux code running as a guest on a Windows host; and so the guest cannot tolerate a failure of the host. In the cases where I have chosen to BUG_ON, there is no reasonable recovery possible when the host is non-functional (as determined by a non-responsive host). If you have a non-responsive host, wouldn't that imply that this guest code wouldn't run at all? :) Having BUG_ON() in drivers is not a good idea either way. Please remove these in future patches. And looking at the code, more appropriate would be completion instead of wait events. And msecs_to_jiffies(1000) == HZ. Agreed. In this first round of cleanup, I chose to keep the primitives as they were in osd.c. Greg, if it is ok with you, I will send you a patch that fixes these issues on top of the patches I have already sent. Yes, that is fine. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 2/3]: Staging: hv: Use native wait primitives
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Tuesday, February 15, 2011 9:03 AM To: KY Srinivasan Cc: Jiri Slaby; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives On Tue, Feb 15, 2011 at 01:35:56PM +, KY Srinivasan wrote: -Original Message- From: Jiri Slaby [mailto:jirisl...@gmail.com] Sent: Tuesday, February 15, 2011 4:21 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives On 02/11/2011 06:59 PM, K. Y. Srinivasan wrote: In preperation for getting rid of the osd layer; change the code to use native wait interfaces. As part of this, fixed the buggy implementation in the osd_wait_primitive where the condition was cleared potentially after the condition was signalled. ... @@ -566,7 +567,11 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer, } } - osd_waitevent_wait(msginfo-waitevent); + wait_event_timeout(msginfo-waitevent, + msginfo-wait_condition, + msecs_to_jiffies(1000)); + BUG_ON(msginfo-wait_condition == 0); The added BUG_ONs all over the code look scary. These shouldn't be BUG_ONs at all. You should maybe warn and bail out, but not kill the whole machine. This is Linux code running as a guest on a Windows host; and so the guest cannot tolerate a failure of the host. In the cases where I have chosen to BUG_ON, there is no reasonable recovery possible when the host is non-functional (as determined by a non-responsive host). If you have a non-responsive host, wouldn't that imply that this guest code wouldn't run at all? :) The fact that on a particular transaction the host has not responded within an expected time interval does not necessarily mean that the guest code would not be running. There may be issues on the host side that may be either transient or permanent that may cause problems like this. Keep in mind, HyperV is a type 1 hypervisor that would schedule all VMs including the host and so, guest would get scheduled. Having BUG_ON() in drivers is not a good idea either way. Please remove these in future patches. In situations where there is not a reasonable rollback strategy (for instance in one of the cases, we are granting access to the guest physical pages to the host) we really have only 2 options: 1) Wait until the host responds. This wait could potentially be unbounded and in fact this was the way the code was to begin with. One of the reviewers had suggested that unbounded wait was to be corrected. 2) Wait for a specific period and if the host does not respond within a reasonable period, kill the guest since there is no recovery possible. I chose option 2, as part of addressing some of the prior review comments. If the consensus now is to go back to option 1, I am fine with that; I will send you a patch to rectify this. Regards, K. Y And looking at the code, more appropriate would be completion instead of wait events. And msecs_to_jiffies(1000) == HZ. Agreed. In this first round of cleanup, I chose to keep the primitives as they were in osd.c. Greg, if it is ok with you, I will send you a patch that fixes these issues on top of the patches I have already sent. Yes, that is fine. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/3]: Staging: hv: Use native wait primitives
On Tue, Feb 15, 2011 at 04:22:20PM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Tuesday, February 15, 2011 9:03 AM To: KY Srinivasan Cc: Jiri Slaby; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives On Tue, Feb 15, 2011 at 01:35:56PM +, KY Srinivasan wrote: -Original Message- From: Jiri Slaby [mailto:jirisl...@gmail.com] Sent: Tuesday, February 15, 2011 4:21 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives On 02/11/2011 06:59 PM, K. Y. Srinivasan wrote: In preperation for getting rid of the osd layer; change the code to use native wait interfaces. As part of this, fixed the buggy implementation in the osd_wait_primitive where the condition was cleared potentially after the condition was signalled. ... @@ -566,7 +567,11 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer, } } - osd_waitevent_wait(msginfo-waitevent); + wait_event_timeout(msginfo-waitevent, + msginfo-wait_condition, + msecs_to_jiffies(1000)); + BUG_ON(msginfo-wait_condition == 0); The added BUG_ONs all over the code look scary. These shouldn't be BUG_ONs at all. You should maybe warn and bail out, but not kill the whole machine. This is Linux code running as a guest on a Windows host; and so the guest cannot tolerate a failure of the host. In the cases where I have chosen to BUG_ON, there is no reasonable recovery possible when the host is non-functional (as determined by a non-responsive host). If you have a non-responsive host, wouldn't that imply that this guest code wouldn't run at all? :) The fact that on a particular transaction the host has not responded within an expected time interval does not necessarily mean that the guest code would not be running. There may be issues on the host side that may be either transient or permanent that may cause problems like this. Keep in mind, HyperV is a type 1 hypervisor that would schedule all VMs including the host and so, guest would get scheduled. Having BUG_ON() in drivers is not a good idea either way. Please remove these in future patches. In situations where there is not a reasonable rollback strategy (for instance in one of the cases, we are granting access to the guest physical pages to the host) we really have only 2 options: 1) Wait until the host responds. This wait could potentially be unbounded and in fact this was the way the code was to begin with. One of the reviewers had suggested that unbounded wait was to be corrected. 2) Wait for a specific period and if the host does not respond within a reasonable period, kill the guest since there is no recovery possible. Killing the guest is a very serious thing, causing all sorts of possible problems with it, right? I chose option 2, as part of addressing some of the prior review comments. If the consensus now is to go back to option 1, I am fine with that; Unbounded waits aren't ok either, you need some sort of timeout. But, as this is a bit preferable to dieing, I suggest doing this, and comment the heck out of it to explain all of this for anyone who reads it. thanks, greg k-h ___ 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
On Tue, Feb 15, 2011 at 07:15:37AM -0800, K. Y. Srinivasan wrote:
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/vmbus_drv.c | 49 +++
1 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 459c707..f279e6a 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -36,9 +36,7 @@
#include vmbus_private.h
-/* FIXME! We need to do this dynamically for PIC and APIC system */
-#define VMBUS_IRQ0x5
-#define VMBUS_IRQ_VECTOR IRQ5_VECTOR
+static int vmbus_irq;
/* Main vmbus driver data structure */
struct vmbus_driver_context {
@@ -57,6 +55,25 @@ struct vmbus_driver_context {
struct vm_device device_ctx;
};
+/*
+ * Find an un-used IRQ that the VMBUS can use. If none is available; return
-1.
+ */
+
+static int vmbus_get_irq(void)
No extra blank line between function comment and function.
+{
+ unsigned int avail_irq_mask;
+ int irq = -1;
+ /*
Put an extra blank line after your variables please.
+ * 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 +97,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[] = {
@@ -466,7 +482,7 @@ static int vmbus_bus_init(void)
struct hv_driver *driver = vmbus_drv.drv_obj;
struct vm_device *dev_ctx = vmbus_drv.device_ctx;
int ret;
- unsigned int vector;
+ unsigned int vector, prev_irq = ~0;
DPRINT_INFO(VMBUS, +++ HV Driver version = %s +++,
HV_DRV_VERSION);
@@ -518,19 +534,23 @@ static int vmbus_bus_init(void)
}
/* 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_irq_again:
+ vmbus_irq = vmbus_get_irq();
+ if ((vmbus_irq 0) || (prev_irq == vmbus_irq)) {
+ printk(KERN_WARNING VMBUS_DRV: Failed to allocate IRQ\n);
bus_unregister(vmbus_drv_ctx-bus);
-
ret = -1;
Please provide a real error code.
goto cleanup;
}
- vector = VMBUS_IRQ_VECTOR;
+ prev_irq = vmbus_irq;
+
+ ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
+ driver-name, NULL);
+
+ if (ret != 0)
+ goto get_irq_again;
You just set up the potential for an endless loop. You almost _never_
want to goto backwards in a function. Please don't do this.
+
+ vector = IRQ0_VECTOR + vmbus_irq;
What's this for?
DPRINT_INFO(VMBUS_DRV, irq 0x%x vector 0x%x, vmbus_irq, vector);
And why are you still printing this out for the whole world to see?
thanks,
greg k-h
___
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
-Original Message-
From: Greg KH [mailto:gre...@suse.de]
Sent: Tuesday, February 15, 2011 11:00 AM
To: KY Srinivasan
Cc: 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, Feb 15, 2011 at 07:15:37AM -0800, K. Y. Srinivasan wrote:
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/vmbus_drv.c | 49 +++--
--
1 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 459c707..f279e6a 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -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,25 @@ struct vmbus_driver_context {
struct vm_device device_ctx;
};
+/*
+ * Find an un-used IRQ that the VMBUS can use. If none is available;
return -1.
+ */
+
+static int vmbus_get_irq(void)
No extra blank line between function comment and function.
I will take care of this.
+{
+ unsigned int avail_irq_mask;
+ int irq = -1;
+ /*
Put an extra blank line after your variables please.
Will do.
+* 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 +97,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[] = {
@@ -466,7 +482,7 @@ static int vmbus_bus_init(void)
struct hv_driver *driver = vmbus_drv.drv_obj;
struct vm_device *dev_ctx = vmbus_drv.device_ctx;
int ret;
- unsigned int vector;
+ unsigned int vector, prev_irq = ~0;
DPRINT_INFO(VMBUS, +++ HV Driver version = %s +++,
HV_DRV_VERSION);
@@ -518,19 +534,23 @@ static int vmbus_bus_init(void)
}
/* 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_irq_again:
+ vmbus_irq = vmbus_get_irq();
+ if ((vmbus_irq 0) || (prev_irq == vmbus_irq)) {
+ printk(KERN_WARNING VMBUS_DRV: Failed to allocate IRQ\n);
bus_unregister(vmbus_drv_ctx-bus);
-
ret = -1;
Please provide a real error code.
Will do.
goto cleanup;
}
- vector = VMBUS_IRQ_VECTOR;
+ prev_irq = vmbus_irq;
+
+ ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
+ driver-name, NULL);
+
+ if (ret != 0)
+ goto get_irq_again;
You just set up the potential for an endless loop. You almost _never_
want to goto backwards in a function. Please don't do this.
The loop is not endless. We need to take care of two conditions here:
1) From the point we selected an irq line to the point where we call
request_irq(), it is possible that someone else could have requested
the same line and so we need to close this window.
2) request_irq() may fail for reasons other than the fact that we
may have lost the line that we thought we got.
So, while we loopback, we check to see if the irq line we got is the same
that we got before (refer to the check soon after the call
to vmbus_get_irq()). This check would ensure that we would not loop
endlessly.
+
+ vector = IRQ0_VECTOR + vmbus_irq;
What's this for?
This is what gets passed to the hypervisor - refer to vmbus_dev_add().
This is existing code.
DPRINT_INFO(VMBUS_DRV, irq 0x%x vector 0x%x, vmbus_irq, vector);
And why are you still printing this out for the whole world to see?
Greg, this patch addressed a specific comment with regards dynamically allocate
the irq.
Hank is working on a patch to address the various DPRINTS in the code.
Regards,
K. Y
thanks,
greg k-h
RE: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically
-Original Message- From: KY Srinivasan Sent: Tuesday, February 15, 2011 8:54 AM -Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Tuesday, February 15, 2011 11:00 AM To: KY Srinivasan Cc: 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 And why are you still printing this out for the whole world to see? Greg, this patch addressed a specific comment with regards dynamically allocate the irq. Hank is working on a patch to address the various DPRINTS in the code. Before the end of the week I will submit two patches for this; Remove DPRINT and change it to printk Remove excessive printouts on startup of vmbus Hank. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCH 2/2] staging: hv: Remove dead code from rndis_filter.c
Signed-off-by: Haiyang Zhang haiya...@microsoft.com Signed-off-by: K. Y. Srinivasan k...@microsoft.com --- drivers/staging/hv/rndis_filter.c | 15 --- 1 files changed, 0 insertions(+), 15 deletions(-) diff --git a/drivers/staging/hv/rndis_filter.c b/drivers/staging/hv/rndis_filter.c index 9dde936..e7189cd 100644 --- a/drivers/staging/hv/rndis_filter.c +++ b/drivers/staging/hv/rndis_filter.c @@ -355,10 +355,6 @@ static void rndis_filter_receive_data(struct rndis_device *dev, struct rndis_packet *rndis_pkt; u32 data_offset; - /* empty ethernet frame ?? */ - /* ASSERT(Packet-PageBuffers[0].Length */ - /* RNDIS_MESSAGE_SIZE(struct rndis_packet)); */ - rndis_pkt = msg-msg.pkt; /* @@ -455,8 +451,6 @@ static int rndis_filter_receive(struct hv_device *dev, case REMOTE_NDIS_INITIALIZE_CMPLT: case REMOTE_NDIS_QUERY_CMPLT: case REMOTE_NDIS_SET_CMPLT: - /* case REMOTE_NDIS_RESET_CMPLT: */ - /* case REMOTE_NDIS_KEEPALIVE_CMPLT: */ /* completion msgs */ rndis_filter_receive_response(rndis_dev, rndis_msg); break; @@ -563,9 +557,6 @@ static int rndis_filter_set_packet_filter(struct rndis_device *dev, u32 status; int ret; - /* ASSERT(RNDIS_MESSAGE_SIZE(struct rndis_set_request) + sizeof(u32) = */ - /* sizeof(struct rndis_message)); */ - request = get_rndis_request(dev, REMOTE_NDIS_SET_MSG, RNDIS_MESSAGE_SIZE(struct rndis_set_request) + sizeof(u32)); @@ -634,8 +625,6 @@ int rndis_filter_init(struct netvsc_driver *drv) drv-base.dev_rm; rndis_filter.inner_drv.base.cleanup = drv-base.cleanup; - /* ASSERT(Driver-OnSend); */ - /* ASSERT(Driver-OnReceiveCallback); */ rndis_filter.inner_drv.send = drv-send; rndis_filter.inner_drv.recv_cb = drv-recv_cb; rndis_filter.inner_drv.link_status_change = @@ -646,7 +635,6 @@ int rndis_filter_init(struct netvsc_driver *drv) drv-base.dev_rm = rndis_filter_device_remove; drv-base.cleanup = rndis_filter_cleanup; drv-send = rndis_filter_send; - /* Driver-QueryLinkStatus = RndisFilterQueryDeviceLinkStatus; */ drv-recv_cb = rndis_filter_receive; return 0; @@ -793,8 +781,6 @@ static int rndis_filte_device_add(struct hv_device *dev, /* Initialize the rndis device */ netDevice = dev-ext; - /* ASSERT(netDevice); */ - /* ASSERT(netDevice-Device); */ netDevice-extension = rndisDevice; rndisDevice-net_dev = netDevice; @@ -882,7 +868,6 @@ static int rndis_filter_send(struct hv_device *dev, /* Add the rndis header */ filterPacket = (struct rndis_filter_packet *)pkt-extension; - /* ASSERT(filterPacket); */ memset(filterPacket, 0, sizeof(struct rndis_filter_packet)); -- 1.6.3.2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] staging: hv: Remove dead code from netvsc.c
On Mon, 14 Feb 2011, Haiyang Zhang wrote: Signed-off-by: Haiyang Zhang haiya...@microsoft.com Signed-off-by: K. Y. Srinivasan k...@microsoft.com --- drivers/staging/hv/netvsc.c | 34 -- 1 files changed, 0 insertions(+), 34 deletions(-) ... Reviewed-by: Jesper Juhl j...@chaosbits.net -- Jesper Juhl j...@chaosbits.nethttp://www.chaosbits.net/ Plain text mails only, please. Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] staging: hv: Remove dead code from rndis_filter.c
On Mon, 14 Feb 2011, Haiyang Zhang wrote: Signed-off-by: Haiyang Zhang haiya...@microsoft.com Signed-off-by: K. Y. Srinivasan k...@microsoft.com --- drivers/staging/hv/rndis_filter.c | 15 --- 1 files changed, 0 insertions(+), 15 deletions(-) ... I would probably have said remove commented out code from rndis_filter.c rather than remove dead code ..., but nevermind :-) Looks good to me. Reviewed-by: Jesper Juhl j...@chaosbits.net -- Jesper Juhl j...@chaosbits.nethttp://www.chaosbits.net/ Plain text mails only, please. Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html ___ 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
On Tue, Feb 15, 2011 at 04:53:41PM +, KY Srinivasan wrote:
@@ -518,19 +534,23 @@ static int vmbus_bus_init(void)
}
/* 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_irq_again:
+ vmbus_irq = vmbus_get_irq();
+ if ((vmbus_irq 0) || (prev_irq == vmbus_irq)) {
+ printk(KERN_WARNING VMBUS_DRV: Failed to allocate IRQ\n);
bus_unregister(vmbus_drv_ctx-bus);
-
ret = -1;
Please provide a real error code.
Will do.
goto cleanup;
}
- vector = VMBUS_IRQ_VECTOR;
+ prev_irq = vmbus_irq;
+
+ ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
+ driver-name, NULL);
+
+ if (ret != 0)
+ goto get_irq_again;
You just set up the potential for an endless loop. You almost _never_
want to goto backwards in a function. Please don't do this.
The loop is not endless. We need to take care of two conditions here:
1) From the point we selected an irq line to the point where we call
request_irq(), it is possible that someone else could have requested
the same line and so we need to close this window.
2) request_irq() may fail for reasons other than the fact that we
may have lost the line that we thought we got.
So, while we loopback, we check to see if the irq line we got is the same
that we got before (refer to the check soon after the call
to vmbus_get_irq()). This check would ensure that we would not loop
endlessly.
That's very subtle, and easy to overlook. So please don't do it. Make
it obvious that you will not constantly loop, and again, don't call goto
backwards. goto is for error handling only, unless you are writing
scheduler code, and you aren't doing that here.
Please rework your logic before resending this.
thanks,
greg k-h
___
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
On Tue, Feb 15, 2011 at 04:59:28PM +, Hank Janssen wrote: -Original Message- From: KY Srinivasan Sent: Tuesday, February 15, 2011 8:54 AM -Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Tuesday, February 15, 2011 11:00 AM To: KY Srinivasan Cc: 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 And why are you still printing this out for the whole world to see? Greg, this patch addressed a specific comment with regards dynamically allocate the irq. Hank is working on a patch to address the various DPRINTS in the code. Before the end of the week I will submit two patches for this; Remove DPRINT and change it to printk No, use dev_dbg() and friends instead of raw printk() calls. Remove excessive printouts on startup of vmbus That would be nice to see. There should be almost nothing outputed by these drivers on normal operation. thanks, greg k-h ___ 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
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Tuesday, February 15, 2011 9:23 AM Before the end of the week I will submit two patches for this; Remove DPRINT and change it to printk No, use dev_dbg() and friends instead of raw printk() calls. Will do, you caught me just as I was starting the conversion :) Remove excessive printouts on startup of vmbus That would be nice to see. There should be almost nothing outputed by these drivers on normal operation. Agreed. Hank. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 2/3]: Staging: hv: Use native wait primitives
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Tuesday, February 15, 2011 11:30 AM To: KY Srinivasan Cc: Jiri Slaby; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives On Tue, Feb 15, 2011 at 04:22:20PM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Tuesday, February 15, 2011 9:03 AM To: KY Srinivasan Cc: Jiri Slaby; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives On Tue, Feb 15, 2011 at 01:35:56PM +, KY Srinivasan wrote: -Original Message- From: Jiri Slaby [mailto:jirisl...@gmail.com] Sent: Tuesday, February 15, 2011 4:21 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives On 02/11/2011 06:59 PM, K. Y. Srinivasan wrote: In preperation for getting rid of the osd layer; change the code to use native wait interfaces. As part of this, fixed the buggy implementation in the osd_wait_primitive where the condition was cleared potentially after the condition was signalled. ... @@ -566,7 +567,11 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer, } } - osd_waitevent_wait(msginfo-waitevent); + wait_event_timeout(msginfo-waitevent, + msginfo-wait_condition, + msecs_to_jiffies(1000)); + BUG_ON(msginfo-wait_condition == 0); The added BUG_ONs all over the code look scary. These shouldn't be BUG_ONs at all. You should maybe warn and bail out, but not kill the whole machine. This is Linux code running as a guest on a Windows host; and so the guest cannot tolerate a failure of the host. In the cases where I have chosen to BUG_ON, there is no reasonable recovery possible when the host is non-functional (as determined by a non-responsive host). If you have a non-responsive host, wouldn't that imply that this guest code wouldn't run at all? :) The fact that on a particular transaction the host has not responded within an expected time interval does not necessarily mean that the guest code would not be running. There may be issues on the host side that may be either transient or permanent that may cause problems like this. Keep in mind, HyperV is a type 1 hypervisor that would schedule all VMs including the host and so, guest would get scheduled. Having BUG_ON() in drivers is not a good idea either way. Please remove these in future patches. In situations where there is not a reasonable rollback strategy (for instance in one of the cases, we are granting access to the guest physical pages to the host) we really have only 2 options: 1) Wait until the host responds. This wait could potentially be unbounded and in fact this was the way the code was to begin with. One of the reviewers had suggested that unbounded wait was to be corrected. 2) Wait for a specific period and if the host does not respond within a reasonable period, kill the guest since there is no recovery possible. Killing the guest is a very serious thing, causing all sorts of possible problems with it, right? If there was a reasonable rollback strategy, I would not be killing the guest. I chose option 2, as part of addressing some of the prior review comments. If the consensus now is to go back to option 1, I am fine with that; Unbounded waits aren't ok either, you need some sort of timeout. But, as this is a bit preferable to dieing, I suggest doing this, and comment the heck out of it to explain all of this for anyone who reads it. If I understand you correctly, you would be prefer to have unbounded waiting with comments justifying why we cannot have timeouts. I will roll out a patch once the tree stabilizes. Regards, K. Y thanks, greg k-h ___ 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
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Tuesday, February 15, 2011 9:23 AM Before the end of the week I will submit two patches for this; Remove DPRINT and change it to printk No, use dev_dbg() and friends instead of raw printk() calls. Will do, you caught me just as I was starting the conversion :) While cleaning this up there are a few places in vmbus and channel behavior where it is not in a device context. Are printk's okay in that context? The three drivers network/SCSI and Block of course will use dev_XX family. Hank. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCH]: Staging: hv: Allocate the vmbus irq dynamically
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/vmbus_drv.c | 72 +++-
1 files changed, 56 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 459c707..441ce85 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -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);
--
1.5.5.6
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
