All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO
@ 2020-09-28  3:39 ` xiangxia.m.yue
  0 siblings, 0 replies; 36+ messages in thread
From: xiangxia.m.yue @ 2020-09-28  3:39 UTC (permalink / raw)
  To: jasowang, mst; +Cc: virtualization, netdev, Tonghao Zhang, Willem de Bruijn

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

Open vSwitch and Linux bridge will disable LRO of the interface
when this interface added to them. Now when disable the LRO, the
virtio-net csum is disable too. That drops the forwarding performance.

Fixes: e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 drivers/net/virtio_net.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7145c83c6c8c..21b71148c532 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
 	VIRTIO_NET_F_GUEST_CSUM
 };
 
+#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
+				(1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
+				(1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
+				(1ULL << VIRTIO_NET_F_GUEST_UFO))
+
 struct virtnet_stat_desc {
 	char desc[ETH_GSTRING_LEN];
 	size_t offset;
@@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device *dev,
 		if (features & NETIF_F_LRO)
 			offloads = vi->guest_offloads_capable;
 		else
-			offloads = 0;
+			offloads = vi->guest_offloads_capable &
+				   ~GUEST_OFFLOAD_LRO_MASK;
 
 		err = virtnet_set_guest_offloads(vi, offloads);
 		if (err)
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO
@ 2020-09-28  3:39 ` xiangxia.m.yue
  0 siblings, 0 replies; 36+ messages in thread
From: xiangxia.m.yue @ 2020-09-28  3:39 UTC (permalink / raw)
  To: jasowang, mst; +Cc: netdev, Willem de Bruijn, virtualization

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

Open vSwitch and Linux bridge will disable LRO of the interface
when this interface added to them. Now when disable the LRO, the
virtio-net csum is disable too. That drops the forwarding performance.

Fixes: e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 drivers/net/virtio_net.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7145c83c6c8c..21b71148c532 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
 	VIRTIO_NET_F_GUEST_CSUM
 };
 
