thr3ads.net - Linux Virtualization - [PATCH 2/2] virtio-net: ethtool configurable RXCSUM [Sep 2020]

If this information is useful, please help other people find it:
Share via:

Michael S. Tsirkin

2020-Sep-29 06:22 UTC

[PATCH 2/2] virtio-net: ethtool configurable RXCSUM

On Tue, Sep 29, 2020 at 02:10:56PM +0800, Tonghao Zhang
wrote:> On Tue, Sep 29, 2020 at 1:55 PM Michael S. Tsirkin <mst at
redhat.com> wrote:
> >
> > On Tue, Sep 29, 2020 at 09:45:24AM +0800, Tonghao Zhang wrote:
> > > On Tue, Sep 29, 2020 at 3:25 AM Michael S. Tsirkin <mst at
redhat.com> wrote:
> > > >
> > > > On Mon, Sep 28, 2020 at 11:39:15AM +0800, xiangxia.m.yue at
gmail.com wrote:
> > > > > From: Tonghao Zhang <xiangxia.m.yue at gmail.com>
> > > > >
> > > > > Allow user configuring RXCSUM separately with ethtool
-K,
> > > > > reusing the existing virtnet_set_guest_offloads helper
> > > > > that configures RXCSUM for XDP. This is conditional on
> > > > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > > > >
> > > > > Cc: Michael S. Tsirkin <mst at redhat.com>
> > > > > Cc: Jason Wang <jasowang at redhat.com>
> > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue at
gmail.com>
> > > > > ---
> > > > >  drivers/net/virtio_net.c | 40
++++++++++++++++++++++++++++------------
> > > > >  1 file changed, 28 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/virtio_net.c
b/drivers/net/virtio_net.c
> > > > > index 21b71148c532..2e3af0b2c281 100644
> > > > > --- a/drivers/net/virtio_net.c
> > > > > +++ b/drivers/net/virtio_net.c
> > > > > @@ -68,6 +68,8 @@ static const unsigned long
guest_offloads[] = {
> > > > >                               (1ULL <<
VIRTIO_NET_F_GUEST_ECN)  | \
> > > > >                               (1ULL <<
VIRTIO_NET_F_GUEST_UFO))
> > > > >
> > > > > +#define GUEST_OFFLOAD_CSUM_MASK (1ULL <<
VIRTIO_NET_F_GUEST_CSUM)
> > > > > +
> > > > >  struct virtnet_stat_desc {
> > > > >       char desc[ETH_GSTRING_LEN];
> > > > >       size_t offset;
> > > > > @@ -2526,25 +2528,37 @@ static int
virtnet_set_features(struct net_device *dev,
> > > > >                               netdev_features_t
features)
> > > > >  {
> > > > >       struct virtnet_info *vi = netdev_priv(dev);
> > > > > -     u64 offloads;
> > > > > +     u64 offloads = vi->guest_offloads &
> > > > > +                    vi->guest_offloads_capable;
> > > > >       int err;
> > > > >
> > > > > -     if ((dev->features ^ features) &
NETIF_F_LRO) {
> > > > > -             if (vi->xdp_queue_pairs)
> > > > > -                     return -EBUSY;
> > > > > +     /* Don't allow configuration while XDP is
active. */
> > > > > +     if (vi->xdp_queue_pairs)
> > > > > +             return -EBUSY;
> > > > >
> > > > > +     if ((dev->features ^ features) &
NETIF_F_LRO) {
> > > > >               if (features & NETIF_F_LRO)
> > > > > -                     offloads =
vi->guest_offloads_capable;
> > > > > +                     offloads |=
GUEST_OFFLOAD_LRO_MASK;
> > > > >               else
> > > > > -                     offloads =
vi->guest_offloads_capable &
> > > > > -                               
~GUEST_OFFLOAD_LRO_MASK;
> > > > > +                     offloads &=
~GUEST_OFFLOAD_LRO_MASK;
> > > > > +     }
> > > > >
> > > > > -             err = virtnet_set_guest_offloads(vi,
offloads);
> > > > > -             if (err)
> > > > > -                     return err;
> > > > > -             vi->guest_offloads = offloads;
> > > > > +     if ((dev->features ^ features) &
NETIF_F_RXCSUM) {
> > > > > +             if (features & NETIF_F_RXCSUM)
> > > > > +                     offloads |=
GUEST_OFFLOAD_CSUM_MASK;
> > > > > +             else
> > > > > +                     offloads &=
~GUEST_OFFLOAD_CSUM_MASK;
> > > > >       }
> > > > >
> > > > > +     if (offloads == (vi->guest_offloads &
> > > > > +                      vi->guest_offloads_capable))
> > > > > +             return 0;
> > > >
> > > > Hmm, what exactly does this do?
> > > If the features(lro, rxcsum) we supported, are not changed, it is
not
> > > necessary to invoke virtnet_set_guest_offloads.
> >
> > okay, could you describe the cases where this triggers in a bit more
> > detail pls?
> Hi
> As I known,  when we run che commands show as below:
> ethtool -K eth1 sg off
> ethtool -K eth1 tso off
> 
> In that case, we will not invoke virtnet_set_guest_offloads.
How about initialization though? E.g. it looks like guest_offloads
is 0-initialized, won't this skip the first command to disable
offloads?
> > > > > +
> > > > > +     err = virtnet_set_guest_offloads(vi, offloads);
> > > > > +     if (err)
> > > > > +             return err;
> > > > > +
> > > > > +     vi->guest_offloads = offloads;
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > @@ -3013,8 +3027,10 @@ static int virtnet_probe(struct
virtio_device *vdev)
> > > > >       if (virtio_has_feature(vdev,
VIRTIO_NET_F_GUEST_TSO4) ||
> > > > >           virtio_has_feature(vdev,
VIRTIO_NET_F_GUEST_TSO6))
> > > > >               dev->features |= NETIF_F_LRO;
> > > > > -     if (virtio_has_feature(vdev,
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > > > > +     if (virtio_has_feature(vdev,
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
> > > > > +             dev->hw_features |= NETIF_F_RXCSUM;
> > > > >               dev->hw_features |= NETIF_F_LRO;
> > > > > +     }
> > > > >
> > > > >       dev->vlan_features = dev->features;
> > > > >
> > > > > --
> > > > > 2.23.0
> > > >
> > >
> > >
> > > --
> > > Best regards, Tonghao
> >
> 
> 
> -- 
> Best regards, Tonghao

Tonghao Zhang

2020-Sep-29 07:17 UTC

head link

[PATCH 2/2] virtio-net: ethtool configurable RXCSUM

On Tue, Sep 29, 2020 at 2:22 PM Michael S. Tsirkin <mst at redhat.com>
wrote:>
> On Tue, Sep 29, 2020 at 02:10:56PM +0800, Tonghao Zhang wrote:
> > On Tue, Sep 29, 2020 at 1:55 PM Michael S. Tsirkin <mst at
redhat.com> wrote:
> > >
> > > On Tue, Sep 29, 2020 at 09:45:24AM +0800, Tonghao Zhang wrote:
> > > > On Tue, Sep 29, 2020 at 3:25 AM Michael S. Tsirkin <mst
at redhat.com> wrote:
> > > > >
> > > > > On Mon, Sep 28, 2020 at 11:39:15AM +0800,
xiangxia.m.yue at gmail.com wrote:
> > > > > > From: Tonghao Zhang <xiangxia.m.yue at
gmail.com>
> > > > > >
> > > > > > Allow user configuring RXCSUM separately with
ethtool -K,
> > > > > > reusing the existing virtnet_set_guest_offloads
helper
> > > > > > that configures RXCSUM for XDP. This is
conditional on
> > > > > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > > > > >
> > > > > > Cc: Michael S. Tsirkin <mst at redhat.com>
> > > > > > Cc: Jason Wang <jasowang at redhat.com>
> > > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue at
gmail.com>
> > > > > > ---
> > > > > >  drivers/net/virtio_net.c | 40
++++++++++++++++++++++++++++------------
> > > > > >  1 file changed, 28 insertions(+), 12 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/net/virtio_net.c
b/drivers/net/virtio_net.c
> > > > > > index 21b71148c532..2e3af0b2c281 100644
> > > > > > --- a/drivers/net/virtio_net.c
> > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > @@ -68,6 +68,8 @@ static const unsigned long
guest_offloads[] = {
> > > > > >                               (1ULL <<
VIRTIO_NET_F_GUEST_ECN)  | \
> > > > > >                               (1ULL <<
VIRTIO_NET_F_GUEST_UFO))
> > > > > >
> > > > > > +#define GUEST_OFFLOAD_CSUM_MASK (1ULL <<
VIRTIO_NET_F_GUEST_CSUM)
> > > > > > +
> > > > > >  struct virtnet_stat_desc {
> > > > > >       char desc[ETH_GSTRING_LEN];
> > > > > >       size_t offset;
> > > > > > @@ -2526,25 +2528,37 @@ static int
virtnet_set_features(struct net_device *dev,
> > > > > >                               netdev_features_t
features)
> > > > > >  {
> > > > > >       struct virtnet_info *vi = netdev_priv(dev);
> > > > > > -     u64 offloads;
> > > > > > +     u64 offloads = vi->guest_offloads &
> > > > > > +                   
vi->guest_offloads_capable;
> > > > > >       int err;
> > > > > >
> > > > > > -     if ((dev->features ^ features) &
NETIF_F_LRO) {
> > > > > > -             if (vi->xdp_queue_pairs)
> > > > > > -                     return -EBUSY;
> > > > > > +     /* Don't allow configuration while XDP
is active. */
> > > > > > +     if (vi->xdp_queue_pairs)
> > > > > > +             return -EBUSY;
> > > > > >
> > > > > > +     if ((dev->features ^ features) &
NETIF_F_LRO) {
> > > > > >               if (features & NETIF_F_LRO)
> > > > > > -                     offloads =
vi->guest_offloads_capable;
> > > > > > +                     offloads |=
GUEST_OFFLOAD_LRO_MASK;
> > > > > >               else
> > > > > > -                     offloads =
vi->guest_offloads_capable &
> > > > > > -                               
~GUEST_OFFLOAD_LRO_MASK;
> > > > > > +                     offloads &=
~GUEST_OFFLOAD_LRO_MASK;
> > > > > > +     }
> > > > > >
> > > > > > -             err = virtnet_set_guest_offloads(vi,
offloads);
> > > > > > -             if (err)
> > > > > > -                     return err;
> > > > > > -             vi->guest_offloads = offloads;
> > > > > > +     if ((dev->features ^ features) &
NETIF_F_RXCSUM) {
> > > > > > +             if (features & NETIF_F_RXCSUM)
> > > > > > +                     offloads |=
GUEST_OFFLOAD_CSUM_MASK;
> > > > > > +             else
> > > > > > +                     offloads &=
~GUEST_OFFLOAD_CSUM_MASK;
> > > > > >       }
> > > > > >
> > > > > > +     if (offloads == (vi->guest_offloads &
> > > > > > +                     
vi->guest_offloads_capable))
> > > > > > +             return 0;
> > > > >
> > > > > Hmm, what exactly does this do?
> > > > If the features(lro, rxcsum) we supported, are not changed,
it is not
> > > > necessary to invoke virtnet_set_guest_offloads.
> > >
> > > okay, could you describe the cases where this triggers in a bit
more
> > > detail pls?
> > Hi
> > As I known,  when we run che commands show as below:
> > ethtool -K eth1 sg off
> > ethtool -K eth1 tso off
> >
> > In that case, we will not invoke virtnet_set_guest_offloads.
>
> How about initialization though? E.g. it looks like guest_offloads
> is 0-initialized, won't this skip the first command to disable
> offloads?I guest you mean that: if guest_offloads == 0, and run the command
"ethtool -K eth1 sg off", that will disable offload ?
In that patch
u64 offloads = vi->guest_offloads & vi->guest_offloads_capable; //
offload = 0
.....
 if (offloads == (vi->guest_offloads & vi->guest_offloads_capable)) //
if offload not changed, offload == 0, and (vi->guest_offloads &
vi->guest_offloads_capable) == 0.
        return 0;

virtnet_set_guest_offloads // that will not be invoked, so will not
disable offload
> > > > > > +
> > > > > > +     err = virtnet_set_guest_offloads(vi,
offloads);
> > > > > > +     if (err)
> > > > > > +             return err;
> > > > > > +
> > > > > > +     vi->guest_offloads = offloads;
> > > > > >       return 0;
> > > > > >  }
> > > > > >
> > > > > > @@ -3013,8 +3027,10 @@ static int
virtnet_probe(struct virtio_device *vdev)
> > > > > >       if (virtio_has_feature(vdev,
VIRTIO_NET_F_GUEST_TSO4) ||
> > > > > >           virtio_has_feature(vdev,
VIRTIO_NET_F_GUEST_TSO6))
> > > > > >               dev->features |= NETIF_F_LRO;
> > > > > > -     if (virtio_has_feature(vdev,
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > > > > > +     if (virtio_has_feature(vdev,
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
> > > > > > +             dev->hw_features |=
NETIF_F_RXCSUM;
> > > > > >               dev->hw_features |= NETIF_F_LRO;
> > > > > > +     }
> > > > > >
> > > > > >       dev->vlan_features = dev->features;
> > > > > >
> > > > > > --
> > > > > > 2.23.0
> > > > >
> > > >
> > > >
> > > > --
> > > > Best regards, Tonghao
> > >
> >
> >
> > --
> > Best regards, Tonghao
>

-- 
Best regards, Tonghao

Michael S. Tsirkin

2020-Sep-29 07:25 UTC

head link

[PATCH 2/2] virtio-net: ethtool configurable RXCSUM

On Tue, Sep 29, 2020 at 03:17:50PM +0800, Tonghao Zhang
wrote:> On Tue, Sep 29, 2020 at 2:22 PM Michael S. Tsirkin <mst at
redhat.com> wrote:
> >
> > On Tue, Sep 29, 2020 at 02:10:56PM +0800, Tonghao Zhang wrote:
> > > On Tue, Sep 29, 2020 at 1:55 PM Michael S. Tsirkin <mst at
redhat.com> wrote:
> > > >
> > > > On Tue, Sep 29, 2020 at 09:45:24AM +0800, Tonghao Zhang
wrote:
> > > > > On Tue, Sep 29, 2020 at 3:25 AM Michael S. Tsirkin
<mst at redhat.com> wrote:
> > > > > >
> > > > > > On Mon, Sep 28, 2020 at 11:39:15AM +0800,
xiangxia.m.yue at gmail.com wrote:
> > > > > > > From: Tonghao Zhang <xiangxia.m.yue at
gmail.com>
> > > > > > >
> > > > > > > Allow user configuring RXCSUM separately with
ethtool -K,
> > > > > > > reusing the existing
virtnet_set_guest_offloads helper
> > > > > > > that configures RXCSUM for XDP. This is
conditional on
> > > > > > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > > > > > >
> > > > > > > Cc: Michael S. Tsirkin <mst at
redhat.com>
> > > > > > > Cc: Jason Wang <jasowang at redhat.com>
> > > > > > > Signed-off-by: Tonghao Zhang
<xiangxia.m.yue at gmail.com>
> > > > > > > ---
> > > > > > >  drivers/net/virtio_net.c | 40
++++++++++++++++++++++++++++------------
> > > > > > >  1 file changed, 28 insertions(+), 12
deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/net/virtio_net.c
b/drivers/net/virtio_net.c
> > > > > > > index 21b71148c532..2e3af0b2c281 100644
> > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > @@ -68,6 +68,8 @@ static const unsigned long
guest_offloads[] = {
> > > > > > >                               (1ULL <<
VIRTIO_NET_F_GUEST_ECN)  | \
> > > > > > >                               (1ULL <<
VIRTIO_NET_F_GUEST_UFO))
> > > > > > >
> > > > > > > +#define GUEST_OFFLOAD_CSUM_MASK (1ULL
<< VIRTIO_NET_F_GUEST_CSUM)
> > > > > > > +
> > > > > > >  struct virtnet_stat_desc {
> > > > > > >       char desc[ETH_GSTRING_LEN];
> > > > > > >       size_t offset;
> > > > > > > @@ -2526,25 +2528,37 @@ static int
virtnet_set_features(struct net_device *dev,
> > > > > > >                              
netdev_features_t features)
> > > > > > >  {
> > > > > > >       struct virtnet_info *vi =
netdev_priv(dev);
> > > > > > > -     u64 offloads;
> > > > > > > +     u64 offloads = vi->guest_offloads
&
> > > > > > > +                   
vi->guest_offloads_capable;
> > > > > > >       int err;
> > > > > > >
> > > > > > > -     if ((dev->features ^ features) &
NETIF_F_LRO) {
> > > > > > > -             if (vi->xdp_queue_pairs)
> > > > > > > -                     return -EBUSY;
> > > > > > > +     /* Don't allow configuration while
XDP is active. */
> > > > > > > +     if (vi->xdp_queue_pairs)
> > > > > > > +             return -EBUSY;
> > > > > > >
> > > > > > > +     if ((dev->features ^ features) &
NETIF_F_LRO) {
> > > > > > >               if (features & NETIF_F_LRO)
> > > > > > > -                     offloads =
vi->guest_offloads_capable;
> > > > > > > +                     offloads |=
GUEST_OFFLOAD_LRO_MASK;
> > > > > > >               else
> > > > > > > -                     offloads =
vi->guest_offloads_capable &
> > > > > > > -                               
~GUEST_OFFLOAD_LRO_MASK;
> > > > > > > +                     offloads &=
~GUEST_OFFLOAD_LRO_MASK;
> > > > > > > +     }
> > > > > > >
> > > > > > > -             err =
virtnet_set_guest_offloads(vi, offloads);
> > > > > > > -             if (err)
> > > > > > > -                     return err;
> > > > > > > -             vi->guest_offloads =
offloads;
> > > > > > > +     if ((dev->features ^ features) &
NETIF_F_RXCSUM) {
> > > > > > > +             if (features &
NETIF_F_RXCSUM)
> > > > > > > +                     offloads |=
GUEST_OFFLOAD_CSUM_MASK;
> > > > > > > +             else
> > > > > > > +                     offloads &=
~GUEST_OFFLOAD_CSUM_MASK;
> > > > > > >       }
> > > > > > >
> > > > > > > +     if (offloads == (vi->guest_offloads
&
> > > > > > > +                     
vi->guest_offloads_capable))
> > > > > > > +             return 0;
> > > > > >
> > > > > > Hmm, what exactly does this do?
> > > > > If the features(lro, rxcsum) we supported, are not
changed, it is not
> > > > > necessary to invoke virtnet_set_guest_offloads.
> > > >
> > > > okay, could you describe the cases where this triggers in a
bit more
> > > > detail pls?
> > > Hi
> > > As I known,  when we run che commands show as below:
> > > ethtool -K eth1 sg off
> > > ethtool -K eth1 tso off
> > >
> > > In that case, we will not invoke virtnet_set_guest_offloads.
> >
> > How about initialization though? E.g. it looks like guest_offloads
> > is 0-initialized, won't this skip the first command to disable
> > offloads?
> I guest you mean that: if guest_offloads == 0, and run the command
> "ethtool -K eth1 sg off", that will disable offload ?
> In that patch
> u64 offloads = vi->guest_offloads & vi->guest_offloads_capable;
// offload = 0
> .....
>  if (offloads == (vi->guest_offloads &
vi->guest_offloads_capable)) //
> if offload not changed, offload == 0, and (vi->guest_offloads &
> vi->guest_offloads_capable) == 0.
>         return 0;
> 
> virtnet_set_guest_offloads // that will not be invoked, so will not
> disable offload

Sorry don't understand the question here.
At device init offloads are enabled, I am asking won't this skip
disabling them the first time this function is invoked.
Why are we bothering with this check? Is this called lots of
times where offloads are unchanged to make skipping the
command worthwhile?
> > > > > > > +
> > > > > > > +     err = virtnet_set_guest_offloads(vi,
offloads);
> > > > > > > +     if (err)
> > > > > > > +             return err;
> > > > > > > +
> > > > > > > +     vi->guest_offloads = offloads;
> > > > > > >       return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > @@ -3013,8 +3027,10 @@ static int
virtnet_probe(struct virtio_device *vdev)
> > > > > > >       if (virtio_has_feature(vdev,
VIRTIO_NET_F_GUEST_TSO4) ||
> > > > > > >           virtio_has_feature(vdev,
VIRTIO_NET_F_GUEST_TSO6))
> > > > > > >               dev->features |=
NETIF_F_LRO;
> > > > > > > -     if (virtio_has_feature(vdev,
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > > > > > > +     if (virtio_has_feature(vdev,
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
> > > > > > > +             dev->hw_features |=
NETIF_F_RXCSUM;
> > > > > > >               dev->hw_features |=
NETIF_F_LRO;
> > > > > > > +     }
> > > > > > >
> > > > > > >       dev->vlan_features =
dev->features;
> > > > > > >
> > > > > > > --
> > > > > > > 2.23.0
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Best regards, Tonghao
> > > >
> > >
> > >
> > > --
> > > Best regards, Tonghao
> >
> 
> 
> -- 
> Best regards, Tonghao

Seemingly Similar Threads

Search for more apparently analagous threads

Linux Virtualization - Sep 2020 - [PATCH 2/2] virtio-net: ethtool configurable RXCSUM

[PATCH 2/2] virtio-net: ethtool configurable RXCSUM

[PATCH 2/2] virtio-net: ethtool configurable RXCSUM

[PATCH 2/2] virtio-net: ethtool configurable RXCSUM

Seemingly Similar Threads