[Qemu-devel] [PATCH] net.c: Moved large array in nc_sendv_compat from the stack to the heap

[Qemu-devel] [PATCH] net.c: Moved large array in nc_sendv_compat from the stack to the heap

[Nikos Filippakis]

Hello everyone! I am interested in getting to know the codebase a little better
so that I can eventually apply for a GSOC position.
This is my first attempt at posting a patch to a mailing list, any feedback
is greatly appreciated.

Signed-off-by: Nikos Filippakis <aesmade@gmail.com>
---
 net/net.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/net.c b/net/net.c
index aebf753..79e9d7c 100644
--- a/net/net.c
+++ b/net/net.c
@@ -710,23 +710,30 @@ ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size)
 static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov,
                                int iovcnt, unsigned flags)
 {
-    uint8_t buf[NET_BUFSIZE];
     uint8_t *buffer;
     size_t offset;
+    ssize_t ret;
 
     if (iovcnt == 1) {
         buffer = iov[0].iov_base;
         offset = iov[0].iov_len;
     } else {
-        buffer = buf;
-        offset = iov_to_buf(iov, iovcnt, 0, buf, sizeof(buf));
+        buffer = g_malloc(NET_BUFSIZE * sizeof(uint8_t));
+        offset = iov_to_buf(iov, iovcnt, 0, buffer,
+                            NET_BUFSIZE * sizeof(uint8_t));
     }
 
     if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) {
-        return nc->info->receive_raw(nc, buffer, offset);
+        ret = nc->info->receive_raw(nc, buffer, offset);
     } else {
-        return nc->info->receive(nc, buffer, offset);
+        ret = nc->info->receive(nc, buffer, offset);
     }
+
+    if (iovcnt != 1) {
+        g_free(buffer);
+    }
+
+    return ret;
 }
 
 ssize_t qemu_deliver_packet_iov(NetClientState *sender,
-- 
1.9.1

Re: [Qemu-devel] [PATCH] net.c: Moved large array in nc_sendv_compat from the stack to the heap

[Alex Bennée]

Nikos Filippakis <aesmade@gmail.com> writes:

> Hello everyone! I am interested in getting to know the codebase a little better
> so that I can eventually apply for a GSOC position.
> This is my first attempt at posting a patch to a mailing list, any feedback
> is greatly appreciated.

OK first things first this sort of meta comment belongs in the cover
letter. However for a single patch you may want to put such things below
the --- in the commit message as that will get stripped when the
maintainer eventually applies the patch. Otherwise your meta-comments
will end up in the version log ;-)

You'll see people use the --- area to keep version notes as patches go
through revisions.

>
> Signed-off-by: Nikos Filippakis <aesmade@gmail.com>
> ---
>  net/net.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/net/net.c b/net/net.c
> index aebf753..79e9d7c 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -710,23 +710,30 @@ ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size)
>  static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov,
>                                 int iovcnt, unsigned flags)
>  {
> -    uint8_t buf[NET_BUFSIZE];
>      uint8_t *buffer;
>      size_t offset;
> +    ssize_t ret;

With that said your comment needs to explain why you've just made the
change. I see NET_BUFSIZE is quite large so maybe this should be a
clean-up across the rest of the code-base, what's so special about this
function? Have you measured any difference in performance?

>
>      if (iovcnt == 1) {
>          buffer = iov[0].iov_base;
>          offset = iov[0].iov_len;
>      } else {
> -        buffer = buf;
> -        offset = iov_to_buf(iov, iovcnt, 0, buf, sizeof(buf));
> +        buffer = g_malloc(NET_BUFSIZE * sizeof(uint8_t));
> +        offset = iov_to_buf(iov, iovcnt, 0, buffer,
> +                            NET_BUFSIZE * sizeof(uint8_t));
>      }
>
>      if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) {
> -        return nc->info->receive_raw(nc, buffer, offset);
> +        ret = nc->info->receive_raw(nc, buffer, offset);
>      } else {
> -        return nc->info->receive(nc, buffer, offset);
> +        ret = nc->info->receive(nc, buffer, offset);
>      }
> +
> +    if (iovcnt != 1) {
> +        g_free(buffer);
> +    }

This is a short function so you can get away with it but this sort of
logic can be confusing ("The iovec count was 1 therefor I should have
allocated a buffer" vs "I have an allocated buffer"). In general you
should know the various g_* functions tolerate NULLs well so maybe you
can structure the code differently (skipping the details ;-):

    uint8_t *buffer, *dynbuf = NULL;

    if (iovcnt == 1)
    {
      buffer = ...
    } else {
      buffer = dynbuf = g_malloc(NET_BUFSIZE * sizeof(uint8_t));
      ...
    }
    ...

    g_free(dynbuf)

> +
> +    return ret;
>  }
>
>  ssize_t qemu_deliver_packet_iov(NetClientState *sender,


--
Alex Bennée

Re: [Qemu-devel] [PATCH] net.c: Moved large array in nc_sendv_compat from the stack to the heap

[Nikos Filippakis]

Thank you for your comments!

On Sun, Mar 6, 2016 at 9:47 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Nikos Filippakis <aesmade@gmail.com> writes:
>
> > Hello everyone! I am interested in getting to know the codebase a little better
> > so that I can eventually apply for a GSOC position.
> > This is my first attempt at posting a patch to a mailing list, any feedback
> > is greatly appreciated.
>
> OK first things first this sort of meta comment belongs in the cover
> letter. However for a single patch you may want to put such things below
> the --- in the commit message as that will get stripped when the
> maintainer eventually applies the patch. Otherwise your meta-comments
> will end up in the version log ;-)
>
> You'll see people use the --- area to keep version notes as patches go
> through revisions.
>

I thought that could be considered part of the cover letter, didn't
realize it would end up on the version log. Sorry about that (:

> >
> > Signed-off-by: Nikos Filippakis <aesmade@gmail.com>
> > ---
> >  net/net.c | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/net.c b/net/net.c
> > index aebf753..79e9d7c 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -710,23 +710,30 @@ ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size)
> >  static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov,
> >                                 int iovcnt, unsigned flags)
> >  {
> > -    uint8_t buf[NET_BUFSIZE];
> >      uint8_t *buffer;
> >      size_t offset;
> > +    ssize_t ret;
>
> With that said your comment needs to explain why you've just made the
> change. I see NET_BUFSIZE is quite large so maybe this should be a
> clean-up across the rest of the code-base, what's so special about this
> function? Have you measured any difference in performance?
>

This method is one of several mentioned on the wiki as having big
stack frames because of such arrays, something
someone new to the project could easily fix, either by moving it to
the heap or reducing the array size. Since further
down there is a call to memcpy with NET_BUFSIZE length, I thought I'd
just move it to the heap.

Nothing special about this method, I'm planning to do the same with
the others, just trying to get some
familiarity with the mailing list.

Besides, I'm not sure if I should put such small changes to different
files in many small commits, or a large one.

> >
> >      if (iovcnt == 1) {
> >          buffer = iov[0].iov_base;
> >          offset = iov[0].iov_len;
> >      } else {
> > -        buffer = buf;
> > -        offset = iov_to_buf(iov, iovcnt, 0, buf, sizeof(buf));
> > +        buffer = g_malloc(NET_BUFSIZE * sizeof(uint8_t));
> > +        offset = iov_to_buf(iov, iovcnt, 0, buffer,
> > +                            NET_BUFSIZE * sizeof(uint8_t));
> >      }
> >
> >      if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) {
> > -        return nc->info->receive_raw(nc, buffer, offset);
> > +        ret = nc->info->receive_raw(nc, buffer, offset);
> >      } else {
> > -        return nc->info->receive(nc, buffer, offset);
> > +        ret = nc->info->receive(nc, buffer, offset);
> >      }
> > +
> > +    if (iovcnt != 1) {
> > +        g_free(buffer);
> > +    }
>
> This is a short function so you can get away with it but this sort of
> logic can be confusing ("The iovec count was 1 therefor I should have
> allocated a buffer" vs "I have an allocated buffer"). In general you
> should know the various g_* functions tolerate NULLs well so maybe you
> can structure the code differently (skipping the details ;-):
>
>     uint8_t *buffer, *dynbuf = NULL;
>
>     if (iovcnt == 1)
>     {
>       buffer = ...
>     } else {
>       buffer = dynbuf = g_malloc(NET_BUFSIZE * sizeof(uint8_t));
>       ...
>     }
>     ...
>
>     g_free(dynbuf)
>

You're right, I didn't quite like the way I did it either. I'm
resubmitting it, hopefully fixing these mistakes.

> > +
> > +    return ret;
> >  }
> >
> >  ssize_t qemu_deliver_packet_iov(NetClientState *sender,
>
>
> --
> Alex Bennée

Re: [Qemu-devel] [PATCH] net.c: Moved large array in nc_sendv_compat from the stack to the heap

[Alex Bennée]

Nikos Filippakis <aesmade@gmail.com> writes:

> Thank you for your comments!
>
> On Sun, Mar 6, 2016 at 9:47 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Nikos Filippakis <aesmade@gmail.com> writes:
>>
>> > Hello everyone! I am interested in getting to know the codebase a little better
>> > so that I can eventually apply for a GSOC position.
>> > This is my first attempt at posting a patch to a mailing list, any feedback
>> > is greatly appreciated.
>>
>> OK first things first this sort of meta comment belongs in the cover
>> letter. However for a single patch you may want to put such things below
>> the --- in the commit message as that will get stripped when the
>> maintainer eventually applies the patch. Otherwise your meta-comments
>> will end up in the version log ;-)
>>
>> You'll see people use the --- area to keep version notes as patches go
>> through revisions.
>>
>
> I thought that could be considered part of the cover letter, didn't
> realize it would end up on the version log. Sorry about that (:

When you use git format-patch with --cover-letter to format a series of
patches you'll get exactly that. For individual patches like this one
then bellow the --- works. The fact your a potential GSOC student is
useful information to us on the list, just not in the actual commit log
in git ;-)