+#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
+				(1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
+				(1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
+				(1ULL << VIRTIO_NET_F_GUEST_UFO))
+
 struct virtnet_stat_desc {
 	char desc[ETH_GSTRING_LEN];
 	size_t offset;
@@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device *dev,
 		if (features & NETIF_F_LRO)
 			offloads = vi->guest_offloads_capable;
 		else
-			offloads = 0;
+			offloads = vi->guest_offloads_capable &
+				   ~GUEST_OFFLOAD_LRO_MASK;
 
 		err = virtnet_set_guest_offloads(vi, offloads);
 		if (err)
-- 
2.23.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
  2020-09-28  3:39 ` xiangxia.m.yue
@ 2020-09-28  3:39   ` xiangxia.m.yue
  -1 siblings, 0 replies; 36+ messages in thread
From: xiangxia.m.yue @ 2020-09-28  3:39 UTC (permalink / raw)
  To: jasowang, mst; +Cc: virtualization, netdev, Tonghao Zhang

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;
+
+	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


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
@ 2020-09-28  3:39   ` xiangxia.m.yue
  0 siblings, 0 replies; 36+ messages in thread
From: xiangxia.m.yue @ 2020-09-28  3:39 UTC (permalink / raw)
  To: jasowang, mst; +Cc: netdev, virtualization

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;
+
+	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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO
  2020-09-28  3:39 ` xiangxia.m.yue
@ 2020-09-28  8:34   ` Willem de Bruijn
  -1 siblings, 0 replies; 36+ messages in thread
From: Willem de Bruijn @ 2020-09-28  8:34 UTC (permalink / raw)
  To: xiangxia.m.yue
  Cc: Jason Wang, Michael S. Tsirkin, virtualization, Network Development

On Mon, Sep 28, 2020 at 5:41 AM <xiangxia.m.yue@gmail.com> wrote:
>
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> Open vSwitch and Linux bridge will disable LRO of the interface
> when this interface added to them. Now when disable the LRO, the
> virtio-net csum is disable too. That drops the forwarding performance.
>
> Fixes: e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")

Patch looks fine to me, but wrong commit here?

That commit disables csum on purpose when enabling xdp with ndp_bpf.

This patch refines disabling LRO with ndo_set_features.

The relevant commit is a02e8964eaf9 ("virtio-net: ethtool configurable LRO").

If this is a fix, it should target [PATCH net] separately from the
second patch in the patchset, which is a new feature and targets
[PATCH net-next]. They can arguably target net-next together, but then
it should not have a fixes tag.

> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>  drivers/net/virtio_net.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7145c83c6c8c..21b71148c532 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
>         VIRTIO_NET_F_GUEST_CSUM
>  };
>
> +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> +                               (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> +                               (1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
> +                               (1ULL << VIRTIO_NET_F_GUEST_UFO))
> +
>  struct virtnet_stat_desc {
>         char desc[ETH_GSTRING_LEN];
>         size_t offset;
> @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device *dev,
>                 if (features & NETIF_F_LRO)
>                         offloads = vi->guest_offloads_capable;
>                 else
> -                       offloads = 0;
> +                       offloads = vi->guest_offloads_capable &
> +                                  ~GUEST_OFFLOAD_LRO_MASK;
>
>                 err = virtnet_set_guest_offloads(vi, offloads);
>                 if (err)
> --
> 2.23.0
>

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO
@ 2020-09-28  8:34   ` Willem de Bruijn
  0 siblings, 0 replies; 36+ messages in thread
From: Willem de Bruijn @ 2020-09-28  8:34 UTC (permalink / raw)
  To: xiangxia.m.yue; +Cc: Network Development, virtualization, Michael S. Tsirkin

On Mon, Sep 28, 2020 at 5:41 AM <xiangxia.m.yue@gmail.com> wrote:
>
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> Open vSwitch and Linux bridge will disable LRO of the interface
> when this interface added to them. Now when disable the LRO, the
> virtio-net csum is disable too. That drops the forwarding performance.
>
> Fixes: e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")

Patch looks fine to me, but wrong commit here?

That commit disables csum on purpose when enabling xdp with ndp_bpf.

This patch refines disabling LRO with ndo_set_features.

The relevant commit is a02e8964eaf9 ("virtio-net: ethtool configurable LRO").

If this is a fix, it should target [PATCH net] separately from the
second patch in the patchset, which is a new feature and targets
[PATCH net-next]. They can arguably target net-next together, but then
it should not have a fixes tag.

> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>  drivers/net/virtio_net.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7145c83c6c8c..21b71148c532 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
>         VIRTIO_NET_F_GUEST_CSUM
>  };
>
> +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> +                               (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> +                               (1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
> +                               (1ULL << VIRTIO_NET_F_GUEST_UFO))
> +
>  struct virtnet_stat_desc {
>         char desc[ETH_GSTRING_LEN];
>         size_t offset;
> @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device *dev,
>                 if (features & NETIF_F_LRO)
>                         offloads = vi->guest_offloads_capable;
>                 else
> -                       offloads = 0;
> +                       offloads = vi->guest_offloads_capable &
> +                                  ~GUEST_OFFLOAD_LRO_MASK;
>
>                 err = virtnet_set_guest_offloads(vi, offloads);
>                 if (err)
> --
> 2.23.0
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
  2020-09-28  3:39   ` xiangxia.m.yue
@ 2020-09-28  8:38     ` Willem de Bruijn
  -1 siblings, 0 replies; 36+ messages in thread
From: Willem de Bruijn @ 2020-09-28  8:38 UTC (permalink / raw)
  To: xiangxia.m.yue
  Cc: Jason Wang, Michael S. Tsirkin, virtualization, Network Development

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.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
@ 2020-09-28  8:38     ` Willem de Bruijn
  0 siblings, 0 replies; 36+ messages in thread
From: Willem de Bruijn @ 2020-09-28  8:38 UTC (permalink / raw)
  To: xiangxia.m.yue; +Cc: Network Development, virtualization, Michael S. Tsirkin

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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO
  2020-09-28  8:34   ` Willem de Bruijn
@ 2020-09-28  9:21     ` Tonghao Zhang
  -1 siblings, 0 replies; 36+ messages in thread
From: Tonghao Zhang @ 2020-09-28  9:21 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Jason Wang, Michael S. Tsirkin, virtualization, Network Development

On Mon, Sep 28, 2020 at 4:35 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Mon, Sep 28, 2020 at 5:41 AM <xiangxia.m.yue@gmail.com> wrote:
> >
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > Open vSwitch and Linux bridge will disable LRO of the interface
> > when this interface added to them. Now when disable the LRO, the
> > virtio-net csum is disable too. That drops the forwarding performance.
> >
> > Fixes: e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")
>
> Patch looks fine to me, but wrong commit here?
Yes, I will change the tag.
> That commit disables csum on purpose when enabling xdp with ndp_bpf.
>
> This patch refines disabling LRO with ndo_set_features.
>
> The relevant commit is a02e8964eaf9 ("virtio-net: ethtool configurable LRO").
>
> If this is a fix, it should target [PATCH net] separately from the
> second patch in the patchset, which is a new feature and targets
> [PATCH net-next]. They can arguably target net-next together, but then
> it should not have a fixes tag.
Thanks, I will send first patch for net, and next one to net-next.
>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Jason Wang <jasowang@redhat.com>
> > Cc: Willem de Bruijn <willemb@google.com>
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > ---
> >  drivers/net/virtio_net.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 7145c83c6c8c..21b71148c532 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
> >         VIRTIO_NET_F_GUEST_CSUM
> >  };
> >
> > +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > +                               (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> > +                               (1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
> > +                               (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > +
> >  struct virtnet_stat_desc {
> >         char desc[ETH_GSTRING_LEN];
> >         size_t offset;
> > @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device *dev,
> >                 if (features & NETIF_F_LRO)
> >                         offloads = vi->guest_offloads_capable;
> >                 else
> > -                       offloads = 0;
> > +                       offloads = vi->guest_offloads_capable &
> > +                                  ~GUEST_OFFLOAD_LRO_MASK;
> >
> >                 err = virtnet_set_guest_offloads(vi, offloads);
> >                 if (err)
> > --
> > 2.23.0
> >



-- 
Best regards, Tonghao

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO
@ 2020-09-28  9:21     ` Tonghao Zhang
  0 siblings, 0 replies; 36+ messages in thread
From: Tonghao Zhang @ 2020-09-28  9:21 UTC (permalink / raw)
  To: Willem de Bruijn; +Cc: Network Development, virtualization, Michael S. Tsirkin

On Mon, Sep 28, 2020 at 4:35 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Mon, Sep 28, 2020 at 5:41 AM <xiangxia.m.yue@gmail.com> wrote:
> >
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > Open vSwitch and Linux bridge will disable LRO of the interface
> > when this interface added to them. Now when disable the LRO, the
> > virtio-net csum is disable too. That drops the forwarding performance.
> >
> > Fixes: e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")
>
> Patch looks fine to me, but wrong commit here?
Yes, I will change the tag.
> That commit disables csum on purpose when enabling xdp with ndp_bpf.
>
> This patch refines disabling LRO with ndo_set_features.
>
> The relevant commit is a02e8964eaf9 ("virtio-net: ethtool configurable LRO").
>
> If this is a fix, it should target [PATCH net] separately from the
> second patch in the patchset, which is a new feature and targets
> [PATCH net-next]. They can arguably target net-next together, but then
> it should not have a fixes tag.
Thanks, I will send first patch for net, and next one to net-next.
>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Jason Wang <jasowang@redhat.com>
> > Cc: Willem de Bruijn <willemb@google.com>
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > ---
> >  drivers/net/virtio_net.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 7145c83c6c8c..21b71148c532 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
> >         VIRTIO_NET_F_GUEST_CSUM
> >  };
> >
> > +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > +                               (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> > +                               (1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
> > +                               (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > +
> >  struct virtnet_stat_desc {
> >         char desc[ETH_GSTRING_LEN];
> >         size_t offset;
> > @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device *dev,
> >                 if (features & NETIF_F_LRO)
> >                         offloads = vi->guest_offloads_capable;
> >                 else
> > -                       offloads = 0;
> > +                       offloads = vi->guest_offloads_capable &
> > +                                  ~GUEST_OFFLOAD_LRO_MASK;
> >
> >                 err = virtnet_set_guest_offloads(vi, offloads);
> >                 if (err)
> > --
> > 2.23.0
> >



-- 
Best regards, Tonghao
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
  2020-09-28  8:38     ` Willem de Bruijn
@ 2020-09-28  9:22       ` Tonghao Zhang
  -1 siblings, 0 replies; 36+ messages in thread
From: Tonghao Zhang @ 2020-09-28  9:22 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Jason Wang, Michael S. Tsirkin, virtualization, Network Development

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.



-- 
Best regards, Tonghao

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
@ 2020-09-28  9:22       ` Tonghao Zhang
  0 siblings, 0 replies; 36+ messages in thread
From: Tonghao Zhang @ 2020-09-28  9:22 UTC (permalink / raw)
  To: Willem de Bruijn; +Cc: Network Development, virtualization, Michael S. Tsirkin

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.



-- 
Best regards, Tonghao
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO
  2020-09-28  3:39 ` xiangxia.m.yue
@ 2020-09-28 19:21   ` Michael S. Tsirkin
  -1 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2020-09-28 19:21 UTC (permalink / raw)
  To: xiangxia.m.yue; +Cc: jasowang, virtualization, netdev, Willem de Bruijn

On Mon, Sep 28, 2020 at 11:39:14AM +0800, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> Open vSwitch and Linux bridge will disable LRO of the interface
> when this interface added to them. Now when disable the LRO, the
> virtio-net csum is disable too. That drops the forwarding performance.
> 
> Fixes: e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")

I am a bit confused by this tag. Did this change bring about
disabling checksum when LRO is disabled? I am not sure
I follow how ...

> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>  drivers/net/virtio_net.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7145c83c6c8c..21b71148c532 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
>  	VIRTIO_NET_F_GUEST_CSUM
>  };
>  
> +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> +				(1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> +				(1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
> +				(1ULL << VIRTIO_NET_F_GUEST_UFO))
> +
>  struct virtnet_stat_desc {
>  	char desc[ETH_GSTRING_LEN];
>  	size_t offset;
> @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device *dev,
>  		if (features & NETIF_F_LRO)
>  			offloads = vi->guest_offloads_capable;
>  		else
> -			offloads = 0;
> +			offloads = vi->guest_offloads_capable &
> +				   ~GUEST_OFFLOAD_LRO_MASK;
>  
>  		err = virtnet_set_guest_offloads(vi, offloads);
>  		if (err)
> -- 
> 2.23.0


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO
@ 2020-09-28 19:21   ` Michael S. Tsirkin
  0 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2020-09-28 19:21 UTC (permalink / raw)
  To: xiangxia.m.yue; +Cc: netdev, Willem de Bruijn, virtualization

On Mon, Sep 28, 2020 at 11:39:14AM +0800, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> Open vSwitch and Linux bridge will disable LRO of the interface
> when this interface added to them. Now when disable the LRO, the
> virtio-net csum is disable too. That drops the forwarding performance.
> 
> Fixes: e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")

I am a bit confused by this tag. Did this change bring about
disabling checksum when LRO is disabled? I am not sure
I follow how ...

> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>  drivers/net/virtio_net.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7145c83c6c8c..21b71148c532 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
>  	VIRTIO_NET_F_GUEST_CSUM
>  };
>  
> +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> +				(1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> +				(1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
> +				(1ULL << VIRTIO_NET_F_GUEST_UFO))
> +
>  struct virtnet_stat_desc {
>  	char desc[ETH_GSTRING_LEN];
>  	size_t offset;
> @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device *dev,
>  		if (features & NETIF_F_LRO)
>  			offloads = vi->guest_offloads_capable;
>  		else
> -			offloads = 0;
> +			offloads = vi->guest_offloads_capable &
> +				   ~GUEST_OFFLOAD_LRO_MASK;
>  
>  		err = virtnet_set_guest_offloads(vi, offloads);
>  		if (err)
> -- 
> 2.23.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
  2020-09-28  3:39   ` xiangxia.m.yue
@ 2020-09-28 19:25     ` Michael S. Tsirkin
  -1 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2020-09-28 19:25 UTC (permalink / raw)
  To: xiangxia.m.yue; +Cc: jasowang, virtualization, netdev

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


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
@ 2020-09-28 19:25     ` Michael S. Tsirkin
  0 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2020-09-28 19:25 UTC (permalink / raw)
  To: xiangxia.m.yue; +Cc: netdev, virtualization

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO
  2020-09-28 19:21   ` Michael S. Tsirkin
@ 2020-09-29  1:40     ` Tonghao Zhang
  -1 siblings, 0 replies; 36+ messages in thread
From: Tonghao Zhang @ 2020-09-29  1:40 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Jason Wang, virtualization, Linux Kernel Network Developers,
	Willem de Bruijn

On Tue, Sep 29, 2020 at 3:21 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Sep 28, 2020 at 11:39:14AM +0800, xiangxia.m.yue@gmail.com wrote:
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > Open vSwitch and Linux bridge will disable LRO of the interface
> > when this interface added to them. Now when disable the LRO, the
> > virtio-net csum is disable too. That drops the forwarding performance.
> >
> > Fixes: e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")
>
> I am a bit confused by this tag. Did this change bring about
> disabling checksum when LRO is disabled? I am not sure
> I follow how ...
Hi Michael
It's not right fix tag.
The commit a02e8964eaf9 ("virtio-net: ethtool configurable LRO"),
disable the csum, when we disable the LRO

> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Jason Wang <jasowang@redhat.com>
> > Cc: Willem de Bruijn <willemb@google.com>
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > ---
> >  drivers/net/virtio_net.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 7145c83c6c8c..21b71148c532 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
> >       VIRTIO_NET_F_GUEST_CSUM
> >  };
> >
> > +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > +                             (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> > +                             (1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
> > +                             (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > +
> >  struct virtnet_stat_desc {
> >       char desc[ETH_GSTRING_LEN];
> >       size_t offset;
> > @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device *dev,
> >               if (features & NETIF_F_LRO)
> >                       offloads = vi->guest_offloads_capable;
> >               else
> > -                     offloads = 0;
> > +                     offloads = vi->guest_offloads_capable &
> > +                                ~GUEST_OFFLOAD_LRO_MASK;
> >
> >               err = virtnet_set_guest_offloads(vi, offloads);
> >               if (err)
> > --
> > 2.23.0
>


-- 
Best regards, Tonghao

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO
@ 2020-09-29  1:40     ` Tonghao Zhang
  0 siblings, 0 replies; 36+ messages in thread
From: Tonghao Zhang @ 2020-09-29  1:40 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Linux Kernel Network Developers, Willem de Bruijn, virtualization

On Tue, Sep 29, 2020 at 3:21 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Sep 28, 2020 at 11:39:14AM +0800, xiangxia.m.yue@gmail.com wrote:
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > Open vSwitch and Linux bridge will disable LRO of the interface
> > when this interface added to them. Now when disable the LRO, the
> > virtio-net csum is disable too. That drops the forwarding performance.
> >
> > Fixes: e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")
>
> I am a bit confused by this tag. Did this change bring about
> disabling checksum when LRO is disabled? I am not sure
> I follow how ...
Hi Michael
It's not right fix tag.
The commit a02e8964eaf9 ("virtio-net: ethtool configurable LRO"),
disable the csum, when we disable the LRO

> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Jason Wang <jasowang@redhat.com>
> > Cc: Willem de Bruijn <willemb@google.com>
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > ---
> >  drivers/net/virtio_net.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 7145c83c6c8c..21b71148c532 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
> >       VIRTIO_NET_F_GUEST_CSUM
> >  };
> >
> > +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > +                             (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> > +                             (1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
> > +                             (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > +
> >  struct virtnet_stat_desc {
> >       char desc[ETH_GSTRING_LEN];
> >       size_t offset;
> > @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device *dev,
> >               if (features & NETIF_F_LRO)
> >                       offloads = vi->guest_offloads_capable;
> >               else
> > -                     offloads = 0;
> > +                     offloads = vi->guest_offloads_capable &
> > +                                ~GUEST_OFFLOAD_LRO_MASK;
> >
> >               err = virtnet_set_guest_offloads(vi, offloads);
> >               if (err)
> > --
> > 2.23.0
>


-- 
Best regards, Tonghao
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
  2020-09-28 19:25     ` Michael S. Tsirkin
@ 2020-09-29  1:45       ` Tonghao Zhang
  -1 siblings, 0 replies; 36+ messages in thread
From: Tonghao Zhang @ 2020-09-29  1:45 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Jason Wang, virtualization, Linux Kernel Network Developers

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
>


-- 
Best regards, Tonghao

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
@ 2020-09-29  1:45       ` Tonghao Zhang
  0 siblings, 0 replies; 36+ messages in thread
From: Tonghao Zhang @ 2020-09-29  1:45 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Linux Kernel Network Developers, virtualization

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
>


-- 
Best regards, Tonghao
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
  2020-09-29  1:45       ` Tonghao Zhang
@ 2020-09-29  5:55         ` Michael S. Tsirkin
  -1 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2020-09-29  5:55 UTC (permalink / raw)
  To: Tonghao Zhang; +Cc: Jason Wang, virtualization, Linux Kernel Network Developers

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


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
@ 2020-09-29  5:55         ` Michael S. Tsirkin
  0 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2020-09-29  5:55 UTC (permalink / raw)
  To: Tonghao Zhang; +Cc: Linux Kernel Network Developers, virtualization

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO
  2020-09-29  1:40     ` Tonghao Zhang
@ 2020-09-29  5:56       ` Michael S. Tsirkin
  -1 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2020-09-29  5:56 UTC (permalink / raw)
  To: Tonghao Zhang
  Cc: Jason Wang, virtualization, Linux Kernel Network Developers,
	Willem de Bruijn

On Tue, Sep 29, 2020 at 09:40:22AM +0800, Tonghao Zhang wrote:
> On Tue, Sep 29, 2020 at 3:21 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Sep 28, 2020 at 11:39:14AM +0800, xiangxia.m.yue@gmail.com wrote:
> > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > >
> > > Open vSwitch and Linux bridge will disable LRO of the interface
> > > when this interface added to them. Now when disable the LRO, the
> > > virtio-net csum is disable too. That drops the forwarding performance.
> > >
> > > Fixes: e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")
> >
> > I am a bit confused by this tag. Did this change bring about
> > disabling checksum when LRO is disabled? I am not sure
> > I follow how ...
> Hi Michael
> It's not right fix tag.
> The commit a02e8964eaf9 ("virtio-net: ethtool configurable LRO"),
> disable the csum, when we disable the LRO

OK then, pls send a correct Fixes tag when you repost this ...

> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Jason Wang <jasowang@redhat.com>
> > > Cc: Willem de Bruijn <willemb@google.com>
> > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > > ---
> > >  drivers/net/virtio_net.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 7145c83c6c8c..21b71148c532 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
> > >       VIRTIO_NET_F_GUEST_CSUM
> > >  };
> > >
> > > +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > > +                             (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> > > +                             (1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
> > > +                             (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > > +
> > >  struct virtnet_stat_desc {
> > >       char desc[ETH_GSTRING_LEN];
> > >       size_t offset;
> > > @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device *dev,
> > >               if (features & NETIF_F_LRO)
> > >                       offloads = vi->guest_offloads_capable;
> > >               else
> > > -                     offloads = 0;
> > > +                     offloads = vi->guest_offloads_capable &
> > > +                                ~GUEST_OFFLOAD_LRO_MASK;
> > >
> > >               err = virtnet_set_guest_offloads(vi, offloads);
> > >               if (err)
> > > --
> > > 2.23.0
> >
> 
> 
> -- 
> Best regards, Tonghao


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO
@ 2020-09-29  5:56       ` Michael S. Tsirkin
  0 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2020-09-29  5:56 UTC (permalink / raw)
  To: Tonghao Zhang
  Cc: Linux Kernel Network Developers, Willem de Bruijn, virtualization

On Tue, Sep 29, 2020 at 09:40:22AM +0800, Tonghao Zhang wrote:
> On Tue, Sep 29, 2020 at 3:21 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Sep 28, 2020 at 11:39:14AM +0800, xiangxia.m.yue@gmail.com wrote:
> > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > >
> > > Open vSwitch and Linux bridge will disable LRO of the interface
> > > when this interface added to them. Now when disable the LRO, the
> > > virtio-net csum is disable too. That drops the forwarding performance.
> > >
> > > Fixes: e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")
> >
> > I am a bit confused by this tag. Did this change bring about
> > disabling checksum when LRO is disabled? I am not sure
> > I follow how ...
> Hi Michael
> It's not right fix tag.
> The commit a02e8964eaf9 ("virtio-net: ethtool configurable LRO"),
> disable the csum, when we disable the LRO

OK then, pls send a correct Fixes tag when you repost this ...

> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Jason Wang <jasowang@redhat.com>
> > > Cc: Willem de Bruijn <willemb@google.com>
> > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > > ---
> > >  drivers/net/virtio_net.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 7145c83c6c8c..21b71148c532 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
> > >       VIRTIO_NET_F_GUEST_CSUM
> > >  };
> > >
> > > +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > > +                             (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> > > +                             (1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
> > > +                             (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > > +
> > >  struct virtnet_stat_desc {
> > >       char desc[ETH_GSTRING_LEN];
> > >       size_t offset;
> > > @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device *dev,
> > >               if (features & NETIF_F_LRO)
> > >                       offloads = vi->guest_offloads_capable;
> > >               else
> > > -                     offloads = 0;
> > > +                     offloads = vi->guest_offloads_capable &
> > > +                                ~GUEST_OFFLOAD_LRO_MASK;
> > >
> > >               err = virtnet_set_guest_offloads(vi, offloads);
> > >               if (err)
> > > --
> > > 2.23.0
> >
> 
> 
> -- 
> Best regards, Tonghao

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO
  2020-09-29  5:56       ` Michael S. Tsirkin
@ 2020-09-29  6:01         ` Tonghao Zhang
  -1 siblings, 0 replies; 36+ messages in thread
From: Tonghao Zhang @ 2020-09-29  6:01 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Jason Wang, virtualization, Linux Kernel Network Developers,
	Willem de Bruijn

On Tue, Sep 29, 2020 at 1:57 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Sep 29, 2020 at 09:40:22AM +0800, Tonghao Zhang wrote:
> > On Tue, Sep 29, 2020 at 3:21 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Sep 28, 2020 at 11:39:14AM +0800, xiangxia.m.yue@gmail.com wrote:
> > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > > >
> > > > Open vSwitch and Linux bridge will disable LRO of the interface
> > > > when this interface added to them. Now when disable the LRO, the
> > > > virtio-net csum is disable too. That drops the forwarding performance.
> > > >
> > > > Fixes: e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")
> > >
> > > I am a bit confused by this tag. Did this change bring about
> > > disabling checksum when LRO is disabled? I am not sure
> > > I follow how ...
> > Hi Michael
> > It's not right fix tag.
> > The commit a02e8964eaf9 ("virtio-net: ethtool configurable LRO"),
> > disable the csum, when we disable the LRO
>
> OK then, pls send a correct Fixes tag when you repost this ...
Hi Michael
I repost this patch, please review. thanks.
http://patchwork.ozlabs.org/project/netdev/patch/20200929015806.19171-1-xiangxia.m.yue@gmail.com/
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Jason Wang <jasowang@redhat.com>
> > > > Cc: Willem de Bruijn <willemb@google.com>
> > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > > > ---
> > > >  drivers/net/virtio_net.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 7145c83c6c8c..21b71148c532 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
> > > >       VIRTIO_NET_F_GUEST_CSUM
> > > >  };
> > > >
> > > > +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > > > +                             (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> > > > +                             (1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
> > > > +                             (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > > > +
> > > >  struct virtnet_stat_desc {
> > > >       char desc[ETH_GSTRING_LEN];
> > > >       size_t offset;
> > > > @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device *dev,
> > > >               if (features & NETIF_F_LRO)
> > > >                       offloads = vi->guest_offloads_capable;
> > > >               else
> > > > -                     offloads = 0;
> > > > +                     offloads = vi->guest_offloads_capable &
> > > > +                                ~GUEST_OFFLOAD_LRO_MASK;
> > > >
> > > >               err = virtnet_set_guest_offloads(vi, offloads);
> > > >               if (err)
> > > > --
> > > > 2.23.0
> > >
> >
> >
> > --
> > Best regards, Tonghao
>


-- 
Best regards, Tonghao

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO
@ 2020-09-29  6:01         ` Tonghao Zhang
  0 siblings, 0 replies; 36+ messages in thread
From: Tonghao Zhang @ 2020-09-29  6:01 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Linux Kernel Network Developers, Willem de Bruijn, virtualization

On Tue, Sep 29, 2020 at 1:57 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Sep 29, 2020 at 09:40:22AM +0800, Tonghao Zhang wrote:
> > On Tue, Sep 29, 2020 at 3:21 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Sep 28, 2020 at 11:39:14AM +0800, xiangxia.m.yue@gmail.com wrote:
> > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > > >
> > > > Open vSwitch and Linux bridge will disable LRO of the interface
> > > > when this interface added to them. Now when disable the LRO, the
> > > > virtio-net csum is disable too. That drops the forwarding performance.
> > > >
> > > > Fixes: e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")
> > >
> > > I am a bit confused by this tag. Did this change bring about
> > > disabling checksum when LRO is disabled? I am not sure
> > > I follow how ...
> > Hi Michael
> > It's not right fix tag.
> > The commit a02e8964eaf9 ("virtio-net: ethtool configurable LRO"),
> > disable the csum, when we disable the LRO
>
> OK then, pls send a correct Fixes tag when you repost this ...
Hi Michael
I repost this patch, please review. thanks.
http://patchwork.ozlabs.org/project/netdev/patch/20200929015806.19171-1-xiangxia.m.yue@gmail.com/
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Jason Wang <jasowang@redhat.com>
> > > > Cc: Willem de Bruijn <willemb@google.com>
> > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > > > ---
> > > >  drivers/net/virtio_net.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 7145c83c6c8c..21b71148c532 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
> > > >       VIRTIO_NET_F_GUEST_CSUM
> > > >  };
> > > >
> > > > +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > > > +                             (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> > > > +                             (1ULL << VIRTIO_NET_F_GUEST_ECN)  | \
> > > > +                             (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > > > +
> > > >  struct virtnet_stat_desc {
> > > >       char desc[ETH_GSTRING_LEN];
> > > >       size_t offset;
> > > > @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device *dev,
> > > >               if (features & NETIF_F_LRO)
> > > >                       offloads = vi->guest_offloads_capable;
> > > >               else
> > > > -                     offloads = 0;
> > > > +                     offloads = vi->guest_offloads_capable &
> > > > +                                ~GUEST_OFFLOAD_LRO_MASK;
> > > >
> > > >               err = virtnet_set_guest_offloads(vi, offloads);
> > > >               if (err)
> > > > --
> > > > 2.23.0
> > >
> >
> >
> > --
> > Best regards, Tonghao
>


-- 
Best regards, Tonghao
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
  2020-09-29  5:55         ` Michael S. Tsirkin
@ 2020-09-29  6:10           ` Tonghao Zhang
  -1 siblings, 0 replies; 36+ messages in thread
From: Tonghao Zhang @ 2020-09-29  6:10 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Jason Wang, virtualization, Linux Kernel Network Developers

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
>


-- 
Best regards, Tonghao

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
@ 2020-09-29  6:10           ` Tonghao Zhang
  0 siblings, 0 replies; 36+ messages in thread
From: Tonghao Zhang @ 2020-09-29  6:10 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Linux Kernel Network Developers, virtualization

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
>


-- 
Best regards, Tonghao
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
  2020-09-29  6:10           ` Tonghao Zhang
@ 2020-09-29  6:22             ` Michael S. Tsirkin
  -1 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2020-09-29  6:22 UTC (permalink / raw)
  To: Tonghao Zhang; +Cc: Jason Wang, virtualization, Linux Kernel Network Developers

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


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
@ 2020-09-29  6:22             ` Michael S. Tsirkin
  0 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2020-09-29  6:22 UTC (permalink / raw)
  To: Tonghao Zhang; +Cc: Linux Kernel Network Developers, virtualization

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
  2020-09-29  6:22             ` Michael S. Tsirkin
@ 2020-09-29  7:17               ` Tonghao Zhang
  -1 siblings, 0 replies; 36+ messages in thread
From: Tonghao Zhang @ 2020-09-29  7:17 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Jason Wang, virtualization, Linux Kernel Network Developers

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
>


-- 
Best regards, Tonghao

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
@ 2020-09-29  7:17               ` Tonghao Zhang
  0 siblings, 0 replies; 36+ messages in thread
From: Tonghao Zhang @ 2020-09-29  7:17 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Linux Kernel Network Developers, virtualization

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
>


-- 
Best regards, Tonghao
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
  2020-09-29  7:17               ` Tonghao Zhang
@ 2020-09-29  7:25                 ` Michael S. Tsirkin
  -1 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2020-09-29  7:25 UTC (permalink / raw)
  To: Tonghao Zhang; +Cc: Jason Wang, virtualization, Linux Kernel Network Developers

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


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
@ 2020-09-29  7:25                 ` Michael S. Tsirkin
  0 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2020-09-29  7:25 UTC (permalink / raw)
  To: Tonghao Zhang; +Cc: Linux Kernel Network Developers, virtualization

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
  2020-09-29  7:25                 ` Michael S. Tsirkin
@ 2020-09-30  2:09                   ` Tonghao Zhang
  -1 siblings, 0 replies; 36+ messages in thread
From: Tonghao Zhang @ 2020-09-30  2:09 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Jason Wang, virtualization, Linux Kernel Network Developers

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
>


-- 
Best regards, Tonghao

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
@ 2020-09-30  2:09                   ` Tonghao Zhang
  0 siblings, 0 replies; 36+ messages in thread
From: Tonghao Zhang @ 2020-09-30  2:09 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Linux Kernel Network Developers, virtualization

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
>


-- 
Best regards, Tonghao
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2020-09-30  2:11 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-28  3:39 [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO xiangxia.m.yue
2020-09-28  3:39 ` xiangxia.m.yue
2020-09-28  3:39 ` [PATCH 2/2] virtio-net: ethtool configurable RXCSUM xiangxia.m.yue
2020-09-28  3:39   ` xiangxia.m.yue
2020-09-28  8:38   ` Willem de Bruijn
2020-09-28  8:38     ` Willem de Bruijn
2020-09-28  9:22     ` Tonghao Zhang
2020-09-28  9:22       ` Tonghao Zhang
2020-09-28 19:25   ` Michael S. Tsirkin
2020-09-28 19:25     ` Michael S. Tsirkin
2020-09-29  1:45     ` Tonghao Zhang
2020-09-29  1:45       ` Tonghao Zhang
2020-09-29  5:55       ` Michael S. Tsirkin
2020-09-29  5:55         ` Michael S. Tsirkin
2020-09-29  6:10         ` Tonghao Zhang
2020-09-29  6:10           ` Tonghao Zhang
2020-09-29  6:22           ` Michael S. Tsirkin
2020-09-29  6:22             ` Michael S. Tsirkin
2020-09-29  7:17             ` Tonghao Zhang
2020-09-29  7:17               ` Tonghao Zhang
2020-09-29  7:25               ` Michael S. Tsirkin
2020-09-29  7:25                 ` Michael S. Tsirkin
2020-09-30  2:09                 ` Tonghao Zhang
2020-09-30  2:09                   ` Tonghao Zhang
2020-09-28  8:34 ` [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO Willem de Bruijn
2020-09-28  8:34   ` Willem de Bruijn
2020-09-28  9:21   ` Tonghao Zhang
2020-09-28  9:21     ` Tonghao Zhang
2020-09-28 19:21 ` Michael S. Tsirkin
2020-09-28 19:21   ` Michael S. Tsirkin
2020-09-29  1:40   ` Tonghao Zhang
2020-09-29  1:40     ` Tonghao Zhang
2020-09-29  5:56     ` Michael S. Tsirkin
2020-09-29  5:56       ` Michael S. Tsirkin
2020-09-29  6:01       ` Tonghao Zhang
2020-09-29  6:01         ` Tonghao Zhang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.