2014/1/13 Stefan Hajnoczi <stefa...@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/
>
--
Vincenzo Maffione
