Re: [PATCH net-next 3/4] virtio-net: support coexistence of XDP and _F_GUEST_CSUM

From: Heng Qi <hengqi@linux.alibaba.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: netdev@vger.kernel.org, bpf@vger.kernel.org,
	Jason Wang <jasowang@redhat.com>,
	Xuan Zhuo <xuanzhuo@linux.alibaba.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Jesper Dangaard Brouer <hawk@kernel.org>,
	John Fastabend <john.fastabend@gmail.com>
Subject: Re: [PATCH net-next 3/4] virtio-net: support coexistence of XDP and _F_GUEST_CSUM
Date: Tue, 20 Jun 2023 19:01:48 +0800	[thread overview]
Message-ID: <20230620110148.GF74977@h68b04307.sqa.eu95> (raw)
In-Reply-To: <20230620064711-mutt-send-email-mst@kernel.org>

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