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

[Eric Blake]

On 03/14/2016 06:46 AM, Nikos Filippakis wrote:
> Allocate array in nc_sendv_compat on the heap if it is large to reduce stack 
> frame size, as stated in the BiteSizedTasks wiki page.

Please wrap your commit messages at 70 or so columns (since 'git log'
will display your text with indentation, and many people still prefer
80-column terminal windows).

> 
> Signed-off-by: Nikos Filippakis <address@hidden>
> ---
>  net/net.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/net/net.c b/net/net.c
> index b0c832e..a9be287 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -709,23 +709,41 @@ 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;
> +    const size_t STACKBUF_SIZE = 2048;
> +
> +    uint8_t *buffer, *dynbuf = NULL;
> +    uint8_t stackbuf[STACKBUF_SIZE];
> +    size_t offset, iov_len;
> +    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));
> +        iov_len = iov_size(iov, iovcnt);
> +
> +        if (iov_len <= STACKBUF_SIZE) {
> +            buffer = stackbuf;
> +        } else if (iov_len <= NET_BUFSIZE) {
> +            buffer = dynbuf = g_new(uint8_t, iov_len);
> +        } else {
> +            errno = EINVAL;
> +            return 0;

Is 0 the correct thing to return on an error?  Why not -1 or negative
errno?  Or is this a sign of a coding error, in which case it could be
an assertion instead of a special return?

> +        }
> +
> +        offset = iov_to_buf(iov, iovcnt, 0, buffer,
> +                            NET_BUFSIZE * sizeof(uint8_t));

sizeof(uint8_t) == 1, so this is simpler to write as just:

iov_to_buf(iov, iovcnt, 0, buffer, NET_BUFSIZE)

except that NET_BUFSIZE is not the right size any more.  You want to use
iov_len, which is the guaranteed length of buffer, and not NET_BUFSIZE
which might be larger than the buffer.

>      }
>  
>      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);
>      }
> +
> +    g_free(dynbuf);
> +
> +    return ret;
>  }
>  
>  ssize_t qemu_deliver_packet_iov(NetClientState *sender,
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature