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

[PATCH 1/1] hv: Use only one receive buffer per channel 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 |   84 +++++++++++++++++++++-------------------
 1 files changed, 44 insertions(+), 40 deletions(-)

diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index 53e1e29..e0ecc23 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -38,12 +38,14 @@
 #include "vmbus_api.h"
 #include "utils.h"
 
+static u8 *shut_txf_buf;
+static u8 *time_txf_buf;
+static u8 *hbeat_txf_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,24 +54,23 @@ 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, shut_txf_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 *)&shut_txf_buf[
 			sizeof(struct vmbuspipe_hdr)];
 
 		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
-			prep_negotiate_resp(icmsghdrp, negop, buf);
+			prep_negotiate_resp(icmsghdrp, negop, shut_txf_buf);
 		} else {
-			shutdown_msg = (struct shutdown_msg_data *)&buf[
-				sizeof(struct vmbuspipe_hdr) +
-				sizeof(struct icmsg_hdr)];
+			shutdown_msg =
+				(struct shutdown_msg_data *)&shut_txf_buf[
+					sizeof(struct vmbuspipe_hdr) +
+					sizeof(struct icmsg_hdr)];
 
 			switch (shutdown_msg->flags) {
 			case 0:
@@ -93,13 +94,11 @@ static void shutdown_onchannelcallback(void *context)
 		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
 			| ICMSGHDRFLAG_RESPONSE;
 
-		vmbus_sendpacket(channel, buf,
+		vmbus_sendpacket(channel, shut_txf_buf,
 				       recvlen, requestid,
 				       VmbusPacketTypeDataInBand, 0);
 	}
 
-	kfree(buf);
-
 	if (execute_shutdown == true)
 		orderly_poweroff(false);
 }
@@ -150,28 +149,25 @@ 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, time_txf_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 *)&time_txf_buf[
 				sizeof(struct vmbuspipe_hdr)];
 
 		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
