[PATCH 0/8] Staging: hv: vmbus: Driver cleanup

[PATCH 0/8] Staging: hv: vmbus: Driver cleanup

[K. Y. Srinivasan]

Further cleanup of the vmbus driver:

	1) Cleanup the interrupt handler by inlining some code. 

	2) Ensure message handling is performed on the same CPU that
	   takes the vmbus interrupt. 

	3) Check for events before messages (from the host).

	4) Disable auto eoi for the vmbus interrupt since Linux will eoi the
	   interrupt anyway. 

	5) Some general cleanup.
	  

Regards,

K. Y 

[PATCH 1/8] Staging: hv: vmbus: Get rid of vmbus_on_isr() by inlining the code

[K. Y. Srinivasan]

Get rid of vmbus_on_isr() by inlining this code in the
real interrupt handler.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/staging/hv/vmbus_drv.c |   41 +++++++++++----------------------------
 1 files changed, 12 insertions(+), 29 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index b382829..bb25c5b 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -411,53 +411,36 @@ static void vmbus_on_msg_dpc(unsigned long data)
 	}
 }
 
-/*
- * vmbus_on_isr - ISR routine
- */
-static int vmbus_on_isr(void)
+static irqreturn_t vmbus_isr(int irq, void *dev_id)
 {
-	int ret = 0;
 	int cpu = smp_processor_id();
 	void *page_addr;
 	struct hv_message *msg;
 	union hv_synic_event_flags *event;
+	bool handled = false;
 
 	page_addr = hv_context.synic_message_page[cpu];
 	msg = (struct hv_message *)page_addr + VMBUS_MESSAGE_SINT;
 
 	/* Check if there are actual msgs to be process */
-	if (msg->header.message_type != HVMSG_NONE)
-		ret |= 0x1;
+	if (msg->header.message_type != HVMSG_NONE) {
+		handled = true;
+		tasklet_schedule(&msg_dpc);
+	}
 
 	page_addr = hv_context.synic_event_page[cpu];
 	event = (union hv_synic_event_flags *)page_addr + VMBUS_MESSAGE_SINT;
 
 	/* Since we are a child, we only need to check bit 0 */
-	if (sync_test_and_clear_bit(0, (unsigned long *) &event->flags32[0]))
-		ret |= 0x2;
-
-	return ret;
-}
-
-
-static irqreturn_t vmbus_isr(int irq, void *dev_id)
-{
-	int ret;
-
-	ret = vmbus_on_isr();
-
-	/* Schedules a dpc if necessary */
-	if (ret > 0) {
-		if (test_bit(0, (unsigned long *)&ret))
-			tasklet_schedule(&msg_dpc);
-
-		if (test_bit(1, (unsigned long *)&ret))
-			tasklet_schedule(&event_dpc);
+	if (sync_test_and_clear_bit(0, (unsigned long *) &event->flags32[0])) {
+		handled = true;
+		tasklet_schedule(&event_dpc);
+	}
 
+	if (handled)
 		return IRQ_HANDLED;
-	} else {
+	else
 		return IRQ_NONE;
-	}
 }
 
 /*
-- 
1.7.4.1

[PATCH 2/8] Staging: hv: vmbus: Invoke vmbus_on_msg_dpc() directly

[K. Y. Srinivasan]

The message processing function needs to execute on the same CPU where
the interrupt was taken. tasklets cannot gurantee this; so, invoke the 
function directly.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/staging/hv/vmbus_drv.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index bb25c5b..1d4e878 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -39,7 +39,6 @@
 
 static struct acpi_device  *hv_acpi_dev;
 
-static struct tasklet_struct msg_dpc;
 static struct tasklet_struct event_dpc;
 
 unsigned int vmbus_loglevel = (ALL_MODULES << 16 | INFO_LVL);
@@ -425,7 +424,7 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
 	/* Check if there are actual msgs to be process */
 	if (msg->header.message_type != HVMSG_NONE) {
 		handled = true;
-		tasklet_schedule(&msg_dpc);
+		vmbus_on_msg_dpc(0);
 	}
 
 	page_addr = hv_context.synic_event_page[cpu];
@@ -465,7 +464,6 @@ static int vmbus_bus_init(int irq)
 	}
 
 	/* Initialize the bus context */
-	tasklet_init(&msg_dpc, vmbus_on_msg_dpc, 0);
 	tasklet_init(&event_dpc, vmbus_on_event, 0);
 
 	/* Now, register the bus  with LDM */
-- 
1.7.4.1

[PATCH 3/8] Staging: hv: vmbus: Check for events before messages

[K. Y. Srinivasan]

