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

[Vincenzo Maffione]

2014/1/13 Stefan Hajnoczi <address@hidden>

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.

Completely agree. As usual, I didn't want to change too much the existing code.
This means that I have to remove the tap_using_vnet_hdr() calls from virtio-net and vmxnet3 NICs before this patches, haven't I?

 

> +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()!

Yes, we are considering to do that, but at the moment there is no such a support.

 

> +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/

--
Vincenzo Maffione