On 02/23/2016 04:46 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 <v.maffi...@gmail.com>
Looks good overall, just few nits, see below.
Thanks
> ---
> net/netmap.c | 62
> +++++++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 40 insertions(+), 22 deletions(-)
>
> diff --git a/net/netmap.c b/net/netmap.c
> index 9710321..ef64be0 100644
> --- a/net/netmap.c
> +++ b/net/netmap.c
> @@ -323,20 +323,51 @@ 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)
> {
Like tap, how about rename this function to netmap_fd_set_vnet_hdr_len?
> - 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) {
> + return err;
> + }
> +
> + /* Keep track of the current length. */
> + s->vnet_hdr_len = len;
How about move this to netmap_set_vnet_hdr_len() to let this function
only cares about ioctl?
> +
> + 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)) {
> + return false;
> + }
> +
> + /* Restore the previous length. */
> + netmap_do_set_vnet_hdr_len(s, prev_len);
Do we need to check and abort() if it fails like tap?
> +
> 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)
> @@ -347,23 +378,11 @@ 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);
> + err = netmap_do_set_vnet_hdr_len(s, len);
> 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;
> }
> }
>
> @@ -373,8 +392,7 @@ static void netmap_set_offload(NetClientState *nc, int
> csum, int tso4, int tso6,
> NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
>
> /* Setting a virtio-net header length greater than zero automatically
> - * enables the offloadings.
> - */
> + * enables the offloadings. */
> if (!s->vnet_hdr_len) {
> netmap_set_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
> }
> @@ -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,
> .has_vnet_hdr = netmap_has_vnet_hdr,
> .has_vnet_hdr_len = netmap_has_vnet_hdr_len,
> .using_vnet_hdr = netmap_using_vnet_hdr,
