| Message ID | 20230619105738.117733-4-hengqi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio-net: avoid XDP and _F_GUEST_CSUM | expand |
On Mon, Jun 19, 2023 at 06:57:37PM +0800, Heng Qi wrote: > We are now re-probing the csum related fields and trying > to have XDP and RX hw checksum capabilities coexist on the > XDP path. For the benefit of: > 1. RX hw checksum capability can be used if XDP is loaded. > 2. Avoid packet loss when loading XDP in the vm-vm scenario. > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > --- > drivers/net/virtio_net.c | 36 ++++++++++++++++++++++++------------ > 1 file changed, 24 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 07b4801d689c..25b486ab74db 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > struct net_device *dev = vi->dev; > struct sk_buff *skb; > struct virtio_net_hdr_mrg_rxbuf *hdr; > + __u8 flags; > > if (unlikely(len < vi->hdr_len + ETH_HLEN)) { > pr_debug("%s: short packet %i\n", dev->name, len); > @@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > return; > } > > + flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags; > + > if (vi->mergeable_rx_bufs) > skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit, > stats); what's going on here? > @@ -1728,19 +1731,28 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > if (unlikely(!skb)) > return; > > - hdr = skb_vnet_hdr(skb); > - if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report) > - virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb); > - > - if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID) > - skb->ip_summed = CHECKSUM_UNNECESSARY; > + if (unlikely(vi->xdp_enabled)) { > + if (virtnet_set_csum_after_xdp(vi, skb, flags) < 0) { > + pr_debug("%s: errors occurred in flow dissector setting csum", > + dev->name); > + goto frame_err; > + } > > - if (virtio_net_hdr_to_skb(skb, &hdr->hdr, > - virtio_is_little_endian(vi->vdev))) { > - net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n", > - dev->name, hdr->hdr.gso_type, > - hdr->hdr.gso_size); > - goto frame_err; > + } else { > + hdr = skb_vnet_hdr(skb); > + if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report) > + virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb); > + > + if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID) > + skb->ip_summed = CHECKSUM_UNNECESSARY; > + > + if (virtio_net_hdr_to_skb(skb, &hdr->hdr, > + virtio_is_little_endian(vi->vdev))) { > + net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n", > + dev->name, hdr->hdr.gso_type, > + hdr->hdr.gso_size); > + goto frame_err; > + } > } > > skb_record_rx_queue(skb, vq2rxq(rq->vq)); > -- > 2.19.1.6.gb485710b
On Mon, Jun 19, 2023 at 07:26:44AM -0400, Michael S. Tsirkin wrote: > On Mon, Jun 19, 2023 at 06:57:37PM +0800, Heng Qi wrote: > > We are now re-probing the csum related fields and trying > > to have XDP and RX hw checksum capabilities coexist on the > > XDP path. For the benefit of: > > 1. RX hw checksum capability can be used if XDP is loaded. > > 2. Avoid packet loss when loading XDP in the vm-vm scenario. > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > --- > > drivers/net/virtio_net.c | 36 ++++++++++++++++++++++++------------ > > 1 file changed, 24 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 07b4801d689c..25b486ab74db 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > struct net_device *dev = vi->dev; > > struct sk_buff *skb; > > struct virtio_net_hdr_mrg_rxbuf *hdr; > > + __u8 flags; > > > > if (unlikely(len < vi->hdr_len + ETH_HLEN)) { > > pr_debug("%s: short packet %i\n", dev->name, len); > > @@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > return; > > } > > > > + flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags; > > + > > if (vi->mergeable_rx_bufs) > > skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit, > > stats); > > what's going on here? Thanks for pointing this out. Will insert into mergeable and small modes respectively. > > > @@ -1728,19 +1731,28 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > if (unlikely(!skb)) > > return; > > > > - hdr = skb_vnet_hdr(skb); > > - if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report) > > - virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb); > > - > > - if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID) > > - skb->ip_summed = CHECKSUM_UNNECESSARY; > > + if (unlikely(vi->xdp_enabled)) { > > + if (virtnet_set_csum_after_xdp(vi, skb, flags) < 0) { > > + pr_debug("%s: errors occurred in flow dissector setting csum", > > + dev->name); > > + goto frame_err; > > + } > > > > - if (virtio_net_hdr_to_skb(skb, &hdr->hdr, > > - virtio_is_little_endian(vi->vdev))) { > > - net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n", > > - dev->name, hdr->hdr.gso_type, > > - hdr->hdr.gso_size); > > - goto frame_err; > > + } else { > > + hdr = skb_vnet_hdr(skb); > > + if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report) > > + virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb); > > + > > + if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID) > > + skb->ip_summed = CHECKSUM_UNNECESSARY; > > + > > + if (virtio_net_hdr_to_skb(skb, &hdr->hdr, > > + virtio_is_little_endian(vi->vdev))) { > > + net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n", > > + dev->name, hdr->hdr.gso_type, > > + hdr->hdr.gso_size); > > + goto frame_err; > > + } > > } > > > > skb_record_rx_queue(skb, vq2rxq(rq->vq)); > > -- > > 2.19.1.6.gb485710b
On Mon, Jun 19, 2023 at 07:26:44AM -0400, Michael S. Tsirkin wrote: > On Mon, Jun 19, 2023 at 06:57:37PM +0800, Heng Qi wrote: > > We are now re-probing the csum related fields and trying > > to have XDP and RX hw checksum capabilities coexist on the > > XDP path. For the benefit of: > > 1. RX hw checksum capability can be used if XDP is loaded. > > 2. Avoid packet loss when loading XDP in the vm-vm scenario. > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > --- > > drivers/net/virtio_net.c | 36 ++++++++++++++++++++++++------------ > > 1 file changed, 24 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 07b4801d689c..25b486ab74db 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > struct net_device *dev = vi->dev; > > struct sk_buff *skb; > > struct virtio_net_hdr_mrg_rxbuf *hdr; > > + __u8 flags; > > > > if (unlikely(len < vi->hdr_len + ETH_HLEN)) { > > pr_debug("%s: short packet %i\n", dev->name, len); > > @@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > return; > > } > > > > + flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags; > > + > > if (vi->mergeable_rx_bufs) > > skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit, > > stats); > > what's going on here? Hi, Michael. Is your question about the function of this code? 1. If yes, this sentence saves the flags value in virtio-net-hdr in advance before entering the XDP processing logic, so that it can be used to judge further logic after XDP processing. If _NEEDS_CSUM is included in flags before XDP processing, then after XDP processing we need to re-probe the csum fields and calculate the pseudo-header checksum. 2. If not, do you mean that mergeable and small modes should be distinguished for save actions? The answer is that we don't need it. The information in virtio-net-hdr in the current small mode is also forcibly converted into the virtio_net_hdr_mrg_rxbuf structure, which is consistent with the virtio_net_hdr structure in terms of code layout, and the results are consistent. If you think this is semantically wrong, then we need a bugfix patch. The simplest example is in receive_small_xdp(): " unsigned int header_offset = VIRTNET_RX_PAD + xdp_headroom; ... struct virtio_net_hdr_mrg_rxbuf *hdr = buf + header_offset; " Thanks. > > > @@ -1728,19 +1731,28 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > if (unlikely(!skb)) > > return; > > > > - hdr = skb_vnet_hdr(skb); > > - if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report) > > - virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb); > > - > > - if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID) > > - skb->ip_summed = CHECKSUM_UNNECESSARY; > > + if (unlikely(vi->xdp_enabled)) { > > + if (virtnet_set_csum_after_xdp(vi, skb, flags) < 0) { > > + pr_debug("%s: errors occurred in flow dissector setting csum", > > + dev->name); > > + goto frame_err; > > + } > > > > - if (virtio_net_hdr_to_skb(skb, &hdr->hdr, > > - virtio_is_little_endian(vi->vdev))) { > > - net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n", > > - dev->name, hdr->hdr.gso_type, > > - hdr->hdr.gso_size); > > - goto frame_err; > > + } else { > > + hdr = skb_vnet_hdr(skb); > > + if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report) > > + virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb); > > + > > + if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID) > > + skb->ip_summed = CHECKSUM_UNNECESSARY; > > + > > + if (virtio_net_hdr_to_skb(skb, &hdr->hdr, > > + virtio_is_little_endian(vi->vdev))) { > > + net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n", > > + dev->name, hdr->hdr.gso_type, > > + hdr->hdr.gso_size); > > + goto frame_err; > > + } > > } > > > > skb_record_rx_queue(skb, vq2rxq(rq->vq)); > > -- > > 2.19.1.6.gb485710b
On Tue, Jun 20, 2023 at 11:24:30AM +0800, Heng Qi wrote: > On Mon, Jun 19, 2023 at 07:26:44AM -0400, Michael S. Tsirkin wrote: > > On Mon, Jun 19, 2023 at 06:57:37PM +0800, Heng Qi wrote: > > > We are now re-probing the csum related fields and trying > > > to have XDP and RX hw checksum capabilities coexist on the > > > XDP path. For the benefit of: > > > 1. RX hw checksum capability can be used if XDP is loaded. > > > 2. Avoid packet loss when loading XDP in the vm-vm scenario. > > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > --- > > > drivers/net/virtio_net.c | 36 ++++++++++++++++++++++++------------ > > > 1 file changed, 24 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index 07b4801d689c..25b486ab74db 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > > struct net_device *dev = vi->dev; > > > struct sk_buff *skb; > > > struct virtio_net_hdr_mrg_rxbuf *hdr; > > > + __u8 flags; > > > > > > if (unlikely(len < vi->hdr_len + ETH_HLEN)) { > > > pr_debug("%s: short packet %i\n", dev->name, len); > > > @@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > > return; > > > } > > > > > > + flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags; > > > + > > > if (vi->mergeable_rx_bufs) > > > skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit, > > > stats); > > > > what's going on here? > > Hi, Michael. > > Is your question about the function of this code? > 1. If yes, > this sentence saves the flags value in virtio-net-hdr in advance > before entering the XDP processing logic, so that it can be used to > judge further logic after XDP processing. > > If _NEEDS_CSUM is included in flags before XDP processing, then after > XDP processing we need to re-probe the csum fields and calculate the > pseudo-header checksum. Yes but we previously used this: - hdr = skb_vnet_hdr(skb); which pokes at the copy in skb cb. Is anything wrong with this? It seems preferable not to poke at the header an extra time.
On Tue, Jun 20, 2023 at 06:50:34AM -0400, Michael S. Tsirkin wrote: > On Tue, Jun 20, 2023 at 11:24:30AM +0800, Heng Qi wrote: > > On Mon, Jun 19, 2023 at 07:26:44AM -0400, Michael S. Tsirkin wrote: > > > On Mon, Jun 19, 2023 at 06:57:37PM +0800, Heng Qi wrote: > > > > We are now re-probing the csum related fields and trying > > > > to have XDP and RX hw checksum capabilities coexist on the > > > > XDP path. For the benefit of: > > > > 1. RX hw checksum capability can be used if XDP is loaded. > > > > 2. Avoid packet loss when loading XDP in the vm-vm scenario. > > > > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > > --- > > > > drivers/net/virtio_net.c | 36 ++++++++++++++++++++++++------------ > > > > 1 file changed, 24 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > index 07b4801d689c..25b486ab74db 100644 > > > > --- a/drivers/net/virtio_net.c > > > > +++ b/drivers/net/virtio_net.c > > > > @@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > > > struct net_device *dev = vi->dev; > > > > struct sk_buff *skb; > > > > struct virtio_net_hdr_mrg_rxbuf *hdr; > > > > + __u8 flags; > > > > > > > > if (unlikely(len < vi->hdr_len + ETH_HLEN)) { > > > > pr_debug("%s: short packet %i\n", dev->name, len); > > > > @@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > > > return; > > > > } > > > > > > > > + flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags; > > > > + > > > > if (vi->mergeable_rx_bufs) > > > > skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit, > > > > stats); > > > > > > what's going on here? > > > > Hi, Michael. > > > > Is your question about the function of this code? > > 1. If yes, > > this sentence saves the flags value in virtio-net-hdr in advance > > before entering the XDP processing logic, so that it can be used to > > judge further logic after XDP processing. > > > > If _NEEDS_CSUM is included in flags before XDP processing, then after > > XDP processing we need to re-probe the csum fields and calculate the > > pseudo-header checksum. > > Yes but we previously used this: > - hdr = skb_vnet_hdr(skb); > which pokes at the copy in skb cb. > > Is anything wrong with this? > This is where we save the hdr when there is no XDP loaded (note that this is the complete hdr, including flags, and also including GSO and other information). When XDP is loaded, because hdr is invalid, we will not save it into skb->cb. But the above situation is not what we want. Now our purpose is to save the hdr information before XDP processing, that is, when the driver has just received the packet and has not built the skb (in fact, we only need flags). Therefore, only flags are saved here. Thanks. > It seems preferable not to poke at the header an extra time. > > -- > MST
On Tue, Jun 20, 2023 at 07:01:48PM +0800, Heng Qi wrote: > On Tue, Jun 20, 2023 at 06:50:34AM -0400, Michael S. Tsirkin wrote: > > On Tue, Jun 20, 2023 at 11:24:30AM +0800, Heng Qi wrote: > > > On Mon, Jun 19, 2023 at 07:26:44AM -0400, Michael S. Tsirkin wrote: > > > > On Mon, Jun 19, 2023 at 06:57:37PM +0800, Heng Qi wrote: > > > > > We are now re-probing the csum related fields and trying > > > > > to have XDP and RX hw checksum capabilities coexist on the > > > > > XDP path. For the benefit of: > > > > > 1. RX hw checksum capability can be used if XDP is loaded. > > > > > 2. Avoid packet loss when loading XDP in the vm-vm scenario. > > > > > > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > > > --- > > > > > drivers/net/virtio_net.c | 36 ++++++++++++++++++++++++------------ > > > > > 1 file changed, 24 insertions(+), 12 deletions(-) > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > > index 07b4801d689c..25b486ab74db 100644 > > > > > --- a/drivers/net/virtio_net.c > > > > > +++ b/drivers/net/virtio_net.c > > > > > @@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > > > > struct net_device *dev = vi->dev; > > > > > struct sk_buff *skb; > > > > > struct virtio_net_hdr_mrg_rxbuf *hdr; > > > > > + __u8 flags; > > > > > > > > > > if (unlikely(len < vi->hdr_len + ETH_HLEN)) { > > > > > pr_debug("%s: short packet %i\n", dev->name, len); > > > > > @@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > > > > return; > > > > > } > > > > > > > > > > + flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags; > > > > > + > > > > > if (vi->mergeable_rx_bufs) > > > > > skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit, > > > > > stats); > > > > > > > > what's going on here? > > > > > > Hi, Michael. > > > > > > Is your question about the function of this code? > > > 1. If yes, > > > this sentence saves the flags value in virtio-net-hdr in advance > > > before entering the XDP processing logic, so that it can be used to > > > judge further logic after XDP processing. > > > > > > If _NEEDS_CSUM is included in flags before XDP processing, then after > > > XDP processing we need to re-probe the csum fields and calculate the > > > pseudo-header checksum. > > > > Yes but we previously used this: > > - hdr = skb_vnet_hdr(skb); > > which pokes at the copy in skb cb. > > > > Is anything wrong with this? > > > > This is where we save the hdr when there is no XDP loaded (note that > this is the complete hdr, including flags, and also including GSO and > other information). When XDP is loaded, because hdr is invalid, we will > not save it into skb->cb. > > But the above situation is not what we want. Now our purpose is to save > the hdr information before XDP processing, that is, when the driver has > just received the packet and has not built the skb (in fact, we only > need flags). Therefore, only flags are saved here. > > Thanks. I don't get it. this seems to be the only use of flags: + if (unlikely(vi->xdp_enabled)) { + if (virtnet_set_csum_after_xdp(vi, skb, flags) < 0) { + pr_debug("%s: errors occurred in flow dissector setting csum", + dev->name); + goto frame_err; + } looks like skb has already been created here. is there another use of flags that I missed? > > It seems preferable not to poke at the header an extra time. > > > > -- > > MST
On Tue, Jun 20, 2023 at 08:10:38AM -0400, Michael S. Tsirkin wrote: > On Tue, Jun 20, 2023 at 07:01:48PM +0800, Heng Qi wrote: > > On Tue, Jun 20, 2023 at 06:50:34AM -0400, Michael S. Tsirkin wrote: > > > On Tue, Jun 20, 2023 at 11:24:30AM +0800, Heng Qi wrote: > > > > On Mon, Jun 19, 2023 at 07:26:44AM -0400, Michael S. Tsirkin wrote: > > > > > On Mon, Jun 19, 2023 at 06:57:37PM +0800, Heng Qi wrote: > > > > > > We are now re-probing the csum related fields and trying > > > > > > to have XDP and RX hw checksum capabilities coexist on the > > > > > > XDP path. For the benefit of: > > > > > > 1. RX hw checksum capability can be used if XDP is loaded. > > > > > > 2. Avoid packet loss when loading XDP in the vm-vm scenario. > > > > > > > > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > > > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > > > > --- > > > > > > drivers/net/virtio_net.c | 36 ++++++++++++++++++++++++------------ > > > > > > 1 file changed, 24 insertions(+), 12 deletions(-) > > > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > > > index 07b4801d689c..25b486ab74db 100644 > > > > > > --- a/drivers/net/virtio_net.c > > > > > > +++ b/drivers/net/virtio_net.c > > > > > > @@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > > > > > struct net_device *dev = vi->dev; > > > > > > struct sk_buff *skb; > > > > > > struct virtio_net_hdr_mrg_rxbuf *hdr; > > > > > > + __u8 flags; > > > > > > > > > > > > if (unlikely(len < vi->hdr_len + ETH_HLEN)) { > > > > > > pr_debug("%s: short packet %i\n", dev->name, len); > > > > > > @@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > > > > > return; > > > > > > } > > > > > > > > > > > > + flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags; > > > > > > + > > > > > > if (vi->mergeable_rx_bufs) > > > > > > skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit, > > > > > > stats); > > > > > > > > > > what's going on here? > > > > > > > > Hi, Michael. > > > > > > > > Is your question about the function of this code? > > > > 1. If yes, > > > > this sentence saves the flags value in virtio-net-hdr in advance > > > > before entering the XDP processing logic, so that it can be used to > > > > judge further logic after XDP processing. > > > > > > > > If _NEEDS_CSUM is included in flags before XDP processing, then after > > > > XDP processing we need to re-probe the csum fields and calculate the > > > > pseudo-header checksum. > > > > > > Yes but we previously used this: > > > - hdr = skb_vnet_hdr(skb); > > > which pokes at the copy in skb cb. > > > > > > Is anything wrong with this? > > > > > > > This is where we save the hdr when there is no XDP loaded (note that > > this is the complete hdr, including flags, and also including GSO and > > other information). When XDP is loaded, because hdr is invalid, we will > > not save it into skb->cb. > > > > But the above situation is not what we want. Now our purpose is to save > > the hdr information before XDP processing, that is, when the driver has > > just received the packet and has not built the skb (in fact, we only > > need flags). Therefore, only flags are saved here. > > > > Thanks. > > I don't get it. this seems to be the only use of flags: > > > + if (unlikely(vi->xdp_enabled)) { > + if (virtnet_set_csum_after_xdp(vi, skb, flags) < 0) { > + pr_debug("%s: errors occurred in flow dissector setting csum", > + dev->name); > + goto frame_err; > + } > > looks like skb has already been created here. I explain more: First, this patchset only focuses on XDP loaded scenes. Then in the same-host vm-vm scenario, when the receiver loads XDP, this is the packet receiving process: 1. The driver receives a packet, represented by *buf. 2. The XDP program builds xdp_buff based on *buf. virtio_net_hdr_mrg_rxbuf is located in the headroom of xdp_buff. 3. The XDP program returns XDP_PASS and modifies the headroom, that is, the information in virtio_net_hdr_mrg_rxbuf becomes invalid (including flags). So information like csum_{start, offset} is no longer correct. Therefore, the skb converted from xdp_buff does not save information in skb cb. And, skb->ip_summed = CHECKSUM_NONE, there is a high probability of packet loss at this time because the incorrect check value. So, in order to solve this problem (not only), we save the flags in virtio_net_hdr_mrg_rxbuf before the #2 step above, which is the original information, just to know whether the packet is _NEEDS_CSUM or _DATA_VALID. If the saved flags contains _NEEDS_CSUM, then we use virtnet_set_csum_after_xdp() to recalculate csum_{start, offset}, pseudo-header checksum for the skb converted from xdp_buff. That is, the saved flags are only used to identify whether we want to re-probe for the packet to avoiding the packet dropping. Thanks. > > is there another use of flags that I missed? > > > > > > It seems preferable not to poke at the header an extra time. > > > > > > -- > > > MST
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 07b4801d689c..25b486ab74db 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, struct net_device *dev = vi->dev; struct sk_buff *skb; struct virtio_net_hdr_mrg_rxbuf *hdr; + __u8 flags; if (unlikely(len < vi->hdr_len + ETH_HLEN)) { pr_debug("%s: short packet %i\n", dev->name, len); @@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, return; } + flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags; + if (vi->mergeable_rx_bufs) skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit, stats); @@ -1728,19 +1731,28 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, if (unlikely(!skb)) return; - hdr = skb_vnet_hdr(skb); - if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report) - virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb); - - if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID) - skb->ip_summed = CHECKSUM_UNNECESSARY; + if (unlikely(vi->xdp_enabled)) { + if (virtnet_set_csum_after_xdp(vi, skb, flags) < 0) { + pr_debug("%s: errors occurred in flow dissector setting csum", + dev->name); + goto frame_err; + } - if (virtio_net_hdr_to_skb(skb, &hdr->hdr, - virtio_is_little_endian(vi->vdev))) { - net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n", - dev->name, hdr->hdr.gso_type, - hdr->hdr.gso_size); - goto frame_err; + } else { + hdr = skb_vnet_hdr(skb); + if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report) + virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb); + + if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID) + skb->ip_summed = CHECKSUM_UNNECESSARY; + + if (virtio_net_hdr_to_skb(skb, &hdr->hdr, + virtio_is_little_endian(vi->vdev))) { + net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n", + dev->name, hdr->hdr.gso_type, + hdr->hdr.gso_size); + goto frame_err; + } } skb_record_rx_queue(skb, vq2rxq(rq->vq));