Re: [Qemu-devel] [PATCH 4/5] net: add offloadings support to netmap backend

[Stefan Hajnoczi <stefanha@gmail.com>]

On Fri, Dec 13, 2013 at 01:05:02PM +0100, Vincenzo Maffione wrote:
> +static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
> +{
> +}

I was trying to figure out whether it's okay for this function to be a
nop.  I've come to the conclusion that it's okay:

If the netdev supports vnet_hdr then we enable vnet_hdr.  If not, it
will not enable it.

Other NICs never enable vnet_hdr even if the netdev supports it.

This interface is basically useless because set_vnet_hdr_len() is
also needed and provides the same information, i.e. that we are
transitioning from vnet_hdr off -> on.

The bool argument is misleading since we never use this function to
disable vnet_hdr.  It's impossible to transition on -> off.

I suggest removing using_vnet_hdr() and instead relying solely on
set_vnet_hdr_len().  Do this as the first patch in the series before
introducing the function pointers, that way all your following patches
become simpler.

> +static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
> +                               int ecn, int ufo)
> +{
> +}

This interface is necessary for toggling offload features at runtime,
e.g. because ethtool was used inside the guest.  Offloads can impact
performance and sometimes expose bugs.  Therefore users may wish to
disable certain offloads.

Please consider supporting set_offload()!

> +static void netmap_set_vnet_hdr_len(NetClientState *nc, int len)
> +{
> +    NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
> +    int err;
> +    struct nmreq req;
> +
> +    /* Issue a NETMAP_BDG_OFFSET command to change the netmap adapter
> +       offset of 'me->ifname'. */
> +    memset(&req, 0, sizeof(req));
> +    pstrcpy(req.nr_name, sizeof(req.nr_name), s->me.ifname);
> +    req.nr_version = NETMAP_API;
> +    req.nr_cmd = NETMAP_BDG_OFFSET;
> +    req.nr_arg1 = len;
> +    err = ioctl(s->me.fd, NIOCREGIF, &req);
> +    if (err) {
> +        error_report("Unable to execute NETMAP_BDG_OFFSET on %s: %s",
> +                     s->me.ifname, strerror(errno));
> +    } else {
> +        /* Keep track of the current length, may be usefule in the future. */

s/usefule/useful/