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 <address@hidden> wrote:
>
>
> Nikos Filippakis <address@hidden> 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 <address@hidden>
> > ---
> >  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