Hyper-V requires that events be processed before processing messages. Make the
necessary code adjustments.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/staging/hv/vmbus_drv.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 1d4e878..536466b 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -418,15 +418,6 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
 	union hv_synic_event_flags *event;
 	bool handled = false;
 
-	page_addr = hv_context.synic_message_page[cpu];
-	msg = (struct hv_message *)page_addr + VMBUS_MESSAGE_SINT;
-
-	/* Check if there are actual msgs to be process */
-	if (msg->header.message_type != HVMSG_NONE) {
-		handled = true;
-		vmbus_on_msg_dpc(0);
-	}
-
 	page_addr = hv_context.synic_event_page[cpu];
 	event = (union hv_synic_event_flags *)page_addr + VMBUS_MESSAGE_SINT;
 
@@ -436,6 +427,15 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
 		tasklet_schedule(&event_dpc);
 	}
 
+	page_addr = hv_context.synic_message_page[cpu];
+	msg = (struct hv_message *)page_addr + VMBUS_MESSAGE_SINT;
+
+	/* Check if there are actual msgs to be process */
+	if (msg->header.message_type != HVMSG_NONE) {
+		handled = true;
+		vmbus_on_msg_dpc(0);
+	}
+
 	if (handled)
 		return IRQ_HANDLED;
 	else
-- 
1.7.4.1

[PATCH 4/8] Staging: hv: vmbus: Do not enable auto eoi

[K. Y. Srinivasan]

The standard interrupt handling in Linux issues an eoi when the
interrupt handling is done. So, setup the vmbus interrupt without
enabling auto eoi.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/staging/hv/hv.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/hv/hv.c b/drivers/staging/hv/hv.c
index 14e6315..3b0f9f4 100644
--- a/drivers/staging/hv/hv.c
+++ b/drivers/staging/hv/hv.c
@@ -370,7 +370,7 @@ void hv_synic_init(void *irqarg)
 	shared_sint.as_uint64 = 0;
 	shared_sint.vector = irq_vector; /* HV_SHARED_SINT_IDT_VECTOR + 0x20; */
 	shared_sint.masked = false;
-	shared_sint.auto_eoi = true;
+	shared_sint.auto_eoi = false;
 
 	wrmsrl(HV_X64_MSR_SINT0 + VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
 
-- 
1.7.4.1

[PATCH 5/8] Staging: hv: vmbus: Fixup indentation in vmbus_acpi_add()

[K. Y. Srinivasan]

Fixup some  indentation issues in vmbus_acpi_add().

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/staging/hv/vmbus_drv.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 536466b..275c4ef 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -654,9 +654,8 @@ static int vmbus_acpi_add(struct acpi_device *device)
 
 	hv_acpi_dev = device;
 
-	result =
-	acpi_walk_resources(device->handle, METHOD_NAME__CRS,
-			vmbus_walk_resources, &irq);
+	result = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
+					vmbus_walk_resources, &irq);
 
 	if (ACPI_FAILURE(result)) {
 		complete(&probe_event);
-- 
1.7.4.1

[PATCH 6/8] Staging: hv: vmbus: Get rid of some dated/redundant comments

[K. Y. Srinivasan]

 Get rid of some dated/redundant comments.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/staging/hv/vmbus_drv.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 275c4ef..f20ab9a 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -364,9 +364,6 @@ static void vmbus_onmessage_work(struct work_struct *work)
 	kfree(ctx);
 }
 
-/*
- * vmbus_on_msg_dpc - DPC routine to handle messages from the hypervisior
- */
 static void vmbus_on_msg_dpc(unsigned long data)
 {
 	int cpu = smp_processor_id();
@@ -463,15 +460,12 @@ static int vmbus_bus_init(int irq)
 		return ret;
 	}
 
-	/* Initialize the bus context */
 	tasklet_init(&event_dpc, vmbus_on_event, 0);
 
-	/* Now, register the bus  with LDM */
 	ret = bus_register(&hv_bus);
 	if (ret)
 		return ret;
 
-	/* Get the interrupt resource */
 	ret = request_irq(irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
 			driver_name, hv_acpi_dev);
 
@@ -522,7 +516,6 @@ int vmbus_child_driver_register(struct hv_driver *hv_drv)
 
 	pr_info("child driver registering - name %s\n", drv->name);
 
-	/* The child driver on this vmbus */
 	drv->bus = &hv_bus;
 
 	ret = driver_register(drv);
