Re: [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
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 z...@ioremap.net for pointing this out.
And thanks to Jesper Juhl j...@chaosbits.net and Ky Srinivasan
ksriniva...@novell.com for suggesting a better implementation of
my original patch.
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
Cc: Evgeniy Polyakov z...@ioremap.net
Cc: Jesper Juhl j...@chaosbits.net
Cc: Ky Srinivasan ksriniva...@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
RE: [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
-Original Message-
From: Jesper Juhl [mailto:j...@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.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
-Original Message- From: Jesper Juhl [mailto:j...@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. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
On Mon, 13 Dec 2010, Hank Janssen wrote:
-Original Message-
From: Jesper Juhl [mailto:j...@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 j...@chaosbits.nethttp://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
On 12/13/2010 at 3:34 PM, in message
1292272498-29483-1-git-send-email-hjans...@microsoft.com, Hank Janssen
hjans...@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 z...@ioremap.net for pointing this out.
And thanks to Jesper Juhl j...@chaosbits.net and Ky Srinivasan
ksriniva...@novell.com for suggesting a better implementation of
my original patch.
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
Cc: Evgeniy Polyakov z...@ioremap.net
Cc: Jesper Juhl j...@chaosbits.net
Cc: Ky Srinivasan ksriniva...@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)];
Re: [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
On 12/13/2010 at 3:34 PM, in message
1292272498-29483-1-git-send-email-hjans...@microsoft.com, Hank Janssen
hjans...@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 z...@ioremap.net for pointing this out.
And thanks to Jesper Juhl j...@chaosbits.net and Ky Srinivasan
ksriniva...@novell.com for suggesting a better implementation of
my original patch.
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
Cc: Evgeniy Polyakov z...@ioremap.net
Cc: Jesper Juhl j...@chaosbits.net
Cc: Ky Srinivasan ksriniva...@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[
RE: [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
-Original Message- From: Jesper Juhl [mailto:j...@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. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