>
>> >
>> > Signed-off-by: Nikos Filippakis <aesmade@gmail.com>
>> > ---
>> >  net/net.c | 17 ++++++++++++-----
>> >  1 file changed, 12 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/net/net.c b/net/net.c
>> > index aebf753..79e9d7c 100644
>> > --- a/net/net.c
>> > +++ b/net/net.c
>> > @@ -710,23 +710,30 @@ ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size)
>> >  static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov,
>> >                                 int iovcnt, unsigned flags)
>> >  {
>> > -    uint8_t buf[NET_BUFSIZE];
>> >      uint8_t *buffer;
>> >      size_t offset;
>> > +    ssize_t ret;
>>
>> With that said your comment needs to explain why you've just made the
>> change. I see NET_BUFSIZE is quite large so maybe this should be a
>> clean-up across the rest of the code-base, what's so special about this
>> function? Have you measured any difference in performance?
>>
>
> This method is one of several mentioned on the wiki as having big
> stack frames because of such arrays, something
> someone new to the project could easily fix, either by moving it to
> the heap or reducing the array size. Since further
> down there is a call to memcpy with NET_BUFSIZE length, I thought I'd
> just move it to the heap.

That's fine. In fact referencing the wiki bite-sized tasks would
probably be enough context for the commit message.

> Nothing special about this method, I'm planning to do the same with
> the others, just trying to get some
> familiarity with the mailing list.

Don't worry too much, it usually takes a few attempts to get your first
patch applied and the workflow sorted out.

> Besides, I'm not sure if I should put such small changes to different
> files in many small commits, or a large one.

The key byword here is bisectability. If regressions get introduced we
want to be able to quickly identify the culprit with git-bisect. So it
is important that every commit in the project builds cleanly. For
something like this I'd argue a series of patches would make sense as
they are likely in different functional places in the code.

>
>> >
>> >      if (iovcnt == 1) {
>> >          buffer = iov[0].iov_base;
>> >          offset = iov[0].iov_len;
>> >      } else {
>> > -        buffer = buf;
>> > -        offset = iov_to_buf(iov, iovcnt, 0, buf, sizeof(buf));
>> > +        buffer = g_malloc(NET_BUFSIZE * sizeof(uint8_t));
>> > +        offset = iov_to_buf(iov, iovcnt, 0, buffer,
>> > +                            NET_BUFSIZE * sizeof(uint8_t));
>> >      }
>> >
>> >      if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) {
>> > -        return nc->info->receive_raw(nc, buffer, offset);
>> > +        ret = nc->info->receive_raw(nc, buffer, offset);
>> >      } else {
>> > -        return nc->info->receive(nc, buffer, offset);
>> > +        ret = nc->info->receive(nc, buffer, offset);
>> >      }
>> > +
>> > +    if (iovcnt != 1) {
>> > +        g_free(buffer);
>> > +    }
>>
>> This is a short function so you can get away with it but this sort of
>> logic can be confusing ("The iovec count was 1 therefor I should have
>> allocated a buffer" vs "I have an allocated buffer"). In general you
>> should know the various g_* functions tolerate NULLs well so maybe you
>> can structure the code differently (skipping the details ;-):
>>
>>     uint8_t *buffer, *dynbuf = NULL;
>>
>>     if (iovcnt == 1)
>>     {
>>       buffer = ...
>>     } else {
>>       buffer = dynbuf = g_malloc(NET_BUFSIZE * sizeof(uint8_t));
>>       ...
>>     }
>>     ...
>>
>>     g_free(dynbuf)
>>
>
> You're right, I didn't quite like the way I did it either. I'm
> resubmitting it, hopefully fixing these mistakes.

This is more a question of style rather than mistakes in the code.
However taste is a good guide, while sometimes code is as ugly as it
needs to be it is often worthwhile investigating alternatives if your
initial reaction is ambivalent.

>
>> > +
>> > +    return ret;
>> >  }
>> >
>> >  ssize_t qemu_deliver_packet_iov(NetClientState *sender,
>>
>>
>> --
>> Alex Bennée


--
Alex Bennée

Re: [Qemu-devel] [PATCH] net.c: Moved large array in nc_sendv_compat from the stack to the heap

[Paolo Bonzini]

On 05/03/2016 21:39, Nikos Filippakis wrote:
> Hello everyone! I am interested in getting to know the codebase a little better
> so that I can eventually apply for a GSOC position.
> This is my first attempt at posting a patch to a mailing list, any feedback
> is greatly appreciated.
> 
> Signed-off-by: Nikos Filippakis <aesmade@gmail.com>
> ---
>  net/net.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/net/net.c b/net/net.c
> index aebf753..79e9d7c 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -710,23 +710,30 @@ ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size)
>  static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov,
>                                 int iovcnt, unsigned flags)
>  {
> -    uint8_t buf[NET_BUFSIZE];
>      uint8_t *buffer;
>      size_t offset;
> +    ssize_t ret;
>  
>      if (iovcnt == 1) {
>          buffer = iov[0].iov_base;
>          offset = iov[0].iov_len;
>      } else {
> -        buffer = buf;
> -        offset = iov_to_buf(iov, iovcnt, 0, buf, sizeof(buf));
> +        buffer = g_malloc(NET_BUFSIZE * sizeof(uint8_t));
> +        offset = iov_to_buf(iov, iovcnt, 0, buffer,
> +                            NET_BUFSIZE * sizeof(uint8_t));
>      }

Hi Nikos,

I suspect an allocation (especially a large one) on this path can have a
performance impact.  On one hand, nc_sendv_compat is not used in
high-performance network cards (e1000 and virtio-net, for example) so
this is not a major issue.  On the other hand, this is the first such
conversion so you could set a good example.

Another possibility thus is to make the buffer smaller (e.g. 2K) and
check the size with iov_size.  If the size is at most 2K, you use the
on-stack buffer.  If the size is bigger than 2K and smaller than
NET_BUFSIZE, you go through the allocation but you only need to allocate
the correct number of bytes.  Finally, if the size is bigger than
NET_BUFSIZE you set errno = EINVAL and return -1.

Thanks,

Paolo

>      if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) {
> -        return nc->info->receive_raw(nc, buffer, offset);
> +        ret = nc->info->receive_raw(nc, buffer, offset);
>      } else {
> -        return nc->info->receive(nc, buffer, offset);
> +        ret = nc->info->receive(nc, buffer, offset);
>      }
> +
> +    if (iovcnt != 1) {
> +        g_free(buffer);
> +    }
> +
> +    return ret;
>  }
>  
>  ssize_t qemu_deliver_packet_iov(NetClientState *sender,
>