[PATCH 1/1] hv: Use only one receive buffer and kmalloc on initialize

[PATCH 1/1] hv: Use only one receive buffer and kmalloc on initialize

[Hank Janssen]

Correct issue with not checking kmalloc return value.
This fix now only uses one receive buffer for all hv_utils 
channels, and will do only one kmalloc on init and will return
with a -ENOMEM if kmalloc fails on initialize.

Thanks to Evgeniy Polyakov <zbr@ioremap.net> for pointing this out.
And thanks to Jesper Juhl <jj@chaosbits.net> and Ky Srinivasan 
<ksrinivasan@novell.com> for suggesting a better implementation of
my original patch.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
Cc:Evgeniy Polyakov <zbr@ioremap.net>
Cc:Jesper Juhl <jj@chaosbits.net>
Cc:Ky Srinivasan <ksrinivasan@novell.com>

---
 drivers/staging/hv/hv_utils.c |   68 +++++++++++++++++++---------------------
 1 files changed, 32 insertions(+), 36 deletions(-)

diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index 53e1e29..4ed4ab8 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -38,12 +38,15 @@
 #include "vmbus_api.h"
 #include "utils.h"
 
+/*
+ * Buffer used to receive packets from Hyper-V
+ */
+static u8 *chan_buf;
 
 static void shutdown_onchannelcallback(void *context)
 {
 	struct vmbus_channel *channel = context;
-	u8 *buf;
-	u32 buflen, recvlen;
+	u32 recvlen;
 	u64 requestid;
 	u8  execute_shutdown = false;
 
@@ -52,22 +55,19 @@ static void shutdown_onchannelcallback(void *context)
 	struct icmsg_hdr *icmsghdrp;
 	struct icmsg_negotiate *negop = NULL;
 
-	buflen = PAGE_SIZE;
-	buf = kmalloc(buflen, GFP_ATOMIC);
-
-	vmbus_recvpacket(channel, buf, buflen, &recvlen, &requestid);
+	vmbus_recvpacket(channel, chan_buf, PAGE_SIZE, &recvlen, &requestid);
 
 	if (recvlen > 0) {
 		DPRINT_DBG(VMBUS, "shutdown packet: len=%d, requestid=%lld",
 			   recvlen, requestid);
 
-		icmsghdrp = (struct icmsg_hdr *)&buf[
+		icmsghdrp = (struct icmsg_hdr *)&chan_buf[
 			sizeof(struct vmbuspipe_hdr)];
 
 		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
-			prep_negotiate_resp(icmsghdrp, negop, buf);
+			prep_negotiate_resp(icmsghdrp, negop, chan_buf);
 		} else {
-			shutdown_msg = (struct shutdown_msg_data *)&buf[
+			shutdown_msg = (struct shutdown_msg_data *)&chan_buf[
 				sizeof(struct vmbuspipe_hdr) +
 				sizeof(struct icmsg_hdr)];
 
@@ -93,13 +93,11 @@ static void shutdown_onchannelcallback(void *context)
 		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
 			| ICMSGHDRFLAG_RESPONSE;
 
-		vmbus_sendpacket(channel, buf,
+		vmbus_sendpacket(channel, chan_buf,
 				       recvlen, requestid,
 				       VmbusPacketTypeDataInBand, 0);
 	}
 
-	kfree(buf);
-
 	if (execute_shutdown == true)
 		orderly_poweroff(false);
 }
@@ -150,28 +148,24 @@ static inline void adj_guesttime(u64 hosttime, u8 flags)
 static void timesync_onchannelcallback(void *context)
 {
 	struct vmbus_channel *channel = context;
-	u8 *buf;
-	u32 buflen, recvlen;
+	u32 recvlen;
 	u64 requestid;
 	struct icmsg_hdr *icmsghdrp;
 	struct ictimesync_data *timedatap;
 
-	buflen = PAGE_SIZE;
-	buf = kmalloc(buflen, GFP_ATOMIC);
-
-	vmbus_recvpacket(channel, buf, buflen, &recvlen, &requestid);
+	vmbus_recvpacket(channel, chan_buf, PAGE_SIZE, &recvlen, &requestid);
 
 	if (recvlen > 0) {
 		DPRINT_DBG(VMBUS, "timesync packet: recvlen=%d, requestid=%lld",
 			recvlen, requestid);
 
-		icmsghdrp = (struct icmsg_hdr *)&buf[
+		icmsghdrp = (struct icmsg_hdr *)&chan_buf[
 				sizeof(struct vmbuspipe_hdr)];
 
 		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
-			prep_negotiate_resp(icmsghdrp, NULL, buf);
+			prep_negotiate_resp(icmsghdrp, NULL, chan_buf);
 		} else {
-			timedatap = (struct ictimesync_data *)&buf[
+			timedatap = (struct ictimesync_data *)&chan_buf[
 				sizeof(struct vmbuspipe_hdr) +
 				sizeof(struct icmsg_hdr)];
 			adj_guesttime(timedatap->parenttime, timedatap->flags);
@@ -180,12 +174,10 @@ static void timesync_onchannelcallback(void *context)
 		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
 			| ICMSGHDRFLAG_RESPONSE;
 
-		vmbus_sendpacket(channel, buf,
+		vmbus_sendpacket(channel, chan_buf,
 				recvlen, requestid,
 				VmbusPacketTypeDataInBand, 0);
 	}
-
-	kfree(buf);
 }
 
 /*
@@ -196,28 +188,24 @@ static void timesync_onchannelcallback(void *context)
 static void heartbeat_onchannelcallback(void *context)
 {
 	struct vmbus_channel *channel = context;
-	u8 *buf;
-	u32 buflen, recvlen;
+	u32 recvlen;
 	u64 requestid;
 	struct icmsg_hdr *icmsghdrp;
 	struct heartbeat_msg_data *heartbeat_msg;
 
-	buflen = PAGE_SIZE;
-	buf = kmalloc(buflen, GFP_ATOMIC);
-
-	vmbus_recvpacket(channel, buf, buflen, &recvlen, &requestid);
+	vmbus_recvpacket(channel, chan_buf, PAGE_SIZE, &recvlen, &requestid);
 
 	if (recvlen > 0) {
 		DPRINT_DBG(VMBUS, "heartbeat packet: len=%d, requestid=%lld",
 			   recvlen, requestid);
 
-		icmsghdrp = (struct icmsg_hdr *)&buf[
+		icmsghdrp = (struct icmsg_hdr *)&chan_buf[
 				sizeof(struct vmbuspipe_hdr)];
 
 		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
-			prep_negotiate_resp(icmsghdrp, NULL, buf);
+			prep_negotiate_resp(icmsghdrp, NULL, chan_buf);
 		} else {
-			heartbeat_msg = (struct heartbeat_msg_data *)&buf[
+			heartbeat_msg = (struct heartbeat_msg_data *)&chan_buf[
 				sizeof(struct vmbuspipe_hdr) +
 				sizeof(struct icmsg_hdr)];
 
@@ -230,12 +218,10 @@ static void heartbeat_onchannelcallback(void *context)
 		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
 			| ICMSGHDRFLAG_RESPONSE;
 
-		vmbus_sendpacket(channel, buf,
+		vmbus_sendpacket(channel, chan_buf,
 				       recvlen, requestid,
 				       VmbusPacketTypeDataInBand, 0);
 	}
-
-	kfree(buf);
 }
 
 static const struct pci_device_id __initconst
@@ -268,6 +254,14 @@ static int __init init_hyperv_utils(void)
 	if (!dmi_check_system(hv_utils_dmi_table))
 		return -ENODEV;
 
+	chan_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
+
+	if (!chan_buf) {
+		printk(KERN_INFO
+		       "Unable to allocate memory for receive buffer\n");
+		return -ENOMEM;
+	}
+
 	hv_cb_utils[HV_SHUTDOWN_MSG].channel->onchannel_callback =
 		&shutdown_onchannelcallback;
 	hv_cb_utils[HV_SHUTDOWN_MSG].callback = &shutdown_onchannelcallback;
@@ -298,6 +292,8 @@ static void exit_hyperv_utils(void)
 	hv_cb_utils[HV_HEARTBEAT_MSG].channel->onchannel_callback =
 		&chn_cb_negotiate;
 	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &chn_cb_negotiate;
+
+	kfree(chan_buf);
 }
 
 module_init(init_hyperv_utils);
-- 
1.6.0.2

Re: [PATCH 1/1] hv: Use only one receive buffer and kmalloc on initialize

[Greg KH]

On Mon, Dec 13, 2010 at 09:45:50AM -0800, Hank Janssen wrote:
> Correct issue with not checking kmalloc return value.
> This fix now only uses one receive buffer for all hv_utils 
> channels, and will do only one kmalloc on init and will return
> with a -ENOMEM if kmalloc fails on initialize.
> 
> Thanks to Evgeniy Polyakov <zbr@ioremap.net> for pointing this out.
> And thanks to Jesper Juhl <jj@chaosbits.net> and Ky Srinivasan 
> <ksrinivasan@novell.com> for suggesting a better implementation of
> my original patch.
> 
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> Cc:Evgeniy Polyakov <zbr@ioremap.net>
> Cc:Jesper Juhl <jj@chaosbits.net>
> Cc:Ky Srinivasan <ksrinivasan@novell.com>
> 
> ---
>  drivers/staging/hv/hv_utils.c |   68 +++++++++++++++++++---------------------
>  1 files changed, 32 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
> index 53e1e29..4ed4ab8 100644
> --- a/drivers/staging/hv/hv_utils.c
> +++ b/drivers/staging/hv/hv_utils.c
> @@ -38,12 +38,15 @@
>  #include "vmbus_api.h"
>  #include "utils.h"
>  
> +/*
> + * Buffer used to receive packets from Hyper-V
> + */
> +static u8 *chan_buf;

One buffer is nicer, yes, but what's controlling access to this buffer?
You use it in multiple functions, and what's to say those functions
can't be called at the same time on different cpus?  So, shouldn't you
either have some locking for access to the buffer, or have a
per-function buffer instead?

And if you have a per-function buffer, again, you might need to control
access to it as the functions could be called multiple times at the same
time, right?

thanks,

greg k-h

RE: [PATCH 1/1] hv: Use only one receive buffer and kmalloc on initialize

[Hank Janssen]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Monday, December 13, 2010 10:35 AM
> > ---
> >  drivers/staging/hv/hv_utils.c |   68 +++++++++++++++++++------------------
> ---
> >  1 files changed, 32 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/staging/hv/hv_utils.c
> > b/drivers/staging/hv/hv_utils.c index 53e1e29..4ed4ab8 100644
> > --- a/drivers/staging/hv/hv_utils.c
> > +++ b/drivers/staging/hv/hv_utils.c
> > @@ -38,12 +38,15 @@
> >  #include "vmbus_api.h"
> >  #include "utils.h"
> >
> > +/*
> > + * Buffer used to receive packets from Hyper-V  */ static u8
> > +*chan_buf;
> 
> One buffer is nicer, yes, but what's controlling access to this buffer?
> You use it in multiple functions, and what's to say those functions can't be
> called at the same time on different cpus?  So, shouldn't you either have
> some locking for access to the buffer, or have a per-function buffer instead?
> 
> And if you have a per-function buffer, again, you might need to control
> access to it as the functions could be called multiple times at the same time,
> right?
> 

The current versions of Hyper-V support interrupt handling on CPU0 only.
I can make multiple buffers per channel, but because of Hyper-V implementation
It does not really make a difference.

Hank.

Re: [PATCH 1/1] hv: Use only one receive buffer and kmalloc on initialize

[Greg KH]

On Mon, Dec 13, 2010 at 07:31:42PM +0000, Hank Janssen wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Monday, December 13, 2010 10:35 AM
> > > ---
> > >  drivers/staging/hv/hv_utils.c |   68 +++++++++++++++++++------------------
> > ---
> > >  1 files changed, 32 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/drivers/staging/hv/hv_utils.c
> > > b/drivers/staging/hv/hv_utils.c index 53e1e29..4ed4ab8 100644
> > > --- a/drivers/staging/hv/hv_utils.c
> > > +++ b/drivers/staging/hv/hv_utils.c
> > > @@ -38,12 +38,15 @@
> > >  #include "vmbus_api.h"
> > >  #include "utils.h"
> > >
> > > +/*
> > > + * Buffer used to receive packets from Hyper-V  */ static u8
> > > +*chan_buf;
> > 
> > One buffer is nicer, yes, but what's controlling access to this buffer?
> > You use it in multiple functions, and what's to say those functions can't be
> > called at the same time on different cpus?  So, shouldn't you either have
> > some locking for access to the buffer, or have a per-function buffer instead?
> > 
> > And if you have a per-function buffer, again, you might need to control
> > access to it as the functions could be called multiple times at the same time,
> > right?
> > 
> 
> The current versions of Hyper-V support interrupt handling on CPU0 only.
> I can make multiple buffers per channel, but because of Hyper-V implementation
> It does not really make a difference.

Then put a big fat note in there saying this, and that it will have to
change if the hyperv channel ever changes.

Hm, how will you handle things if the hyperv core changes and an old
kernel is running this code where things were "assumed" about the
reentrancy happening here?

thanks,

greg k-h

RE: [PATCH 1/1] hv: Use only one receive buffer and kmalloc on initialize

[Hank Janssen]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Monday, December 13, 2010 11:41 AM
> > The current versions of Hyper-V support interrupt handling on CPU0 only.
> > I can make multiple buffers per channel, but because of Hyper-V
> > implementation It does not really make a difference.
> 
> Then put a big fat note in there saying this, and that it will have to change if
> the hyperv channel ever changes.
> 
> Hm, how will you handle things if the hyperv core changes and an old kernel
> is running this code where things were "assumed" about the reentrancy
> happening here?

I will re-roll this patch to have a buffer per channel. It is a more elegant design
Even though Hyper-V behavior is not changing in Win8 for this.

Hank.

Re: [PATCH 1/1] hv: Use only one receive buffer and kmalloc on initialize

[Greg KH]

On Mon, Dec 13, 2010 at 08:06:20PM +0000, Hank Janssen wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Monday, December 13, 2010 11:41 AM
> > > The current versions of Hyper-V support interrupt handling on CPU0 only.
> > > I can make multiple buffers per channel, but because of Hyper-V
> > > implementation It does not really make a difference.
> > 
> > Then put a big fat note in there saying this, and that it will have to change if
> > the hyperv channel ever changes.
> > 
> > Hm, how will you handle things if the hyperv core changes and an old kernel
> > is running this code where things were "assumed" about the reentrancy
> > happening here?
> 
> I will re-roll this patch to have a buffer per channel. It is a more elegant design
> Even though Hyper-V behavior is not changing in Win8 for this.

Win8, fine, but what about Win9?  :)

thanks for changing it.

greg k-h