From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32861) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWEiK-0005Wx-Tk for qemu-devel@nongnu.org; Wed, 17 Feb 2016 21:53:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aWEiH-0000HW-Lc for qemu-devel@nongnu.org; Wed, 17 Feb 2016 21:53:08 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49679) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWEiH-0000HS-E0 for qemu-devel@nongnu.org; Wed, 17 Feb 2016 21:53:05 -0500 References: <47884767b4d9df94dc2660d1aef7b6124e6d701f.1454667697.git.v.maffione@gmail.com> From: Jason Wang Message-ID: <56C531FD.5080905@redhat.com> Date: Thu, 18 Feb 2016 10:52:45 +0800 MIME-Version: 1.0 In-Reply-To: <47884767b4d9df94dc2660d1aef7b6124e6d701f.1454667697.git.v.maffione@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] net: netmap: probe netmap interface for virtio-net header List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vincenzo Maffione , qemu-devel@nongnu.org Cc: g.lettieri@iet.unipi.it, rizzo@iet.unipi.it On 02/05/2016 06:30 PM, Vincenzo Maffione wrote: > Previous implementation of has_ufo, has_vnet_hdr, has_vnet_hdr_len, etc. > did not really probe for virtio-net header support for the netmap > interface attached to the backend. These callbacks were correct for > VALE ports, but incorrect for hardware NICs, pipes, monitors, etc. > > This patch fixes the implementation to work properly with all kinds > of netmap ports. > > Signed-off-by: Vincenzo Maffione > --- > net/netmap.c | 70 ++++++++++++++++++++++++++++++++++++++---------------------- > 1 file changed, 44 insertions(+), 26 deletions(-) > > diff --git a/net/netmap.c b/net/netmap.c > index 9710321..f2dcaeb 100644 > --- a/net/netmap.c > +++ b/net/netmap.c > @@ -323,20 +323,55 @@ static void netmap_cleanup(NetClientState *nc) > } > > /* Offloading manipulation support callbacks. */ > -static bool netmap_has_ufo(NetClientState *nc) > +static int netmap_do_set_vnet_hdr_len(NetmapState *s, int len, bool err_report) Passing something like err_report usually means it was the responsibility of caller to report. So let's remove the err_report and let caller to decide based on the return value. > { > - return true; > + struct nmreq req; > + int err; > + > + /* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header > + * length for the netmap adapter associated to 's->ifname'. > + */ > + memset(&req, 0, sizeof(req)); > + pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname); > + req.nr_version = NETMAP_API; > + req.nr_cmd = NETMAP_BDG_VNET_HDR; > + req.nr_arg1 = len; > + err = ioctl(s->nmd->fd, NIOCREGIF, &req); > + if (err) { > + if (err_report) { > + error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s", > + s->ifname, strerror(errno)); > + } > + return -1; > + } > + > + /* Keep track of the current length. */ > + s->vnet_hdr_len = len; > + > + return 0; > } > > -static bool netmap_has_vnet_hdr(NetClientState *nc) > +static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len) > { > + NetmapState *s = DO_UPCAST(NetmapState, nc, nc); > + int prev_len = s->vnet_hdr_len; > + > + /* Check that we can set the new length. */ > + if (netmap_do_set_vnet_hdr_len(s, len, false)) { > + return false; > + } > + > + /* Restore the previous length. */ > + netmap_do_set_vnet_hdr_len(s, prev_len, true); > + > return true; > } > > -static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len) > +/* A netmap interface that supports virtio-net headers always > + * supports UFO, so we use this callback also for the has_ufo hook. */ > +static bool netmap_has_vnet_hdr(NetClientState *nc) > { > - return len == 0 || len == sizeof(struct virtio_net_hdr) || > - len == sizeof(struct virtio_net_hdr_mrg_rxbuf); > + return netmap_has_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr)); > } > > static void netmap_using_vnet_hdr(NetClientState *nc, bool enable) > @@ -346,25 +381,8 @@ static void netmap_using_vnet_hdr(NetClientState *nc, bool enable) > 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_VNET_HDR command to change the virtio-net header > - * length for the netmap adapter associated to 's->ifname'. > - */ > - memset(&req, 0, sizeof(req)); > - pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname); > - req.nr_version = NETMAP_API; > - req.nr_cmd = NETMAP_BDG_VNET_HDR; > - req.nr_arg1 = len; > - err = ioctl(s->nmd->fd, NIOCREGIF, &req); > - if (err) { > - error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s", > - s->ifname, strerror(errno)); > - } else { > - /* Keep track of the current length. */ > - s->vnet_hdr_len = len; > - } > + netmap_do_set_vnet_hdr_len(s, len, true); > } > > static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6, > @@ -376,7 +394,7 @@ static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6, > * enables the offloadings. > */ > if (!s->vnet_hdr_len) { > - netmap_set_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr)); > + netmap_do_set_vnet_hdr_len(s, sizeof(struct virtio_net_hdr), true); > } > } > > @@ -388,7 +406,7 @@ static NetClientInfo net_netmap_info = { > .receive_iov = netmap_receive_iov, > .poll = netmap_poll, > .cleanup = netmap_cleanup, > - .has_ufo = netmap_has_ufo, > + .has_ufo = netmap_has_vnet_hdr, This look suspicious, I'm not sure this is correct for netmap. May need a comment to explain. Usually vnet hdr does not imply ufo. > .has_vnet_hdr = netmap_has_vnet_hdr, > .has_vnet_hdr_len = netmap_has_vnet_hdr_len, > .using_vnet_hdr = netmap_using_vnet_hdr,