netdev - Re: [PATCH net-next v2] virtio-net: ethtool configurable LRO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF=yD-+Au42TRRXLPH4o8pXcFbLkEnRYipz1YdHD0WiWSst04Q@mail.gmail.com>
Date:   Thu, 20 Dec 2018 17:40:12 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jason Wang <jasowang@...hat.com>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net-next v2] virtio-net: ethtool configurable LRO

On Thu, Dec 20, 2018 at 5:34 PM Michael S. Tsirkin <mst@...hat.com> wrote:
>
> On Thu, Dec 20, 2018 at 05:14:54PM -0500, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@...gle.com>
> >
> > Virtio-net devices negotiate LRO support with the host.
> > Display the initially negotiated state with ethtool -k.
> >
> > Also allow configuring it with ethtool -K, reusing the existing
> > virtnet_set_guest_offloads helper that configures LRO for XDP.
> > This is conditional on VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> >
> > Virtio-net negotiates TSO4 and TSO6 separately, but ethtool does not
> > distinguish between the two. Display LRO as on only if any offload
> > is active.
> >
> > RTNL is held while calling virtnet_set_features, same as on the path
> > from virtnet_xdp_set.
> >
> > Changes v1 -> v2
> >   - allow ethtool config (-K) only if VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> >   - show LRO as enabled if any LRO variant is enabled
> >   - do not allow configuration while XDP is active
> >   - differentiate current features from the capable set, to restore
> >     on XDP down only those features that were active on XDP up
> >   - move test out of VIRTIO_NET_F_CSUM/TSO branch, which is tx only
>
> This part shouldn't be in the commit log, right? Should be after "---".

I believe that David prefers to have this recorded, so that anyone
stumbling on the patch later (e.g., through git blame) understands why
we made certain choices.

> > Signed-off-by: Willem de Bruijn <willemb@...gle.com>
>
> Acked-by: Michael S. Tsirkin <mst@...hat.com>

Thanks for the super fast review!