| Message ID | 20200928033915.82810-2-xiangxia.m.yue@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/2] virtio-net: don't disable guest csum when disable LRO | expand |
On Mon, Sep 28, 2020 at 5:42 AM <xiangxia.m.yue@gmail.com> wrote: > > From: Tonghao Zhang <xiangxia.m.yue@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@redhat.com> > Cc: Jason Wang <jasowang@redhat.com> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@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; > } LRO requires receive checksum offload: packets must have their checksum verified prior to coalescing. The two features can thus not be configured fully independently.
On Mon, Sep 28, 2020 at 4:39 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Mon, Sep 28, 2020 at 5:42 AM <xiangxia.m.yue@gmail.com> wrote: > > > > From: Tonghao Zhang <xiangxia.m.yue@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@redhat.com> > > Cc: Jason Wang <jasowang@redhat.com> > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@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; > > } > > LRO requires receive checksum offload: packets must have their > checksum verified prior to coalescing. Oh, sorry for that, I will change that patch. thanks! > The two features can thus not be configured fully independently.
On Mon, Sep 28, 2020 at 11:39:15AM +0800, xiangxia.m.yue@gmail.com wrote: > From: Tonghao Zhang <xiangxia.m.yue@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@redhat.com> > Cc: Jason Wang <jasowang@redhat.com> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@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? > + > + 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
On Tue, Sep 29, 2020 at 3:25 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Sep 28, 2020 at 11:39:15AM +0800, xiangxia.m.yue@gmail.com wrote: > > From: Tonghao Zhang <xiangxia.m.yue@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@redhat.com> > > Cc: Jason Wang <jasowang@redhat.com> > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@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. > > + > > + 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 >
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@redhat.com> wrote: > > > > On Mon, Sep 28, 2020 at 11:39:15AM +0800, xiangxia.m.yue@gmail.com wrote: > > > From: Tonghao Zhang <xiangxia.m.yue@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@redhat.com> > > > Cc: Jason Wang <jasowang@redhat.com> > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@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? > > > + > > > + 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
On Tue, Sep 29, 2020 at 1:55 PM Michael S. Tsirkin <mst@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@redhat.com> wrote: > > > > > > On Mon, Sep 28, 2020 at 11:39:15AM +0800, xiangxia.m.yue@gmail.com wrote: > > > > From: Tonghao Zhang <xiangxia.m.yue@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@redhat.com> > > > > Cc: Jason Wang <jasowang@redhat.com> > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@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. > > > > + > > > > + 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 >
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@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@redhat.com> wrote: > > > > > > > > On Mon, Sep 28, 2020 at 11:39:15AM +0800, xiangxia.m.yue@gmail.com wrote: > > > > > From: Tonghao Zhang <xiangxia.m.yue@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@redhat.com> > > > > > Cc: Jason Wang <jasowang@redhat.com> > > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@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
On Tue, Sep 29, 2020 at 2:22 PM Michael S. Tsirkin <mst@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@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@redhat.com> wrote: > > > > > > > > > > On Mon, Sep 28, 2020 at 11:39:15AM +0800, xiangxia.m.yue@gmail.com wrote: > > > > > > From: Tonghao Zhang <xiangxia.m.yue@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@redhat.com> > > > > > > Cc: Jason Wang <jasowang@redhat.com> > > > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@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 >
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@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@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@redhat.com> wrote: > > > > > > > > > > > > On Mon, Sep 28, 2020 at 11:39:15AM +0800, xiangxia.m.yue@gmail.com wrote: > > > > > > > From: Tonghao Zhang <xiangxia.m.yue@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@redhat.com> > > > > > > > Cc: Jason Wang <jasowang@redhat.com> > > > > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@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
On Tue, Sep 29, 2020 at 3:25 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > 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@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@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@redhat.com> wrote: > > > > > > > > > > > > > > On Mon, Sep 28, 2020 at 11:39:15AM +0800, xiangxia.m.yue@gmail.com wrote: > > > > > > > > From: Tonghao Zhang <xiangxia.m.yue@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@redhat.com> > > > > > > > > Cc: Jason Wang <jasowang@redhat.com> > > > > > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@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? Hi Michael I remove the check and when rxcum is disabled, LRO also is disabled (suggested by Willem de Bruijn) please review, thanks. http://patchwork.ozlabs.org/project/netdev/patch/20200930020300.62245-1-xiangxia.m.yue@gmail.com/ > > > > > > > > + > > > > > > > > + 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 >
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; + + 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;