2010-12-13 20:31:37

by Hank Janssen

[permalink] [raw]
Subject: [PATCH 1/1] hv: Use only one receive buffer per channel 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 <[email protected]> for pointing this out.
And thanks to Jesper Juhl <[email protected]> and Ky Srinivasan
<[email protected]> for suggesting a better implementation of
my original patch.

Signed-off-by: Haiyang Zhang <[email protected]>
Signed-off-by: Hank Janssen <[email protected]>
Cc: Evgeniy Polyakov <[email protected]>
Cc: Jesper Juhl <[email protected]>
Cc: Ky Srinivasan <[email protected]>

---
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


2010-12-13 20:54:27

by Jesper Juhl

[permalink] [raw]
Subject: 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 <[email protected]> for pointing this out.
> And thanks to Jesper Juhl <[email protected]> and Ky Srinivasan
> <[email protected]> for suggesting a better implementation of
> my original patch.
>
> Signed-off-by: Haiyang Zhang <[email protected]>
> Signed-off-by: Hank Janssen <[email protected]>
> Cc: Evgeniy Polyakov <[email protected]>
> Cc: Jesper Juhl <[email protected]>
> Cc: Ky Srinivasan <[email protected]>
>
> ---
> 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 <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

2010-12-13 21:07:49

by Hank Janssen

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



> -----Original Message-----
> From: Jesper Juhl [mailto:[email protected]]
> 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.

2010-12-13 21:12:32

by Jesper Juhl

[permalink] [raw]
Subject: 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:[email protected]]
> > 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 <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

2010-12-13 21:17:01

by Hank Janssen

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



> -----Original Message-----
> From: Jesper Juhl [mailto:[email protected]]
> 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.

2010-12-13 22:05:17

by Ky Srinivasan

[permalink] [raw]
Subject: 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
<[email protected]>, Hank Janssen
<[email protected]> 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 <[email protected]> for pointing this out.
> And thanks to Jesper Juhl <[email protected]> and Ky Srinivasan
> <[email protected]> for suggesting a better implementation of
> my original patch.
>
> Signed-off-by: Haiyang Zhang <[email protected]>
> Signed-off-by: Hank Janssen <[email protected]>
> Cc: Evgeniy Polyakov <[email protected]>
> Cc: Jesper Juhl <[email protected]>
> Cc: Ky Srinivasan <[email protected]>
>
> ---
> 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);

2010-12-13 22:07:28

by Ky Srinivasan

[permalink] [raw]
Subject: 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
<[email protected]>, Hank Janssen
<[email protected]> 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 <[email protected]> for pointing this out.
> And thanks to Jesper Juhl <[email protected]> and Ky Srinivasan
> <[email protected]> for suggesting a better implementation of
> my original patch.
>
> Signed-off-by: Haiyang Zhang <[email protected]>
> Signed-off-by: Hank Janssen <[email protected]>
> Cc: Evgeniy Polyakov <[email protected]>
> Cc: Jesper Juhl <[email protected]>
> Cc: Ky Srinivasan <[email protected]>
>
> ---
> 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);

2010-12-13 23:57:12

by Jesper Juhl

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

On Mon, 13 Dec 2010, Ky Srinivasan wrote:

>
>
> >>> On 12/13/2010 at 3:34 PM, in message
> <[email protected]>, Hank Janssen
> <[email protected]> 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 <[email protected]> for pointing this out.
> > And thanks to Jesper Juhl <[email protected]> and Ky Srinivasan
> > <[email protected]> for suggesting a better implementation of
> > my original patch.
> >
> > Signed-off-by: Haiyang Zhang <[email protected]>
> > Signed-off-by: Hank Janssen <[email protected]>
> > Cc: Evgeniy Polyakov <[email protected]>
> > Cc: Jesper Juhl <[email protected]>
> > Cc: Ky Srinivasan <[email protected]>
> >
> > ---
> > 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 <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

2010-12-14 00:11:26

by Hank Janssen

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



> -----Original Message-----
> From: Jesper Juhl [mailto:[email protected]]
> 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.