* [GIT PULL net-next-2.6] vhost-net: tools, cleanups, optimizations
From: Michael S. Tsirkin @ 2010-12-13 10:44 UTC (permalink / raw)
To: David Miller; +Cc: kvm, virtualization, netdev, linux-kernel
Please merge the following tree for 2.6.38.
Thanks!
The following changes since commit ad1184c6cf067a13e8cb2a4e7ccc407f947027d0:
net: au1000_eth: remove unused global variable. (2010-12-11 12:01:48 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-net-next
Jason Wang (1):
vhost: fix typos in comment
Julia Lawall (1):
drivers/vhost/vhost.c: delete double assignment
Michael S. Tsirkin (9):
vhost: put mm after thread stop
vhost-net: batch use/unuse mm
vhost: copy_to_user -> __copy_to_user
vhost: get/put_user -> __get/__put_user
vhost: remove unused include
vhost: correctly set bits of dirty pages
vhost: better variable name in logging
vhost test module
tools/virtio: virtio_test tool
drivers/vhost/net.c | 9 +-
drivers/vhost/test.c | 320 ++++++++++++++++++++++++++++++++++
drivers/vhost/test.h | 7 +
drivers/vhost/vhost.c | 44 +++---
drivers/vhost/vhost.h | 2 +-
tools/virtio/Makefile | 12 ++
tools/virtio/linux/device.h | 2 +
tools/virtio/linux/slab.h | 2 +
tools/virtio/linux/virtio.h | 223 +++++++++++++++++++++++
tools/virtio/vhost_test/Makefile | 2 +
tools/virtio/vhost_test/vhost_test.c | 1 +
tools/virtio/virtio_test.c | 248 ++++++++++++++++++++++++++
12 files changed, 842 insertions(+), 30 deletions(-)
create mode 100644 drivers/vhost/test.c
create mode 100644 drivers/vhost/test.h
create mode 100644 tools/virtio/Makefile
create mode 100644 tools/virtio/linux/device.h
create mode 100644 tools/virtio/linux/slab.h
create mode 100644 tools/virtio/linux/virtio.h
create mode 100644 tools/virtio/vhost_test/Makefile
create mode 100644 tools/virtio/vhost_test/vhost_test.c
create mode 100644 tools/virtio/virtio_test.c
^ permalink raw reply
* Re: [Qemu-devel] SCSI Command support over VirtIO Block device
From: अनुज @ 2010-12-13 10:47 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, kvm, virtualization
In-Reply-To: <AANLkTi=MVouwpAu5Zfxexx-yOZa3p0SpQ4OrO+_ax-Ew@mail.gmail.com>
Hi
2010/12/13 Stefan Hajnoczi <stefanha@gmail.com>:
>
> On Dec 13, 2010 5:14 AM, "अनुज" <anuj01@gmail.com> wrote:
>>
>> Hi
>>
>> I am trying to implement VirtIO support for a proprietary OS. And It
>> would be great if I am able to process SCSI commands over VirtIO Block
>> device.
>>
>> I tried to execute INQUIRY command but the status returned is UNSUPPORTED.
>> If anyone provide example VirtIO SCSI Command request structure for
>> INQUIRY command as per VirtIO spec Appendix D would be a great help.
>>
>> And also, the paragraph from VirtIO spec - 0.8.9 is confusing for me :
>>
>> "Historically, devices assumed that the fields type, ioprio and
>> sector reside in
>> a single, separate read-only buffer; the fields errors, data_len,
>> sense_len and
>> residual reside in a single, separate write-only buffer; the sense
>> eld in a separate
>> write-only buffer of size 96 bytes, by itself; the fields errors,
>> data_len, sense_len
>> and residual in a single write-only buffer; and the status field is a
>> separate readonly
>> buffer of size 1 byte, by itself."
>>
>> Here 'status field of buffer size 1 byte' is whether readonly or
>> writeonly?
>
> Writeonly
>
>>
>> I want to know from which version of Qemu-kvm supports processing of
>> scsi commands over VirtIO block device as a backend.
>> Although I checked the Host Feature fields in which VIRTIO_BLK_F_SCSI
>> bit is set. I am using qemu-kvm version 0.12.3.
>
> Make sure you have a scsi-generic block device in qemu-kvm, not just a
> regular file or physical block device. Open /dev/sg.
Yes, I have given a file name instead of /dev/sg0. Now it's working as a charm.
That means I can use physical disk as a VirtIO disk in guest OS. right?
So it's kind of passthrough for a physical disk. But how can I
distinguish among different physical disks attached to the host.
is /dev/sg is different for each physical disk?
However I thought VirtIO scsi device operations are for virtual disk
(a regular file) also.
>
> Look at hw/virtio-blk.c in qemu-kvm for host implementation details.
>
>>
>> --
>
>> Anuj Aggarwal
>>
>> .''`.
>> : :Ⓐ : # apt-get install hakuna-matata
>> `. `'`
>> `-
>>
>
Thanks for your help.
Regards
--
Anuj Aggarwal
.''`.
: :Ⓐ : # apt-get install hakuna-matata
`. `'`
`-
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 2/2] tools/virtio: virtio_test tool
From: Michael S. Tsirkin @ 2010-12-13 16:58 UTC (permalink / raw)
To: Thiago Farina; +Cc: linux-kernel, kvm, virtualization
In-Reply-To: <AANLkTinSia0yt3xmNusXezVsmembv9NKoymN+rrBn_pY@mail.gmail.com>
On Mon, Dec 06, 2010 at 02:37:05PM -0200, Thiago Farina wrote:
> On Mon, Nov 29, 2010 at 3:16 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > +#define container_of(ptr, type, member) ({ \
> > + const typeof( ((type *)0)->member ) *__mptr = (ptr); \
> > + (type *)( (char *)__mptr - offsetof(type,member) );})
> > +
> > +#define uninitialized_var(x) x = x
> > +
> > +# ifndef likely
> > +# define likely(x) (__builtin_expect(!!(x), 1))
> > +# endif
> > +# ifndef unlikely
> > +# define unlikely(x) (__builtin_expect(!!(x), 0))
> > +# endif
>
> It seems you are not using these macros. Do you really need them here?
They are used by virtio that I'm compiling in userspace here.
> Can't you include the right linux header files for these macros
> instead?
Far from trivial as linux headers aren't intended to
be built in userspace, if you try you get all kind of
conflicts with libc headers etc.
If you see a way to do this, pls send me a patch.
--
MST
^ permalink raw reply
* Re: [PATCH 2/2] tools/virtio: virtio_test tool
From: Michael S. Tsirkin @ 2010-12-13 17:14 UTC (permalink / raw)
To: Rusty Russell; +Cc: linux-kernel, kvm, virtualization
In-Reply-To: <201012061523.03108.rusty@rustcorp.com.au>
On Mon, Dec 06, 2010 at 03:23:02PM +1030, Rusty Russell wrote:
> On Tue, 30 Nov 2010 03:46:37 am Michael S. Tsirkin wrote:
> > This is the userspace part of the tool: it includes a bunch of stubs for
> > linux APIs, somewhat simular to linuxsched. This makes it possible to
> > recompile the ring code in userspace.
> >
> > A small test example is implemented combining this with vhost_test
> > module.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Hi Michael,
>
> I'm not sure what the point is of this work? You'll still need to
> benchmark on real systems, but it's not low-level enough to measure
> things like cache misses.
The point is to be able to create easy to test workloads:
(just running the single test included here produces a
result that seems repeatable to a high degree)
while still staying as close as possible to what we might expect in real
life.
I also want to be able to measure just the overhead of the ring,
without involving block or network core in guest and host.
In other words, it's a synthetic benchmark.
> I'm assuming you're thinking of playing with layout to measure cache
> behaviour.
In one example, using this test I saw that different publish
used index layouts don't seem to behave at all differently.
But I also saw that the extra pointer hasing
added by my publish used index patches did add
measureable overhead.
Plan to look into that.
> I was thinking of a complete userspace implementation
The disadvantage is that any work done there needs to be
redone in real life, though. And implementation details often matter.
What I did let me actually use the virtio/vhost code that we have and see how
it performs.
> where
> either it was run under cachegrind, or each access was wrapped to allow
> tracking of cachelines to give an exact measure of cache movement
perf stat not good enough?
> under
> various scenarios (esp. ring mostly empty, ring in steady state, ring
> mostly full).
Yes, I do want to add tests to stress various scenarios.
>
> Cheers,
> Rusty.
--
MST
^ permalink raw reply
* Re: [GIT PULL net-next-2.6] vhost-net: tools, cleanups, optimizations
From: Michael S. Tsirkin @ 2010-12-13 17:24 UTC (permalink / raw)
To: David Miller; +Cc: kvm, virtualization, netdev, linux-kernel, Rusty Russell
In-Reply-To: <20101213104413.GA678@redhat.com>
On Mon, Dec 13, 2010 at 12:44:13PM +0200, Michael S. Tsirkin wrote:
> Please merge the following tree for 2.6.38.
> Thanks!
Um, I sent this out before I noticed the mail from Rusty
with some questions on the test code. I missed that and
assumed no comments -> no issues, perhaps wrongly.
Rusty - I tried answering the questions there - any issues
with merging this? It's just a test so won't be hard to remove
later if it's not helpful ...
> The following changes since commit ad1184c6cf067a13e8cb2a4e7ccc407f947027d0:
>
> net: au1000_eth: remove unused global variable. (2010-12-11 12:01:48 -0800)
>
> are available in the git repository at:
> git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-net-next
>
> Jason Wang (1):
> vhost: fix typos in comment
>
> Julia Lawall (1):
> drivers/vhost/vhost.c: delete double assignment
>
> Michael S. Tsirkin (9):
> vhost: put mm after thread stop
> vhost-net: batch use/unuse mm
> vhost: copy_to_user -> __copy_to_user
> vhost: get/put_user -> __get/__put_user
> vhost: remove unused include
> vhost: correctly set bits of dirty pages
> vhost: better variable name in logging
> vhost test module
> tools/virtio: virtio_test tool
>
> drivers/vhost/net.c | 9 +-
> drivers/vhost/test.c | 320 ++++++++++++++++++++++++++++++++++
> drivers/vhost/test.h | 7 +
> drivers/vhost/vhost.c | 44 +++---
> drivers/vhost/vhost.h | 2 +-
> tools/virtio/Makefile | 12 ++
> tools/virtio/linux/device.h | 2 +
> tools/virtio/linux/slab.h | 2 +
> tools/virtio/linux/virtio.h | 223 +++++++++++++++++++++++
> tools/virtio/vhost_test/Makefile | 2 +
> tools/virtio/vhost_test/vhost_test.c | 1 +
> tools/virtio/virtio_test.c | 248 ++++++++++++++++++++++++++
> 12 files changed, 842 insertions(+), 30 deletions(-)
> create mode 100644 drivers/vhost/test.c
> create mode 100644 drivers/vhost/test.h
> create mode 100644 tools/virtio/Makefile
> create mode 100644 tools/virtio/linux/device.h
> create mode 100644 tools/virtio/linux/slab.h
> create mode 100644 tools/virtio/linux/virtio.h
> create mode 100644 tools/virtio/vhost_test/Makefile
> create mode 100644 tools/virtio/vhost_test/vhost_test.c
> create mode 100644 tools/virtio/virtio_test.c
^ permalink raw reply
* [PATCH 1/1] hv: Use only one receive buffer and kmalloc on initialize
From: Hank Janssen @ 2010-12-13 17:45 UTC (permalink / raw)
To: hjanssen, gregkh, linux-kernel, devel, virtualization; +Cc: Haiyang Zhang
Correct issue with not checking kmalloc return value.
This fix now only uses one receive buffer for all hv_utils
channels, and will do only one kmalloc on init and will return
with a -ENOMEM if kmalloc fails on initialize.
Thanks to Evgeniy Polyakov <zbr@ioremap.net> for pointing this out.
And thanks to Jesper Juhl <jj@chaosbits.net> and Ky Srinivasan
<ksrinivasan@novell.com> for suggesting a better implementation of
my original patch.
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
Cc:Evgeniy Polyakov <zbr@ioremap.net>
Cc:Jesper Juhl <jj@chaosbits.net>
Cc:Ky Srinivasan <ksrinivasan@novell.com>
---
drivers/staging/hv/hv_utils.c | 68 +++++++++++++++++++---------------------
1 files changed, 32 insertions(+), 36 deletions(-)
diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index 53e1e29..4ed4ab8 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -38,12 +38,15 @@
#include "vmbus_api.h"
#include "utils.h"
+/*
+ * Buffer used to receive packets from Hyper-V
+ */
+static u8 *chan_buf;
static void shutdown_onchannelcallback(void *context)
{
struct vmbus_channel *channel = context;
- u8 *buf;
- u32 buflen, recvlen;
+ u32 recvlen;
u64 requestid;
u8 execute_shutdown = false;
@@ -52,22 +55,19 @@ static void shutdown_onchannelcallback(void *context)
struct icmsg_hdr *icmsghdrp;
struct icmsg_negotiate *negop = NULL;
- buflen = PAGE_SIZE;
- buf = kmalloc(buflen, GFP_ATOMIC);
-
- vmbus_recvpacket(channel, buf, buflen, &recvlen, &requestid);
+ vmbus_recvpacket(channel, chan_buf, PAGE_SIZE, &recvlen, &requestid);
if (recvlen > 0) {
DPRINT_DBG(VMBUS, "shutdown packet: len=%d, requestid=%lld",
recvlen, requestid);
- icmsghdrp = (struct icmsg_hdr *)&buf[
+ icmsghdrp = (struct icmsg_hdr *)&chan_buf[
sizeof(struct vmbuspipe_hdr)];
if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
- prep_negotiate_resp(icmsghdrp, negop, buf);
+ prep_negotiate_resp(icmsghdrp, negop, chan_buf);
} else {
- shutdown_msg = (struct shutdown_msg_data *)&buf[
+ shutdown_msg = (struct shutdown_msg_data *)&chan_buf[
sizeof(struct vmbuspipe_hdr) +
sizeof(struct icmsg_hdr)];
@@ -93,13 +93,11 @@ static void shutdown_onchannelcallback(void *context)
icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
| ICMSGHDRFLAG_RESPONSE;
- vmbus_sendpacket(channel, buf,
+ vmbus_sendpacket(channel, chan_buf,
recvlen, requestid,
VmbusPacketTypeDataInBand, 0);
}
- kfree(buf);
-
if (execute_shutdown == true)
orderly_poweroff(false);
}
@@ -150,28 +148,24 @@ static inline void adj_guesttime(u64 hosttime, u8 flags)
static void timesync_onchannelcallback(void *context)
{
struct vmbus_channel *channel = context;
- u8 *buf;
- u32 buflen, recvlen;
+ u32 recvlen;
u64 requestid;
struct icmsg_hdr *icmsghdrp;
struct ictimesync_data *timedatap;
- buflen = PAGE_SIZE;
- buf = kmalloc(buflen, GFP_ATOMIC);
-
- vmbus_recvpacket(channel, buf, buflen, &recvlen, &requestid);
+ vmbus_recvpacket(channel, chan_buf, PAGE_SIZE, &recvlen, &requestid);
if (recvlen > 0) {
DPRINT_DBG(VMBUS, "timesync packet: recvlen=%d, requestid=%lld",
recvlen, requestid);
- icmsghdrp = (struct icmsg_hdr *)&buf[
+ icmsghdrp = (struct icmsg_hdr *)&chan_buf[
sizeof(struct vmbuspipe_hdr)];
if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
- prep_negotiate_resp(icmsghdrp, NULL, buf);
+ prep_negotiate_resp(icmsghdrp, NULL, chan_buf);
} else {
- timedatap = (struct ictimesync_data *)&buf[
+ timedatap = (struct ictimesync_data *)&chan_buf[
sizeof(struct vmbuspipe_hdr) +
sizeof(struct icmsg_hdr)];
adj_guesttime(timedatap->parenttime, timedatap->flags);
@@ -180,12 +174,10 @@ static void timesync_onchannelcallback(void *context)
icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
| ICMSGHDRFLAG_RESPONSE;
- vmbus_sendpacket(channel, buf,
+ vmbus_sendpacket(channel, chan_buf,
recvlen, requestid,
VmbusPacketTypeDataInBand, 0);
}
-
- kfree(buf);
}
/*
@@ -196,28 +188,24 @@ static void timesync_onchannelcallback(void *context)
static void heartbeat_onchannelcallback(void *context)
{
struct vmbus_channel *channel = context;
- u8 *buf;
- u32 buflen, recvlen;
+ u32 recvlen;
u64 requestid;
struct icmsg_hdr *icmsghdrp;
struct heartbeat_msg_data *heartbeat_msg;
- buflen = PAGE_SIZE;
- buf = kmalloc(buflen, GFP_ATOMIC);
-
- vmbus_recvpacket(channel, buf, buflen, &recvlen, &requestid);
+ vmbus_recvpacket(channel, chan_buf, PAGE_SIZE, &recvlen, &requestid);
if (recvlen > 0) {
DPRINT_DBG(VMBUS, "heartbeat packet: len=%d, requestid=%lld",
recvlen, requestid);
- icmsghdrp = (struct icmsg_hdr *)&buf[
+ icmsghdrp = (struct icmsg_hdr *)&chan_buf[
sizeof(struct vmbuspipe_hdr)];
if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
- prep_negotiate_resp(icmsghdrp, NULL, buf);
+ prep_negotiate_resp(icmsghdrp, NULL, chan_buf);
} else {
- heartbeat_msg = (struct heartbeat_msg_data *)&buf[
+ heartbeat_msg = (struct heartbeat_msg_data *)&chan_buf[
sizeof(struct vmbuspipe_hdr) +
sizeof(struct icmsg_hdr)];
@@ -230,12 +218,10 @@ static void heartbeat_onchannelcallback(void *context)
icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
| ICMSGHDRFLAG_RESPONSE;
- vmbus_sendpacket(channel, buf,
+ vmbus_sendpacket(channel, chan_buf,
recvlen, requestid,
VmbusPacketTypeDataInBand, 0);
}
-
- kfree(buf);
}
static const struct pci_device_id __initconst
@@ -268,6 +254,14 @@ static int __init init_hyperv_utils(void)
if (!dmi_check_system(hv_utils_dmi_table))
return -ENODEV;
+ chan_buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
+
+ if (!chan_buf) {
+ printk(KERN_INFO
+ "Unable to allocate memory for receive buffer\n");
+ return -ENOMEM;
+ }
+
hv_cb_utils[HV_SHUTDOWN_MSG].channel->onchannel_callback =
&shutdown_onchannelcallback;
hv_cb_utils[HV_SHUTDOWN_MSG].callback = &shutdown_onchannelcallback;
@@ -298,6 +292,8 @@ static void exit_hyperv_utils(void)
hv_cb_utils[HV_HEARTBEAT_MSG].channel->onchannel_callback =
&chn_cb_negotiate;
hv_cb_utils[HV_HEARTBEAT_MSG].callback = &chn_cb_negotiate;
+
+ kfree(chan_buf);
}
module_init(init_hyperv_utils);
--
1.6.0.2
^ permalink raw reply related
* Re: [PATCH 1/1] hv: Use only one receive buffer and kmalloc on initialize
From: Greg KH @ 2010-12-13 18:34 UTC (permalink / raw)
To: Hank Janssen; +Cc: gregkh, linux-kernel, devel, virtualization, Haiyang Zhang
In-Reply-To: <1292262350-29001-1-git-send-email-hjanssen@microsoft.com>
On Mon, Dec 13, 2010 at 09:45:50AM -0800, Hank Janssen wrote:
> Correct issue with not checking kmalloc return value.
> This fix now only uses one receive buffer for all hv_utils
> channels, and will do only one kmalloc on init and will return
> with a -ENOMEM if kmalloc fails on initialize.
>
> Thanks to Evgeniy Polyakov <zbr@ioremap.net> for pointing this out.
> And thanks to Jesper Juhl <jj@chaosbits.net> and Ky Srinivasan
> <ksrinivasan@novell.com> for suggesting a better implementation of
> my original patch.
>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> Cc:Evgeniy Polyakov <zbr@ioremap.net>
> Cc:Jesper Juhl <jj@chaosbits.net>
> Cc:Ky Srinivasan <ksrinivasan@novell.com>
>
> ---
> drivers/staging/hv/hv_utils.c | 68 +++++++++++++++++++---------------------
> 1 files changed, 32 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
> index 53e1e29..4ed4ab8 100644
> --- a/drivers/staging/hv/hv_utils.c
> +++ b/drivers/staging/hv/hv_utils.c
> @@ -38,12 +38,15 @@
> #include "vmbus_api.h"
> #include "utils.h"
>
> +/*
> + * Buffer used to receive packets from Hyper-V
> + */
> +static u8 *chan_buf;
One buffer is nicer, yes, but what's controlling access to this buffer?
You use it in multiple functions, and what's to say those functions
can't be called at the same time on different cpus? So, shouldn't you
either have some locking for access to the buffer, or have a
per-function buffer instead?
And if you have a per-function buffer, again, you might need to control
access to it as the functions could be called multiple times at the same
time, right?
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH 1/1] hv: Use only one receive buffer and kmalloc on initialize
From: Hank Janssen @ 2010-12-13 19:31 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <20101213183448.GA11745@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Monday, December 13, 2010 10:35 AM
> > ---
> > drivers/staging/hv/hv_utils.c | 68 +++++++++++++++++++------------------
> ---
> > 1 files changed, 32 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/staging/hv/hv_utils.c
> > b/drivers/staging/hv/hv_utils.c index 53e1e29..4ed4ab8 100644
> > --- a/drivers/staging/hv/hv_utils.c
> > +++ b/drivers/staging/hv/hv_utils.c
> > @@ -38,12 +38,15 @@
> > #include "vmbus_api.h"
> > #include "utils.h"
> >
> > +/*
> > + * Buffer used to receive packets from Hyper-V */ static u8
> > +*chan_buf;
>
> One buffer is nicer, yes, but what's controlling access to this buffer?
> You use it in multiple functions, and what's to say those functions can't be
> called at the same time on different cpus? So, shouldn't you either have
> some locking for access to the buffer, or have a per-function buffer instead?
>
> And if you have a per-function buffer, again, you might need to control
> access to it as the functions could be called multiple times at the same time,
> right?
>
The current versions of Hyper-V support interrupt handling on CPU0 only.
I can make multiple buffers per channel, but because of Hyper-V implementation
It does not really make a difference.
Hank.
^ permalink raw reply
* Re: [PATCH 1/1] hv: Use only one receive buffer and kmalloc on initialize
From: Greg KH @ 2010-12-13 19:41 UTC (permalink / raw)
To: Hank Janssen
Cc: devel@linuxdriverproject.org, Haiyang Zhang, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <8AFC7968D54FB448A30D8F38F259C5622C0BB71F@TK5EX14MBXC116.redmond.corp.microsoft.com>
On Mon, Dec 13, 2010 at 07:31:42PM +0000, Hank Janssen wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Monday, December 13, 2010 10:35 AM
> > > ---
> > > drivers/staging/hv/hv_utils.c | 68 +++++++++++++++++++------------------
> > ---
> > > 1 files changed, 32 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/drivers/staging/hv/hv_utils.c
> > > b/drivers/staging/hv/hv_utils.c index 53e1e29..4ed4ab8 100644
> > > --- a/drivers/staging/hv/hv_utils.c
> > > +++ b/drivers/staging/hv/hv_utils.c
> > > @@ -38,12 +38,15 @@
> > > #include "vmbus_api.h"
> > > #include "utils.h"
> > >
> > > +/*
> > > + * Buffer used to receive packets from Hyper-V */ static u8
> > > +*chan_buf;
> >
> > One buffer is nicer, yes, but what's controlling access to this buffer?
> > You use it in multiple functions, and what's to say those functions can't be
> > called at the same time on different cpus? So, shouldn't you either have
> > some locking for access to the buffer, or have a per-function buffer instead?
> >
> > And if you have a per-function buffer, again, you might need to control
> > access to it as the functions could be called multiple times at the same time,
> > right?
> >
>
> The current versions of Hyper-V support interrupt handling on CPU0 only.
> I can make multiple buffers per channel, but because of Hyper-V implementation
> It does not really make a difference.
Then put a big fat note in there saying this, and that it will have to
change if the hyperv channel ever changes.
Hm, how will you handle things if the hyperv core changes and an old
kernel is running this code where things were "assumed" about the
reentrancy happening here?
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH 1/1] hv: Use only one receive buffer and kmalloc on initialize
From: Hank Janssen @ 2010-12-13 20:06 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <20101213194101.GA18431@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Monday, December 13, 2010 11:41 AM
> > The current versions of Hyper-V support interrupt handling on CPU0 only.
> > I can make multiple buffers per channel, but because of Hyper-V
> > implementation It does not really make a difference.
>
> Then put a big fat note in there saying this, and that it will have to change if
> the hyperv channel ever changes.
>
> Hm, how will you handle things if the hyperv core changes and an old kernel
> is running this code where things were "assumed" about the reentrancy
> happening here?
I will re-roll this patch to have a buffer per channel. It is a more elegant design
Even though Hyper-V behavior is not changing in Win8 for this.
Hank.
^ permalink raw reply
* Re: [PATCH 1/1] hv: Use only one receive buffer and kmalloc on initialize
From: Greg KH @ 2010-12-13 20:18 UTC (permalink / raw)
To: Hank Janssen
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <8AFC7968D54FB448A30D8F38F259C5622C0BC809@TK5EX14MBXC116.redmond.corp.microsoft.com>
On Mon, Dec 13, 2010 at 08:06:20PM +0000, Hank Janssen wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Monday, December 13, 2010 11:41 AM
> > > The current versions of Hyper-V support interrupt handling on CPU0 only.
> > > I can make multiple buffers per channel, but because of Hyper-V
> > > implementation It does not really make a difference.
> >
> > Then put a big fat note in there saying this, and that it will have to change if
> > the hyperv channel ever changes.
> >
> > Hm, how will you handle things if the hyperv core changes and an old kernel
> > is running this code where things were "assumed" about the reentrancy
> > happening here?
>
> I will re-roll this patch to have a buffer per channel. It is a more elegant design
> Even though Hyper-V behavior is not changing in Win8 for this.
Win8, fine, but what about Win9? :)
thanks for changing it.
greg k-h
^ permalink raw reply
* [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
From: Hank Janssen @ 2010-12-13 20:34 UTC (permalink / raw)
To: hjanssen, gregkh, linux-kernel, devel, virtualization
Cc: Haiyang Zhang, Evgeniy Polyakov, Jesper Juhl, Ky Srinivasan
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
^ permalink raw reply related
* Re: [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
From: Jesper Juhl @ 2010-12-13 20:48 UTC (permalink / raw)
To: Hank Janssen
Cc: Haiyang Zhang, gregkh, linux-kernel, virtualization,
Evgeniy Polyakov, devel, Ky Srinivasan
In-Reply-To: <1292272498-29483-1-git-send-email-hjanssen@microsoft.com>
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.
^ permalink raw reply
* RE: [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
From: Jesper Juhl @ 2010-12-13 21:06 UTC (permalink / raw)
To: Hank Janssen
Cc: Haiyang Zhang, gregkh@suse.de, linux-kernel@vger.kernel.org,
virtualization@lists.osdl.org, Evgeniy Polyakov,
devel@linuxdriverproject.org, Ky Srinivasan
In-Reply-To: <8AFC7968D54FB448A30D8F38F259C5622C0BCB78@TK5EX14MBXC116.redmond.corp.microsoft.com>
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.
^ permalink raw reply
* RE: [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
From: Hank Janssen @ 2010-12-13 21:07 UTC (permalink / raw)
To: Jesper Juhl
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Evgeniy Polyakov, Ky Srinivasan
In-Reply-To: <alpine.LNX.2.00.1012132143590.26491@swampdragon.chaosbits.net>
> -----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.
^ permalink raw reply
* [PATCH 1/1] hv: Use only one txf buffer per channel and kmalloc on initialize
From: Hank Janssen @ 2010-12-13 21:14 UTC (permalink / raw)
To: hjanssen, gregkh, linux-kernel, devel, virtualization
Cc: Haiyang Zhang, Evgeniy Polyakov, Jesper Juhl, Ky Srinivasan
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.
And properly clean up memory on failure.
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,19 @@ 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");
+ kfree(shut_txf_buf);
+ kfree(time_txf_buf);
+ kfree(hbeat_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);
--
1.6.0.2
^ permalink raw reply related
* RE: [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
From: Hank Janssen @ 2010-12-13 21:16 UTC (permalink / raw)
To: Jesper Juhl
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Evgeniy Polyakov, Ky Srinivasan
In-Reply-To: <alpine.LNX.2.00.1012132204240.26491@swampdragon.chaosbits.net>
> -----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.
^ permalink raw reply
* Re: [PATCH 1/1] hv: Use only one txf buffer per channel and kmalloc on initialize
From: Jesper Juhl @ 2010-12-13 21:22 UTC (permalink / raw)
To: Hank Janssen
Cc: gregkh, linux-kernel, devel, virtualization, Haiyang Zhang,
Evgeniy Polyakov, Ky Srinivasan
In-Reply-To: <1292274861-29673-1-git-send-email-hjanssen@microsoft.com>
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.
>
> And properly clean up memory on failure.
>
> 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>
>
I can't spot any problems with these changes now, so feel free to add
Reviewed-by: Jesper Juhl <jj@chaosbits.net>
if you like.
--
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.
^ permalink raw reply
* RE: [PATCH 1/1] hv: Use only one txf buffer per channel and kmalloc on initialize
From: Hank Janssen @ 2010-12-13 21:31 UTC (permalink / raw)
To: Jesper Juhl
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Evgeniy Polyakov, Ky Srinivasan
In-Reply-To: <alpine.LNX.2.00.1012132220220.26491@swampdragon.chaosbits.net>
> From: Jesper Juhl [mailto:jj@chaosbits.net]
> Sent: Monday, December 13, 2010 1:22 PM
>
> I can't spot any problems with these changes now, so feel free to add
> Reviewed-by: Jesper Juhl <jj@chaosbits.net> if you like.
>
Thank you for your help Jesper.
Greg, do you want me to resubmit with the Reviewed by line? Or do you
Add it once you queue it up for linux-next?
Thanks,
Hank.
^ permalink raw reply
* Re: [PATCH 1/1] hv: Use only one txf buffer per channel and kmalloc on initialize
From: Greg KH @ 2010-12-13 21:47 UTC (permalink / raw)
To: Hank Janssen
Cc: Jesper Juhl, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Evgeniy Polyakov, Ky Srinivasan
In-Reply-To: <8AFC7968D54FB448A30D8F38F259C5622C0BCDFC@TK5EX14MBXC116.redmond.corp.microsoft.com>
On Mon, Dec 13, 2010 at 09:31:09PM +0000, Hank Janssen wrote:
>
> > From: Jesper Juhl [mailto:jj@chaosbits.net]
> > Sent: Monday, December 13, 2010 1:22 PM
> >
> > I can't spot any problems with these changes now, so feel free to add
> > Reviewed-by: Jesper Juhl <jj@chaosbits.net> if you like.
> >
>
> Thank you for your help Jesper.
>
> Greg, do you want me to resubmit with the Reviewed by line? Or do you
> Add it once you queue it up for linux-next?
I add it when I apply it.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 1/1] hv: Use only one txf buffer per channel and kmalloc on initialize
From: Evgeniy Polyakov @ 2010-12-13 22:03 UTC (permalink / raw)
To: Hank Janssen
Cc: Haiyang Zhang, gregkh, linux-kernel, virtualization, Jesper Juhl,
devel, Ky Srinivasan
In-Reply-To: <1292274861-29673-1-git-send-email-hjanssen@microsoft.com>
On Mon, Dec 13, 2010 at 01:14:21PM -0800, Hank Janssen (hjanssen@microsoft.com) wrote:
> +static u8 *shut_txf_buf;
> +static u8 *time_txf_buf;
> +static u8 *hbeat_txf_buf;
Those are accessed without any kind of synchronization. I do not know
exact context, but are you sure it is single-threaded?
--
Evgeniy Polyakov
^ permalink raw reply
* Re: [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
From: Ky Srinivasan @ 2010-12-13 22:05 UTC (permalink / raw)
To: devel, virtualization, hjanssen, gregkh, linux-kernel
Cc: Jesper Juhl, Evgeniy Polyakov, Haiyang Zhang
In-Reply-To: <1292272498-29483-1-git-send-email-hjanssen@microsoft.com>
>>> 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);
^ permalink raw reply
* Re: [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
From: Ky Srinivasan @ 2010-12-13 22:07 UTC (permalink / raw)
To: devel, virtualization, hjanssen, gregkh, linux-kernel
Cc: Jesper Juhl, Evgeniy Polyakov, Haiyang Zhang
In-Reply-To: <1292272498-29483-1-git-send-email-hjanssen@microsoft.com>
>>> 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);
^ permalink raw reply
* RE: [PATCH 1/1] hv: Use only one txf buffer per channel and kmalloc on initialize
From: Hank Janssen @ 2010-12-13 22:57 UTC (permalink / raw)
To: Evgeniy Polyakov
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Jesper Juhl, Ky Srinivasan
In-Reply-To: <20101213220333.GA1636@ioremap.net>
-----Original Message-----
>From: Evgeniy Polyakov [mailto:zbr@ioremap.net]
>Sent: Monday, December 13, 2010 2:04 PM
>
>On Mon, Dec 13, 2010 at 01:14:21PM -0800, Hank Janssen (hjanssen@microsoft.com) wrote:
>> +static u8 *shut_txf_buf;
>> +static u8 *time_txf_buf;
>> +static u8 *hbeat_txf_buf;
>
>Those are accessed without any kind of synchronization. I do not know exact context, but are you sure it is single-threaded?
The hv_utils driver services are all single threaded. The other drivers are not, but this one is.
Hank.
^ permalink raw reply
* Re: [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
From: Jesper Juhl @ 2010-12-13 23:50 UTC (permalink / raw)
To: Ky Srinivasan
Cc: devel, virtualization, hjanssen, gregkh, linux-kernel,
Evgeniy Polyakov, Haiyang Zhang
In-Reply-To: <4D0636270200003000091D23@novprvoes0310.provo.novell.com>
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.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox