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

["Alex Bennée" <alex.bennee@linaro.org>]

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