| Message ID | 20200929015806.19171-1-xiangxia.m.yue@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v2] virtio-net: don't disable guest csum when disable LRO | expand |
On Tue, Sep 29, 2020 at 09:58:06AM +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: a02e8964eaf9 ("virtio-net: ethtool configurable 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> > --- > 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
On Tue, Sep 29, 2020 at 2:23 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Sep 29, 2020 at 09:58:06AM +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: a02e8964eaf9 ("virtio-net: ethtool configurable 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> > > --- > > 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@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 >
On Tue, Sep 29, 2020 at 02:59:03PM +0800, Tonghao Zhang wrote: > On Tue, Sep 29, 2020 at 2:23 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Tue, Sep 29, 2020 at 09:58:06AM +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: a02e8964eaf9 ("virtio-net: ethtool configurable 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> > > > --- > > > 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@gmail.com/ OK then maybe keep this in a series like you did with v1. > > > 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
On Tue, Sep 29, 2020 at 9:23 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Sep 29, 2020 at 02:59:03PM +0800, Tonghao Zhang wrote: > > On Tue, Sep 29, 2020 at 2:23 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Tue, Sep 29, 2020 at 09:58:06AM +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: a02e8964eaf9 ("virtio-net: ethtool configurable 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> > > > > --- > > > > 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@gmail.com/ > > OK then maybe keep this in a series like you did with v1. If this is a fix it has to target net, unlike the other patch.
On Tue, Sep 29, 2020 at 4:00 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. 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@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> > --- > 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 >
On Tue, Sep 29, 2020 at 3:32 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Tue, Sep 29, 2020 at 4:00 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. > > 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@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> > > --- > > 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 > >
On Tue, Sep 29, 2020 at 3:29 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Tue, Sep 29, 2020 at 9:23 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Tue, Sep 29, 2020 at 02:59:03PM +0800, Tonghao Zhang wrote: > > > On Tue, Sep 29, 2020 at 2:23 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Tue, Sep 29, 2020 at 09:58:06AM +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: a02e8964eaf9 ("virtio-net: ethtool configurable 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> > > > > > --- > > > > > 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@gmail.com/ > > > > OK then maybe keep this in a series like you did with v1. > > If this is a fix it has to target net, unlike the other patch. Hi Willem, Michael The first patch v2 is for -net, can we apply it? and second patch will be sent for -net-next after discussion ? That is ok?
On Tue, Sep 29, 2020 at 9:56 AM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > On Tue, Sep 29, 2020 at 3:32 PM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > On Tue, Sep 29, 2020 at 4:00 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. > > > > 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). I had expected a pure bridging setup that disables LRO to disable GRO as well. But if not, then, indeed, the checksum needs to be verified before coalescing. Makes sense. > 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
On Tue, Sep 29, 2020 at 4:00 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: a02e8964eaf9 ("virtio-net: ethtool configurable 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> Acked-by: Willem de Bruijn <willemb@google.com>
From: xiangxia.m.yue@gmail.com Date: Tue, 29 Sep 2020 09:58:06 +0800 > 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: a02e8964eaf9 ("virtio-net: ethtool configurable 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> Applied and queued up for -stable, thank you.
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)