|
From: | Vincenzo Maffione |
Subject: | Re: [Qemu-devel] [PATCH 4/5] net: add offloadings support to netmap backend |
Date: | Mon, 13 Jan 2014 16:11:25 +0100 |
On Fri, Dec 13, 2013 at 01:05:02PM +0100, Vincenzo Maffione wrote:I was trying to figure out whether it's okay for this function to be a
> +static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
> +{
> +}
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,This interface is necessary for toggling offload features at runtime,
> + int ecn, int ufo)
> +{
> +}
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()!
s/usefule/useful/
> +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. */
[Prev in Thread] | Current Thread | [Next in Thread] |