| Message ID | 1386936303-7697-5-git-send-email-v.maffione@gmail.com |
|---|---|
| State | New |
| Headers | show |
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/
2014/1/13 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. > > 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/ >
On Mon, Jan 13, 2014 at 04:11:25PM +0100, Vincenzo Maffione wrote: > 2014/1/13 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. > > > > 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? Yep. I suggest starting this patch series with a few cleanup patches to get the tap_*() interface into shape. Then you can move that cleaned-up interface to NetClient in the second half of the patch series. By doing the cleanup first, you make the second half simpler. Stefan
diff --git a/net/netmap.c b/net/netmap.c index 0ccc497..f02a574 100644 --- a/net/netmap.c +++ b/net/netmap.c @@ -54,6 +54,7 @@ typedef struct NetmapState { bool read_poll; bool write_poll; struct iovec iov[IOV_MAX]; + int vnet_hdr_len; /* Current virtio-net header length. */ } NetmapState; #define D(format, ...) \ @@ -274,7 +275,7 @@ static ssize_t netmap_receive_iov(NetClientState *nc, return iov_size(iov, iovcnt); } - i = ring->cur; + last = i = ring->cur; avail = ring->avail; if (avail < iovcnt) { @@ -394,6 +395,60 @@ static void netmap_cleanup(NetClientState *nc) s->me.fd = -1; } +/* Offloading manipulation support callbacks. + * + * Currently we are simply able to pass the virtio-net header + * throughout the VALE switch, without modifying it or interpreting it. + * For this reason we are always able to support TSO/UFO/CSUM and + * different header lengths as long as two virtio-net-header-capable + * VMs are attached to the bridge. + */ +static bool netmap_has_ufo(NetClientState *nc) +{ + return true; +} + +static int netmap_has_vnet_hdr(NetClientState *nc) +{ + return true; +} + +static int netmap_has_vnet_hdr_len(NetClientState *nc, int len) +{ + return len >= 0 && len <= NETMAP_BDG_MAX_OFFSET; +} + +static void netmap_using_vnet_hdr(NetClientState *nc, bool enable) +{ +} + +static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6, + int ecn, int ufo) +{ +} + +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->vnet_hdr_len = len; + } +} /* NetClientInfo methods */ static NetClientInfo net_netmap_info = { @@ -403,6 +458,12 @@ static NetClientInfo net_netmap_info = { .receive_iov = netmap_receive_iov, .poll = netmap_poll, .cleanup = netmap_cleanup, + .has_ufo = netmap_has_ufo, + .has_vnet_hdr = netmap_has_vnet_hdr, + .has_vnet_hdr_len = netmap_has_vnet_hdr_len, + .using_vnet_hdr = netmap_using_vnet_hdr, + .set_offload = netmap_set_offload, + .set_vnet_hdr_len = netmap_set_vnet_hdr_len, }; /* The exported init function @@ -428,6 +489,7 @@ int net_init_netmap(const NetClientOptions *opts, nc = qemu_new_net_client(&net_netmap_info, peer, "netmap", name); s = DO_UPCAST(NetmapState, nc, nc); s->me = me; + s->vnet_hdr_len = 0; netmap_read_poll(s, true); /* Initially only poll for reads. */ return 0;
Whit this patch, the netmap backend supports TSO/UFO/CSUM offloadings, and accepts the virtio-net header, similarly to what happens with TAP. The offloading callbacks in the NetClientInfo interface have been implemented. Signed-off-by: Vincenzo Maffione <v.maffione@gmail.com> --- net/netmap.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-)