thr3ads.net - Linux Virtualization - [PATCH 1/1] hv: Use only one receive buffer and kmalloc on initialize [Dec 2010]

If this information is useful, please help other people find it:
Share via:

Hank Janssen

2010-Dec-13 17:45 UTC

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

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 at ioremap.net> for pointing this out.
And thanks to Jesper Juhl <jj at chaosbits.net> and Ky Srinivasan 
<ksrinivasan at novell.com> for suggesting a better implementation of
my original patch.

Signed-off-by: Haiyang Zhang <haiyangz at microsoft.com>
Signed-off-by: Hank Janssen <hjanssen at microsoft.com>
Cc:Evgeniy Polyakov <zbr at ioremap.net>
Cc:Jesper Juhl <jj at chaosbits.net>
Cc:Ky Srinivasan <ksrinivasan at 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

Greg KH

2010-Dec-13 18:34 UTC

head link

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

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 at ioremap.net> for pointing this
out.
> And thanks to Jesper Juhl <jj at chaosbits.net> and Ky Srinivasan 
> <ksrinivasan at novell.com> for suggesting a better implementation of
> my original patch.
> 
> Signed-off-by: Haiyang Zhang <haiyangz at microsoft.com>
> Signed-off-by: Hank Janssen <hjanssen at microsoft.com>
> Cc:Evgeniy Polyakov <zbr at ioremap.net>
> Cc:Jesper Juhl <jj at chaosbits.net>
> Cc:Ky Srinivasan <ksrinivasan at 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

Greg KH

2010-Dec-13 20:18 UTC

head link

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

On Mon, Dec 13, 2010 at 08:06:20PM +0000, Hank Janssen
wrote:> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:greg at 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

Reasonably Related Threads

Search for more apparently analagous threads

Linux Virtualization - Dec 2010 - [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

[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

Reasonably Related Threads