@@ -563,7 +556,6 @@ struct hv_device *vmbus_child_device_create(uuid_le *type,
 {
 	struct hv_device *child_device_obj;
 
-	/* Allocate the new child device */
 	child_device_obj = kzalloc(sizeof(struct hv_device), GFP_KERNEL);
 	if (!child_device_obj) {
 		pr_err("Unable to allocate device object for child device\n");
@@ -589,12 +581,10 @@ int vmbus_child_device_register(struct hv_device *child_device_obj)
 
 	static atomic_t device_num = ATOMIC_INIT(0);
 
-	/* Set the device name. Otherwise, device_register() will fail. */
 	dev_set_name(&child_device_obj->device, "vmbus_0_%d",
 		     atomic_inc_return(&device_num));
 
-	/* The new device belongs to this bus */
-	child_device_obj->device.bus = &hv_bus; /* device->dev.bus; */
+	child_device_obj->device.bus = &hv_bus;
 	child_device_obj->device.parent = &hv_acpi_dev->dev;
 	child_device_obj->device.release = vmbus_device_release;
 
-- 
1.7.4.1

[PATCH 7/8] Staging: hv: vmbus: Fix a bug in error handling in vmbus_bus_init()

[K. Y. Srinivasan]

Fix a bug in error handling in vmbus_bus_init().

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/staging/hv/vmbus_drv.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index f20ab9a..0b0bff3 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -463,8 +463,10 @@ static int vmbus_bus_init(int irq)
 	tasklet_init(&event_dpc, vmbus_on_event, 0);
 
 	ret = bus_register(&hv_bus);
-	if (ret)
+	if (ret) {
+		hv_cleanup();
 		return ret;
+	}
 
 	ret = request_irq(irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
 			driver_name, hv_acpi_dev);
@@ -473,6 +475,7 @@ static int vmbus_bus_init(int irq)
 		pr_err("Unable to request IRQ %d\n",
 			   irq);
 
+		hv_cleanup();
 		bus_unregister(&hv_bus);
 
 		return ret;
@@ -487,6 +490,7 @@ static int vmbus_bus_init(int irq)
 	on_each_cpu(hv_synic_init, (void *)&vector, 1);
 	ret = vmbus_connect();
 	if (ret) {
+		hv_cleanup();
 		free_irq(irq, hv_acpi_dev);
 		bus_unregister(&hv_bus);
 		return ret;
-- 
1.7.4.1

[PATCH 8/8] Staging: hv: vmbus: Get rid of an unnecessary check in vmbus_connect()

[K. Y. Srinivasan]

Get rid of an unnecessary check in vmbus_connect(). Since vmbus_connect is
only called once, the check being removed is redundant.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/staging/hv/connection.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/hv/connection.c b/drivers/staging/hv/connection.c
index 66b7c4b..9e99c04 100644
--- a/drivers/staging/hv/connection.c
+++ b/drivers/staging/hv/connection.c
@@ -50,10 +50,6 @@ int vmbus_connect(void)
 	struct vmbus_channel_initiate_contact *msg;
 	unsigned long flags;
 
-	/* Make sure we are not connecting or connected */
-	if (vmbus_connection.conn_state != DISCONNECTED)
-		return -EISCONN;
-
 	/* Initialize the vmbus connection */
 	vmbus_connection.conn_state = CONNECTING;
 	vmbus_connection.work_queue = create_workqueue("hv_vmbus_con");
-- 
1.7.4.1

Re: [PATCH 2/8] Staging: hv: vmbus: Invoke vmbus_on_msg_dpc() directly

[Sasha Levin]

On Mon, 2011-08-15 at 15:12 -0700, K. Y. Srinivasan wrote:
> The message processing function needs to execute on the same CPU where
> the interrupt was taken. tasklets cannot gurantee this; so, invoke the 
> function directly.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---

tasklets are guaranteed to run on the same CPU as the function that
scheduled them.

Unless I'm missing something?

>  drivers/staging/hv/vmbus_drv.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> index bb25c5b..1d4e878 100644
> --- a/drivers/staging/hv/vmbus_drv.c
> +++ b/drivers/staging/hv/vmbus_drv.c
> @@ -39,7 +39,6 @@
>  
>  static struct acpi_device  *hv_acpi_dev;
>  
> -static struct tasklet_struct msg_dpc;
>  static struct tasklet_struct event_dpc;
>  
>  unsigned int vmbus_loglevel = (ALL_MODULES << 16 | INFO_LVL);
> @@ -425,7 +424,7 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
>  	/* Check if there are actual msgs to be process */
>  	if (msg->header.message_type != HVMSG_NONE) {
>  		handled = true;
> -		tasklet_schedule(&msg_dpc);
> +		vmbus_on_msg_dpc(0);
>  	}
>  
>  	page_addr = hv_context.synic_event_page[cpu];
> @@ -465,7 +464,6 @@ static int vmbus_bus_init(int irq)
>  	}
>  
>  	/* Initialize the bus context */
> -	tasklet_init(&msg_dpc, vmbus_on_msg_dpc, 0);
>  	tasklet_init(&event_dpc, vmbus_on_event, 0);
>  
>  	/* Now, register the bus  with LDM */

-- 

Sasha.

Re: [PATCH 7/8] Staging: hv: vmbus: Fix a bug in error handling in vmbus_bus_init()

[Sasha Levin]

On Mon, 2011-08-15 at 15:12 -0700, K. Y. Srinivasan wrote:
> Fix a bug in error handling in vmbus_bus_init().
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---

vmbus_bus_init() has a 'cleanup' label which is used just for returning,
while the cleanup logic is replicated all over the code.

Can't we just move all of it to the cleanup label?

>  drivers/staging/hv/vmbus_drv.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> index f20ab9a..0b0bff3 100644
> --- a/drivers/staging/hv/vmbus_drv.c
> +++ b/drivers/staging/hv/vmbus_drv.c
> @@ -463,8 +463,10 @@ static int vmbus_bus_init(int irq)
>  	tasklet_init(&event_dpc, vmbus_on_event, 0);
>  
>  	ret = bus_register(&hv_bus);
> -	if (ret)
> +	if (ret) {
> +		hv_cleanup();
>  		return ret;
> +	}
>  
>  	ret = request_irq(irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
>  			driver_name, hv_acpi_dev);
> @@ -473,6 +475,7 @@ static int vmbus_bus_init(int irq)
>  		pr_err("Unable to request IRQ %d\n",
>  			   irq);
>  
> +		hv_cleanup();
>  		bus_unregister(&hv_bus);
>  
>  		return ret;
> @@ -487,6 +490,7 @@ static int vmbus_bus_init(int irq)
>  	on_each_cpu(hv_synic_init, (void *)&vector, 1);
>  	ret = vmbus_connect();
>  	if (ret) {
> +		hv_cleanup();
>  		free_irq(irq, hv_acpi_dev);
>  		bus_unregister(&hv_bus);
>  		return ret;

-- 

Sasha.

RE: [PATCH 2/8] Staging: hv: vmbus: Invoke vmbus_on_msg_dpc() directly

[KY Srinivasan]

> -----Original Message-----
> From: Sasha Levin [mailto:levinsasha928@gmail.com]
> Sent: Wednesday, August 17, 2011 8:48 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> Subject: Re: [PATCH 2/8] Staging: hv: vmbus: Invoke vmbus_on_msg_dpc()
> directly
> 
> On Mon, 2011-08-15 at 15:12 -0700, K. Y. Srinivasan wrote:
> > The message processing function needs to execute on the same CPU where
> > the interrupt was taken. tasklets cannot gurantee this; so, invoke the
> > function directly.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> 
> tasklets are guaranteed to run on the same CPU as the function that
> scheduled them.
> 
> Unless I'm missing something?

I too was under this impression until I stumbled upon this comment in
include/Linux/interrupt.h where I see that there is no guarantee that
tasklet would run on the same CPU that it was scheduled on
(look at the first listed property):

/* Tasklets --- multithreaded analogue of BHs.

   Main feature differing them of generic softirqs: tasklet
   is running only on one CPU simultaneously.

   Main feature differing them of BHs: different tasklets
   may be run simultaneously on different CPUs.

   Properties:
   * If tasklet_schedule() is called, then tasklet is guaranteed
     to be executed on some cpu at least once after this.
.
.
*/

Given this comment here, I felt that safest thing to do would be to just
not use the tasklet in this scenario.

Regards,

K. Y

RE: [PATCH 7/8] Staging: hv: vmbus: Fix a bug in error handling in vmbus_bus_init()

[KY Srinivasan]

> -----Original Message-----
> From: Sasha Levin [mailto:levinsasha928@gmail.com]
> Sent: Wednesday, August 17, 2011 9:26 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> Subject: Re: [PATCH 7/8] Staging: hv: vmbus: Fix a bug in error handling in
> vmbus_bus_init()
> 
> On Mon, 2011-08-15 at 15:12 -0700, K. Y. Srinivasan wrote:
> > Fix a bug in error handling in vmbus_bus_init().
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> 
> vmbus_bus_init() has a 'cleanup' label which is used just for returning,
> while the cleanup logic is replicated all over the code.
> 
> Can't we just move all of it to the cleanup label?

These are in  two different functions - vmbus_bus_init(),  hv_acpi_init().
 hv_cleanup() rolls back state established in hv_init(). I could look at 
cleaning up the error handling code.

Regards,

K. Y


> 
> >  drivers/staging/hv/vmbus_drv.c |    6 +++++-
> >  1 files changed, 5 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> > index f20ab9a..0b0bff3 100644
> > --- a/drivers/staging/hv/vmbus_drv.c
> > +++ b/drivers/staging/hv/vmbus_drv.c
> > @@ -463,8 +463,10 @@ static int vmbus_bus_init(int irq)
> >  	tasklet_init(&event_dpc, vmbus_on_event, 0);
> >
> >  	ret = bus_register(&hv_bus);
> > -	if (ret)
> > +	if (ret) {
> > +		hv_cleanup();
> >  		return ret;
> > +	}
> >
> >  	ret = request_irq(irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
> >  			driver_name, hv_acpi_dev);
> > @@ -473,6 +475,7 @@ static int vmbus_bus_init(int irq)
> >  		pr_err("Unable to request IRQ %d\n",
> >  			   irq);
> >
> > +		hv_cleanup();
> >  		bus_unregister(&hv_bus);
> >
> >  		return ret;
> > @@ -487,6 +490,7 @@ static int vmbus_bus_init(int irq)
> >  	on_each_cpu(hv_synic_init, (void *)&vector, 1);
> >  	ret = vmbus_connect();
> >  	if (ret) {
> > +		hv_cleanup();
> >  		free_irq(irq, hv_acpi_dev);
> >  		bus_unregister(&hv_bus);
> >  		return ret;
> 
> --
> 
> Sasha.
> 

RE: [PATCH 2/8] Staging: hv: vmbus: Invoke vmbus_on_msg_dpc() directly

[Sasha Levin]

On Wed, 2011-08-17 at 18:43 +0000, KY Srinivasan wrote:
> 
> > -----Original Message-----
> > From: Sasha Levin [mailto:levinsasha928@gmail.com]
> > Sent: Wednesday, August 17, 2011 8:48 AM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> > Subject: Re: [PATCH 2/8] Staging: hv: vmbus: Invoke vmbus_on_msg_dpc()
> > directly
> > 
> > On Mon, 2011-08-15 at 15:12 -0700, K. Y. Srinivasan wrote:
> > > The message processing function needs to execute on the same CPU where
> > > the interrupt was taken. tasklets cannot gurantee this; so, invoke the
> > > function directly.
> > >
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > ---
> > 
> > tasklets are guaranteed to run on the same CPU as the function that
> > scheduled them.
> > 
> > Unless I'm missing something?
> 
> I too was under this impression until I stumbled upon this comment in
> include/Linux/interrupt.h where I see that there is no guarantee that
> tasklet would run on the same CPU that it was scheduled on
> (look at the first listed property):
> 
> /* Tasklets --- multithreaded analogue of BHs.
> 
>    Main feature differing them of generic softirqs: tasklet
>    is running only on one CPU simultaneously.
> 
>    Main feature differing them of BHs: different tasklets
>    may be run simultaneously on different CPUs.
> 
>    Properties:
>    * If tasklet_schedule() is called, then tasklet is guaranteed
>      to be executed on some cpu at least once after this.
> .
> .
> */
> 
> Given this comment here, I felt that safest thing to do would be to just
> not use the tasklet in this scenario.

Hm..

Looking at __tasklet_schedule() it should be guaranteed to run on the
same CPU, but as you said - the comment above is actually saying the
opposite.

The relevant documentation in the 'Linux Device Drivers' book also
states that it is guaranteed, so maybe we can get a confirmation from
Greg that this is still true and update the comment instead?

Not using bottom halves and just calling the dpc handler would mean that
you block interrupts for much longer than you need to.

-- 

Sasha.

Re: [PATCH 0/8] Staging: hv: vmbus: Driver cleanup

[Greg KH]

On Mon, Aug 15, 2011 at 03:10:10PM -0700, K. Y. Srinivasan wrote:
> Further cleanup of the vmbus driver:
> 
> 	1) Cleanup the interrupt handler by inlining some code. 
> 
> 	2) Ensure message handling is performed on the same CPU that
> 	   takes the vmbus interrupt. 
> 
> 	3) Check for events before messages (from the host).
> 
> 	4) Disable auto eoi for the vmbus interrupt since Linux will eoi the
> 	   interrupt anyway. 
> 
> 	5) Some general cleanup.

As I didn't apply your other changes, care to respin all of your
outstanding hv patches against the next linux-next release and resend
them?  I tried picking some of the other patches, but they started not
applying easily so it would be easier for everyone to just resend them
after you fix them up.

thanks,

greg k-h