xiangxia.m.yue at gmail.com
2020-Sep-29 01:58 UTC
[PATCH net v2] virtio-net: don't disable guest csum when disable LRO
From: Tonghao Zhang <xiangxia.m.yue at 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: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
Cc: Michael S. Tsirkin <mst at redhat.com>
Cc: Jason Wang <jasowang at redhat.com>
Cc: Willem de Bruijn <willemb at google.com>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com>
---
v2:
* change the fix-tag
---
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
Michael S. Tsirkin
2020-Sep-29 06:23 UTC
[PATCH net v2] virtio-net: don't disable guest csum when disable LRO
On Tue, Sep 29, 2020 at 09:58:06AM +0800, xiangxia.m.yue at gmail.com wrote:> From: Tonghao Zhang <xiangxia.m.yue at 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: a02e8964eaf9 ("virtio-net: ethtool configurable LRO") > Cc: Michael S. Tsirkin <mst at redhat.com> > Cc: Jason Wang <jasowang at redhat.com> > Cc: Willem de Bruijn <willemb at google.com> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com> > --- > v2: > * change the fix-tag > --- > 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)) > +I think I'd rather we open-coded this, the macro is only used in one place ...> 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
Tonghao Zhang
2020-Sep-29 06:59 UTC
[PATCH net v2] virtio-net: don't disable guest csum when disable LRO
On Tue, Sep 29, 2020 at 2:23 PM Michael S. Tsirkin <mst at redhat.com> wrote:> > On Tue, Sep 29, 2020 at 09:58:06AM +0800, xiangxia.m.yue at gmail.com wrote: > > From: Tonghao Zhang <xiangxia.m.yue at 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: a02e8964eaf9 ("virtio-net: ethtool configurable LRO") > > Cc: Michael S. Tsirkin <mst at redhat.com> > > Cc: Jason Wang <jasowang at redhat.com> > > Cc: Willem de Bruijn <willemb at google.com> > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com> > > --- > > v2: > > * change the fix-tag > > --- > > 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)) > > + > > I think I'd rather we open-coded this, the macro is only > used in one place ...Yes, in this patch, it is used only in one place. But in next patch [1], we use it twice and that make the code look a bit nicer. Would we open-coded this in this patch ? [1] - http://patchwork.ozlabs.org/project/netdev/patch/20200928033915.82810-2-xiangxia.m.yue at gmail.com/> > 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
Willem de Bruijn
2020-Sep-29 07:31 UTC
[PATCH net v2] virtio-net: don't disable guest csum when disable LRO
On Tue, Sep 29, 2020 at 4:00 AM <xiangxia.m.yue at gmail.com> wrote:> > From: Tonghao Zhang <xiangxia.m.yue at 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.I had focused on the code previously. The s/w checksum verification cost is significant in a VM with traffic to local destinations. A bridge does not verify transport layer checksums OTOH?> Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO") > Cc: Michael S. Tsirkin <mst at redhat.com> > Cc: Jason Wang <jasowang at redhat.com> > Cc: Willem de Bruijn <willemb at google.com> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com> > --- > v2: > * change the fix-tag > --- > 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 >
Tonghao Zhang
2020-Sep-29 07:52 UTC
[PATCH net v2] virtio-net: don't disable guest csum when disable LRO
On Tue, Sep 29, 2020 at 3:32 PM Willem de Bruijn <willemdebruijn.kernel at gmail.com> wrote:> > On Tue, Sep 29, 2020 at 4:00 AM <xiangxia.m.yue at gmail.com> wrote: > > > > From: Tonghao Zhang <xiangxia.m.yue at 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. > > I had focused on the code previously. > > The s/w checksum verification cost is significant in a VM with traffic > to local destinations. A bridge does not verify transport layer > checksums OTOH?Hi Willem. No, think about GRO(In the GRO we don't know packets will be forwarded to other ports or to local). The call tree as below: + 5.41% secondary_startup_64 - 1.22% ret_from_fork .... net_rx_action napi_poll virtnet_poll virtnet_receive napi_gro_receive dev_gro_receive inet_gro_receive tcp4_gro_receive __skb_gro_checksum_complete skb_checksum __skb_checksum csum_partial do_csum - 1.13% do_csum $ brctl show bridge name bridge id STP enabled interfaces br0 8000.001122330001 no eth1 eth2> > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO") > > Cc: Michael S. Tsirkin <mst at redhat.com> > > Cc: Jason Wang <jasowang at redhat.com> > > Cc: Willem de Bruijn <willemb at google.com> > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com> > > --- > > v2: > > * change the fix-tag > > --- > > 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
Willem de Bruijn
2020-Sep-29 08:39 UTC
[PATCH net v2] virtio-net: don't disable guest csum when disable LRO
On Tue, Sep 29, 2020 at 4:00 AM <xiangxia.m.yue at gmail.com> wrote:> > From: Tonghao Zhang <xiangxia.m.yue at 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: a02e8964eaf9 ("virtio-net: ethtool configurable LRO") > Cc: Michael S. Tsirkin <mst at redhat.com> > Cc: Jason Wang <jasowang at redhat.com> > Cc: Willem de Bruijn <willemb at google.com> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com>Acked-by: Willem de Bruijn <willemb at google.com>