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

[Paolo Bonzini <pbonzini@redhat.com>]

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