-			prep_negotiate_resp(icmsghdrp, NULL, buf);
+			prep_negotiate_resp(icmsghdrp, NULL, time_txf_buf);
 		} else {
-			timedatap = (struct ictimesync_data *)&buf[
+			timedatap = (struct ictimesync_data *)&time_txf_buf[
 				sizeof(struct vmbuspipe_hdr) +
 				sizeof(struct icmsg_hdr)];
 			adj_guesttime(timedatap->parenttime, timedatap->flags);
@@ -180,12 +176,10 @@ static void timesync_onchannelcallback(void *context)
 		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
 			| ICMSGHDRFLAG_RESPONSE;
 
-		vmbus_sendpacket(channel, buf,
+		vmbus_sendpacket(channel, time_txf_buf,
 				recvlen, requestid,
 				VmbusPacketTypeDataInBand, 0);
 	}
-
-	kfree(buf);
 }
 
 /*
@@ -196,30 +190,28 @@ 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, hbeat_txf_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 *)&hbeat_txf_buf[
 				sizeof(struct vmbuspipe_hdr)];
 
 		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
-			prep_negotiate_resp(icmsghdrp, NULL, buf);
+			prep_negotiate_resp(icmsghdrp, NULL, hbeat_txf_buf);
 		} else {
-			heartbeat_msg = (struct heartbeat_msg_data *)&buf[
-				sizeof(struct vmbuspipe_hdr) +
-				sizeof(struct icmsg_hdr)];
+			heartbeat_msg =
+				(struct heartbeat_msg_data *)&hbeat_txf_buf[
+					sizeof(struct vmbuspipe_hdr) +
+					sizeof(struct icmsg_hdr)];
 
 			DPRINT_DBG(VMBUS, "heartbeat seq = %lld",
 				   heartbeat_msg->seq_num);
@@ -230,12 +222,10 @@ static void heartbeat_onchannelcallback(void *context)
 		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
 			| ICMSGHDRFLAG_RESPONSE;
 
-		vmbus_sendpacket(channel, buf,
+		vmbus_sendpacket(channel, hbeat_txf_buf,
 				       recvlen, requestid,
 				       VmbusPacketTypeDataInBand, 0);
 	}
-
-	kfree(buf);
 }
 
 static const struct pci_device_id __initconst
@@ -268,6 +258,16 @@ static int __init init_hyperv_utils(void)
 	if (!dmi_check_system(hv_utils_dmi_table))
 		return -ENODEV;
 
+	shut_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
+	time_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
+	hbeat_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
+
+	if (!shut_txf_buf || !time_txf_buf || !hbeat_txf_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 +298,10 @@ 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(shut_txf_buf);
+	kfree(time_txf_buf);
+	kfree(hbeat_txf_buf);
 }
 
 module_init(init_hyperv_utils);
-- 
1.6.0.2

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

[Jesper Juhl]

On Mon, 13 Dec 2010, 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 |   84 +++++++++++++++++++++-------------------
>  1 files changed, 44 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
> index 53e1e29..e0ecc23 100644
> --- a/drivers/staging/hv/hv_utils.c
> +++ b/drivers/staging/hv/hv_utils.c
> @@ -38,12 +38,14 @@
>  #include "vmbus_api.h"
>  #include "utils.h"
>  
> +static u8 *shut_txf_buf;
> +static u8 *time_txf_buf;
> +static u8 *hbeat_txf_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,24 +54,23 @@ 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, shut_txf_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 *)&shut_txf_buf[
>  			sizeof(struct vmbuspipe_hdr)];
>  
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> -			prep_negotiate_resp(icmsghdrp, negop, buf);
> +			prep_negotiate_resp(icmsghdrp, negop, shut_txf_buf);
>  		} else {
> -			shutdown_msg = (struct shutdown_msg_data *)&buf[
> -				sizeof(struct vmbuspipe_hdr) +
> -				sizeof(struct icmsg_hdr)];
> +			shutdown_msg =
> +				(struct shutdown_msg_data *)&shut_txf_buf[
> +					sizeof(struct vmbuspipe_hdr) +
> +					sizeof(struct icmsg_hdr)];
>  
>  			switch (shutdown_msg->flags) {
>  			case 0:
> @@ -93,13 +94,11 @@ static void shutdown_onchannelcallback(void *context)
>  		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
>  			| ICMSGHDRFLAG_RESPONSE;
>  
> -		vmbus_sendpacket(channel, buf,
> +		vmbus_sendpacket(channel, shut_txf_buf,
>  				       recvlen, requestid,
>  				       VmbusPacketTypeDataInBand, 0);
>  	}
>  
> -	kfree(buf);
> -
>  	if (execute_shutdown == true)
>  		orderly_poweroff(false);
>  }
> @@ -150,28 +149,25 @@ 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, time_txf_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 *)&time_txf_buf[
>  				sizeof(struct vmbuspipe_hdr)];
>  
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> -			prep_negotiate_resp(icmsghdrp, NULL, buf);
> +			prep_negotiate_resp(icmsghdrp, NULL, time_txf_buf);
>  		} else {
> -			timedatap = (struct ictimesync_data *)&buf[
> +			timedatap = (struct ictimesync_data *)&time_txf_buf[
>  				sizeof(struct vmbuspipe_hdr) +
>  				sizeof(struct icmsg_hdr)];
>  			adj_guesttime(timedatap->parenttime, timedatap->flags);
> @@ -180,12 +176,10 @@ static void timesync_onchannelcallback(void *context)
>  		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
>  			| ICMSGHDRFLAG_RESPONSE;
>  
> -		vmbus_sendpacket(channel, buf,
> +		vmbus_sendpacket(channel, time_txf_buf,
>  				recvlen, requestid,
>  				VmbusPacketTypeDataInBand, 0);
>  	}
> -
> -	kfree(buf);
>  }
>  
>  /*
> @@ -196,30 +190,28 @@ 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, hbeat_txf_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 *)&hbeat_txf_buf[
>  				sizeof(struct vmbuspipe_hdr)];
>  
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> -			prep_negotiate_resp(icmsghdrp, NULL, buf);
> +			prep_negotiate_resp(icmsghdrp, NULL, hbeat_txf_buf);
>  		} else {
> -			heartbeat_msg = (struct heartbeat_msg_data *)&buf[
> -				sizeof(struct vmbuspipe_hdr) +
> -				sizeof(struct icmsg_hdr)];
> +			heartbeat_msg =
> +				(struct heartbeat_msg_data *)&hbeat_txf_buf[
> +					sizeof(struct vmbuspipe_hdr) +
> +					sizeof(struct icmsg_hdr)];
>  
>  			DPRINT_DBG(VMBUS, "heartbeat seq = %lld",
>  				   heartbeat_msg->seq_num);
> @@ -230,12 +222,10 @@ static void heartbeat_onchannelcallback(void *context)
>  		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
>  			| ICMSGHDRFLAG_RESPONSE;
>  
> -		vmbus_sendpacket(channel, buf,
> +		vmbus_sendpacket(channel, hbeat_txf_buf,
>  				       recvlen, requestid,
>  				       VmbusPacketTypeDataInBand, 0);
>  	}
> -
> -	kfree(buf);
>  }
>  
>  static const struct pci_device_id __initconst
> @@ -268,6 +258,16 @@ static int __init init_hyperv_utils(void)
>  	if (!dmi_check_system(hv_utils_dmi_table))
>  		return -ENODEV;
>  
> +	shut_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
> +	time_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
> +	hbeat_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
> +
> +	if (!shut_txf_buf || !time_txf_buf || !hbeat_txf_buf) {
> +		printk(KERN_INFO
> +		       "Unable to allocate memory for receive buffer\n");
> +		return -ENOMEM;

You are leaking memory in the failure path. If for example one or two 
allocations succeed but one or two fail, then you'll leak the two 
successful allocations.

I believe this should be 

     if (!shut_txf_buf || !time_txf_buf || !hbeat_txf_buf) {
             printk(KERN_INFO
                    "Unable to allocate memory for receive buffer\n");
             kfree(hbeat_txf_buf);
             kfree(time_txf_buf);
             kfree(shut_txf_buf);
             return -ENOMEM;
...



> +	}
> +
>  	hv_cb_utils[HV_SHUTDOWN_MSG].channel->onchannel_callback =
>  		&shutdown_onchannelcallback;
>  	hv_cb_utils[HV_SHUTDOWN_MSG].callback = &shutdown_onchannelcallback;
> @@ -298,6 +298,10 @@ 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(shut_txf_buf);
> +	kfree(time_txf_buf);
> +	kfree(hbeat_txf_buf);
>  }
>  
>  module_init(init_hyperv_utils);
> 

-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

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

[Jesper Juhl]

On Mon, 13 Dec 2010, Hank Janssen wrote:

> 
> 
> > -----Original Message-----
> > From: Jesper Juhl [mailto:jj@chaosbits.net]
> > Sent: Monday, December 13, 2010 12:48 PM
> > You are leaking memory in the failure path. If for example one or two
> > allocations succeed but one or two fail, then you'll leak the two successful
> > allocations.
> > 
> > I believe this should be
> > 
> >      if (!shut_txf_buf || !time_txf_buf || !hbeat_txf_buf) {
> >              printk(KERN_INFO
> >                     "Unable to allocate memory for receive buffer\n");
> >              kfree(hbeat_txf_buf);
> >              kfree(time_txf_buf);
> >              kfree(shut_txf_buf);
> >              return -ENOMEM;
> > ...
> > 
>  
> Oops, you are correct. Resubmitting the patch in a few minutes.
> 
Ohh and another little detail; shouldn't this log message be a 
KERN_WARNING level message?
And perhaps the log text should include "HyperV" or something so it's 
clear where it comes from..?

-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

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

[Hank Janssen]

> -----Original Message-----
> From: Jesper Juhl [mailto:jj@chaosbits.net]
> Sent: Monday, December 13, 2010 12:48 PM
> You are leaking memory in the failure path. If for example one or two
> allocations succeed but one or two fail, then you'll leak the two successful
> allocations.
> 
> I believe this should be
> 
>      if (!shut_txf_buf || !time_txf_buf || !hbeat_txf_buf) {
>              printk(KERN_INFO
>                     "Unable to allocate memory for receive buffer\n");
>              kfree(hbeat_txf_buf);
>              kfree(time_txf_buf);
>              kfree(shut_txf_buf);
>              return -ENOMEM;
> ...
> 
 
Oops, you are correct. Resubmitting the patch in a few minutes.

Hank.

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

[Hank Janssen]

> -----Original Message-----
> From: Jesper Juhl [mailto:jj@chaosbits.net]
> Sent: Monday, December 13, 2010 1:06 PM
> On Mon, 13 Dec 2010, Hank Janssen wrote:
> > > ...
> > >
> >
> > Oops, you are correct. Resubmitting the patch in a few minutes.
> >
> Ohh and another little detail; shouldn't this log message be a
> KERN_WARNING level message?
> And perhaps the log text should include "HyperV" or something so it's clear
> where it comes from..?
> 

I will make that part of another set of patches. I need to remove the DPRINT
Completely from the code and replace them all with the correct kprint statements.

Hank.

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

[Ky Srinivasan]

>>> On 12/13/2010 at  3:34 PM, in message
<1292272498-29483-1-git-send-email-hjanssen@microsoft.com>, Hank Janssen
<hjanssen@microsoft.com> 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 |   84 +++++++++++++++++++++-------------------
>  1 files changed, 44 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
> index 53e1e29..e0ecc23 100644
> --- a/drivers/staging/hv/hv_utils.c
> +++ b/drivers/staging/hv/hv_utils.c
> @@ -38,12 +38,14 @@
>  #include "vmbus_api.h"
>  #include "utils.h"
>  
> +static u8 *shut_txf_buf;
> +static u8 *time_txf_buf;
> +static u8 *hbeat_txf_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,24 +54,23 @@ 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, shut_txf_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 *)&shut_txf_buf[
>  			sizeof(struct vmbuspipe_hdr)];
>  
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> -			prep_negotiate_resp(icmsghdrp, negop, buf);
> +			prep_negotiate_resp(icmsghdrp, negop, shut_txf_buf);
>  		} else {
> -			shutdown_msg = (struct shutdown_msg_data *)&buf[
> -				sizeof(struct vmbuspipe_hdr) +
> -				sizeof(struct icmsg_hdr)];
> +			shutdown_msg =
> +				(struct shutdown_msg_data *)&shut_txf_buf[
> +					sizeof(struct vmbuspipe_hdr) +
> +					sizeof(struct icmsg_hdr)];
>  
>  			switch (shutdown_msg->flags) {
>  			case 0:
> @@ -93,13 +94,11 @@ static void shutdown_onchannelcallback(void *context)
>  		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
>  			| ICMSGHDRFLAG_RESPONSE;
>  
> -		vmbus_sendpacket(channel, buf,
> +		vmbus_sendpacket(channel, shut_txf_buf,
>  				       recvlen, requestid,
>  				       VmbusPacketTypeDataInBand, 0);
>  	}
>  
> -	kfree(buf);
> -
>  	if (execute_shutdown == true)
>  		orderly_poweroff(false);
>  }
> @@ -150,28 +149,25 @@ 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, time_txf_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 *)&time_txf_buf[
>  				sizeof(struct vmbuspipe_hdr)];
>  
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> -			prep_negotiate_resp(icmsghdrp, NULL, buf);
> +			prep_negotiate_resp(icmsghdrp, NULL, time_txf_buf);
>  		} else {
> -			timedatap = (struct ictimesync_data *)&buf[
> +			timedatap = (struct ictimesync_data *)&time_txf_buf[
>  				sizeof(struct vmbuspipe_hdr) +
>  				sizeof(struct icmsg_hdr)];
>  			adj_guesttime(timedatap->parenttime, timedatap->flags);
> @@ -180,12 +176,10 @@ static void timesync_onchannelcallback(void *context)
>  		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
>  			| ICMSGHDRFLAG_RESPONSE;
>  
> -		vmbus_sendpacket(channel, buf,
> +		vmbus_sendpacket(channel, time_txf_buf,
>  				recvlen, requestid,
>  				VmbusPacketTypeDataInBand, 0);
>  	}
> -
> -	kfree(buf);
>  }
>  
>  /*
> @@ -196,30 +190,28 @@ 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, hbeat_txf_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 *)&hbeat_txf_buf[
>  				sizeof(struct vmbuspipe_hdr)];
>  
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> -			prep_negotiate_resp(icmsghdrp, NULL, buf);
> +			prep_negotiate_resp(icmsghdrp, NULL, hbeat_txf_buf);
>  		} else {
> -			heartbeat_msg = (struct heartbeat_msg_data *)&buf[
> -				sizeof(struct vmbuspipe_hdr) +
> -				sizeof(struct icmsg_hdr)];
> +			heartbeat_msg =
> +				(struct heartbeat_msg_data *)&hbeat_txf_buf[
> +					sizeof(struct vmbuspipe_hdr) +
> +					sizeof(struct icmsg_hdr)];
>  
>  			DPRINT_DBG(VMBUS, "heartbeat seq = %lld",
>  				   heartbeat_msg->seq_num);
> @@ -230,12 +222,10 @@ static void heartbeat_onchannelcallback(void *context)
>  		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
>  			| ICMSGHDRFLAG_RESPONSE;
>  
> -		vmbus_sendpacket(channel, buf,
> +		vmbus_sendpacket(channel, hbeat_txf_buf,
>  				       recvlen, requestid,
>  				       VmbusPacketTypeDataInBand, 0);
>  	}
> -
> -	kfree(buf);
>  }
>  
>  static const struct pci_device_id __initconst
> @@ -268,6 +258,16 @@ static int __init init_hyperv_utils(void)
>  	if (!dmi_check_system(hv_utils_dmi_table))
>  		return -ENODEV;
>  
> +	shut_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
> +	time_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
> +	hbeat_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
Why are these allocations GFP_ATOMIC. Clearly this is in module loading context and you can afford to sleep. GFP_KERNEL should be fine.

Regards,

K. Y
> +
> +	if (!shut_txf_buf || !time_txf_buf || !hbeat_txf_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 +298,10 @@ 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(shut_txf_buf);
> +	kfree(time_txf_buf);
> +	kfree(hbeat_txf_buf);
>  }
>  
>  module_init(init_hyperv_utils);

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

[Ky Srinivasan]

>>> On 12/13/2010 at  3:34 PM, in message
<1292272498-29483-1-git-send-email-hjanssen@microsoft.com>, Hank Janssen
<hjanssen@microsoft.com> 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 |   84 +++++++++++++++++++++-------------------
>  1 files changed, 44 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
> index 53e1e29..e0ecc23 100644
> --- a/drivers/staging/hv/hv_utils.c
> +++ b/drivers/staging/hv/hv_utils.c
> @@ -38,12 +38,14 @@
>  #include "vmbus_api.h"
>  #include "utils.h"
>  
> +static u8 *shut_txf_buf;
> +static u8 *time_txf_buf;
> +static u8 *hbeat_txf_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,24 +54,23 @@ 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, shut_txf_buf,
> +			 PAGE_SIZE, &recvlen, &requestid);
Even if vmbus_recvpacket were to fail, why don't we just shut the guest down; after all we already know that is the intent of the host.

Regards,

K. Y
>  
>  	if (recvlen > 0) {
>  		DPRINT_DBG(VMBUS, "shutdown packet: len=%d, requestid=%lld",
>  			   recvlen, requestid);
>  
> -		icmsghdrp = (struct icmsg_hdr *)&buf[
> +		icmsghdrp = (struct icmsg_hdr *)&shut_txf_buf[
>  			sizeof(struct vmbuspipe_hdr)];
>  
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> -			prep_negotiate_resp(icmsghdrp, negop, buf);
> +			prep_negotiate_resp(icmsghdrp, negop, shut_txf_buf);
>  		} else {
> -			shutdown_msg = (struct shutdown_msg_data *)&buf[
> -				sizeof(struct vmbuspipe_hdr) +
> -				sizeof(struct icmsg_hdr)];
> +			shutdown_msg =
> +				(struct shutdown_msg_data *)&shut_txf_buf[
> +					sizeof(struct vmbuspipe_hdr) +
> +					sizeof(struct icmsg_hdr)];
>  
>  			switch (shutdown_msg->flags) {
>  			case 0:
> @@ -93,13 +94,11 @@ static void shutdown_onchannelcallback(void *context)
>  		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
>  			| ICMSGHDRFLAG_RESPONSE;
>  
> -		vmbus_sendpacket(channel, buf,
> +		vmbus_sendpacket(channel, shut_txf_buf,
>  				       recvlen, requestid,
>  				       VmbusPacketTypeDataInBand, 0);
>  	}
>  
> -	kfree(buf);
> -
>  	if (execute_shutdown == true)
>  		orderly_poweroff(false);
>  }
> @@ -150,28 +149,25 @@ 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, time_txf_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 *)&time_txf_buf[
>  				sizeof(struct vmbuspipe_hdr)];
>  
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> -			prep_negotiate_resp(icmsghdrp, NULL, buf);
> +			prep_negotiate_resp(icmsghdrp, NULL, time_txf_buf);
>  		} else {
> -			timedatap = (struct ictimesync_data *)&buf[
> +			timedatap = (struct ictimesync_data *)&time_txf_buf[
>  				sizeof(struct vmbuspipe_hdr) +
>  				sizeof(struct icmsg_hdr)];
>  			adj_guesttime(timedatap->parenttime, timedatap->flags);
> @@ -180,12 +176,10 @@ static void timesync_onchannelcallback(void *context)
>  		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
>  			| ICMSGHDRFLAG_RESPONSE;
>  
> -		vmbus_sendpacket(channel, buf,
> +		vmbus_sendpacket(channel, time_txf_buf,
>  				recvlen, requestid,
>  				VmbusPacketTypeDataInBand, 0);
>  	}
> -
> -	kfree(buf);
>  }
>  
>  /*
> @@ -196,30 +190,28 @@ 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, hbeat_txf_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 *)&hbeat_txf_buf[
>  				sizeof(struct vmbuspipe_hdr)];
>  
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> -			prep_negotiate_resp(icmsghdrp, NULL, buf);
> +			prep_negotiate_resp(icmsghdrp, NULL, hbeat_txf_buf);
>  		} else {
> -			heartbeat_msg = (struct heartbeat_msg_data *)&buf[
> -				sizeof(struct vmbuspipe_hdr) +
> -				sizeof(struct icmsg_hdr)];
> +			heartbeat_msg =
> +				(struct heartbeat_msg_data *)&hbeat_txf_buf[
> +					sizeof(struct vmbuspipe_hdr) +
> +					sizeof(struct icmsg_hdr)];
>  
>  			DPRINT_DBG(VMBUS, "heartbeat seq = %lld",
>  				   heartbeat_msg->seq_num);
> @@ -230,12 +222,10 @@ static void heartbeat_onchannelcallback(void *context)
>  		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
>  			| ICMSGHDRFLAG_RESPONSE;
>  
> -		vmbus_sendpacket(channel, buf,
> +		vmbus_sendpacket(channel, hbeat_txf_buf,
>  				       recvlen, requestid,
>  				       VmbusPacketTypeDataInBand, 0);
>  	}
> -
> -	kfree(buf);
>  }
>  
>  static const struct pci_device_id __initconst
> @@ -268,6 +258,16 @@ static int __init init_hyperv_utils(void)
>  	if (!dmi_check_system(hv_utils_dmi_table))
>  		return -ENODEV;
>  
> +	shut_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
> +	time_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
> +	hbeat_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
> +
> +	if (!shut_txf_buf || !time_txf_buf || !hbeat_txf_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 +298,10 @@ 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(shut_txf_buf);
> +	kfree(time_txf_buf);
> +	kfree(hbeat_txf_buf);
>  }
>  
>  module_init(init_hyperv_utils);

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

[Jesper Juhl]

On Mon, 13 Dec 2010, Ky Srinivasan wrote:

> 
> 
> >>> On 12/13/2010 at  3:34 PM, in message
> <1292272498-29483-1-git-send-email-hjanssen@microsoft.com>, Hank Janssen
> <hjanssen@microsoft.com> 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 |   84 +++++++++++++++++++++-------------------
> >  1 files changed, 44 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
> > index 53e1e29..e0ecc23 100644
> > --- a/drivers/staging/hv/hv_utils.c
> > +++ b/drivers/staging/hv/hv_utils.c
> > @@ -38,12 +38,14 @@
> >  #include "vmbus_api.h"
> >  #include "utils.h"
> >  
> > +static u8 *shut_txf_buf;
> > +static u8 *time_txf_buf;
> > +static u8 *hbeat_txf_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,24 +54,23 @@ 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, shut_txf_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 *)&shut_txf_buf[
> >  			sizeof(struct vmbuspipe_hdr)];
> >  
> >  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> > -			prep_negotiate_resp(icmsghdrp, negop, buf);
> > +			prep_negotiate_resp(icmsghdrp, negop, shut_txf_buf);
> >  		} else {
> > -			shutdown_msg = (struct shutdown_msg_data *)&buf[
> > -				sizeof(struct vmbuspipe_hdr) +
> > -				sizeof(struct icmsg_hdr)];
> > +			shutdown_msg =
> > +				(struct shutdown_msg_data *)&shut_txf_buf[
> > +					sizeof(struct vmbuspipe_hdr) +
> > +					sizeof(struct icmsg_hdr)];
> >  
> >  			switch (shutdown_msg->flags) {
> >  			case 0:
> > @@ -93,13 +94,11 @@ static void shutdown_onchannelcallback(void *context)
> >  		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
> >  			| ICMSGHDRFLAG_RESPONSE;
> >  
> > -		vmbus_sendpacket(channel, buf,
> > +		vmbus_sendpacket(channel, shut_txf_buf,
> >  				       recvlen, requestid,
> >  				       VmbusPacketTypeDataInBand, 0);
> >  	}
> >  
> > -	kfree(buf);
> > -
> >  	if (execute_shutdown == true)
> >  		orderly_poweroff(false);
> >  }
> > @@ -150,28 +149,25 @@ 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, time_txf_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 *)&time_txf_buf[
> >  				sizeof(struct vmbuspipe_hdr)];
> >  
> >  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> > -			prep_negotiate_resp(icmsghdrp, NULL, buf);
> > +			prep_negotiate_resp(icmsghdrp, NULL, time_txf_buf);
> >  		} else {
> > -			timedatap = (struct ictimesync_data *)&buf[
> > +			timedatap = (struct ictimesync_data *)&time_txf_buf[
> >  				sizeof(struct vmbuspipe_hdr) +
> >  				sizeof(struct icmsg_hdr)];
> >  			adj_guesttime(timedatap->parenttime, timedatap->flags);
> > @@ -180,12 +176,10 @@ static void timesync_onchannelcallback(void *context)
> >  		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
> >  			| ICMSGHDRFLAG_RESPONSE;
> >  
> > -		vmbus_sendpacket(channel, buf,
> > +		vmbus_sendpacket(channel, time_txf_buf,
> >  				recvlen, requestid,
> >  				VmbusPacketTypeDataInBand, 0);
> >  	}
> > -
> > -	kfree(buf);
> >  }
> >  
> >  /*
> > @@ -196,30 +190,28 @@ 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, hbeat_txf_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 *)&hbeat_txf_buf[
> >  				sizeof(struct vmbuspipe_hdr)];
> >  
> >  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> > -			prep_negotiate_resp(icmsghdrp, NULL, buf);
> > +			prep_negotiate_resp(icmsghdrp, NULL, hbeat_txf_buf);
> >  		} else {
> > -			heartbeat_msg = (struct heartbeat_msg_data *)&buf[
> > -				sizeof(struct vmbuspipe_hdr) +
> > -				sizeof(struct icmsg_hdr)];
> > +			heartbeat_msg =
> > +				(struct heartbeat_msg_data *)&hbeat_txf_buf[
> > +					sizeof(struct vmbuspipe_hdr) +
> > +					sizeof(struct icmsg_hdr)];
> >  
> >  			DPRINT_DBG(VMBUS, "heartbeat seq = %lld",
> >  				   heartbeat_msg->seq_num);
> > @@ -230,12 +222,10 @@ static void heartbeat_onchannelcallback(void *context)
> >  		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
> >  			| ICMSGHDRFLAG_RESPONSE;
> >  
> > -		vmbus_sendpacket(channel, buf,
> > +		vmbus_sendpacket(channel, hbeat_txf_buf,
> >  				       recvlen, requestid,
> >  				       VmbusPacketTypeDataInBand, 0);
> >  	}
> > -
> > -	kfree(buf);
> >  }
> >  
> >  static const struct pci_device_id __initconst
> > @@ -268,6 +258,16 @@ static int __init init_hyperv_utils(void)
> >  	if (!dmi_check_system(hv_utils_dmi_table))
> >  		return -ENODEV;
> >  
> > +	shut_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
> > +	time_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
> > +	hbeat_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
> Why are these allocations GFP_ATOMIC. Clearly this is in module loading context and you can afford to sleep. GFP_KERNEL should be fine.
> 

I actually also noticed this when I did my first review of the patch, but 
I didn't point it out since I thought that "there must be a good reason". 
But now that you point it out and I look at the code once more I can't 
actually think of a "good reason",, so I agree with you completely that 
these should just be GFP_KERNEL.

-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

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

[Hank Janssen]

> -----Original Message-----
> From: Jesper Juhl [mailto:jj@chaosbits.net]
> Sent: Monday, December 13, 2010 3:51 PM
> To: Ky Srinivasan
> > > +	shut_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
> > > +	time_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
> > > +	hbeat_txf_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
> > Why are these allocations GFP_ATOMIC. Clearly this is in module
> loading context and you can afford to sleep. GFP_KERNEL should be fine.
> >
> 
> I actually also noticed this when I did my first review of the patch,
> but
> I didn't point it out since I thought that "there must be a good
> reason".
> But now that you point it out and I look at the code once more I can't
> actually think of a "good reason",, so I agree with you completely that
> these should just be GFP_KERNEL.

I was looking at some of the back code to make sure it is okay, and I see
No reason not make this change either. I will change it and resubmit.

Thanks,

Hank.