netdev - Re: [PATCH net-next 2/2] igc: enable XDP metadata in driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 29 Nov 2021 19:13:20 +0100
From:   Alexander Lobakin <alexandr.lobakin@...el.com>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>
Cc:     Alexander Lobakin <alexandr.lobakin@...el.com>, brouer@...hat.com,
        bpf@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <borkmann@...earbox.net>,
        anthony.l.nguyen@...el.com, jesse.brandeburg@...el.com,
        intel-wired-lan@...ts.osuosl.org, magnus.karlsson@...el.com,
        bjorn@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] igc: enable XDP metadata in driver

From: Alexander Lobakin <alexandr.lobakin@...el.com>
Date: Mon, 29 Nov 2021 15:53:03 +0100

> From: Jesper Dangaard Brouer <jbrouer@...hat.com>
> Date: Mon, 29 Nov 2021 15:39:04 +0100
> 
> > On 26/11/2021 17.16, Alexander Lobakin wrote:
> > > From: Jesper Dangaard Brouer <brouer@...hat.com>
> > > Date: Mon, 15 Nov 2021 21:36:30 +0100
> > > 
> > >> Enabling the XDP bpf_prog access to data_meta area is a very small
> > >> change. Hint passing 'true' to xdp_prepare_buff().

[ snip ]

> > Prefetch works for "full" cachelines. Intel CPUs often prefect two 
> > cache-lines, when doing this, thus I guess we still get xdp->data.
> 
> Sure. I mean, net_prefetch() prefetches 128 bytes in a row.
> xdp->data is usually aligned to XDP_PACKET_HEADROOM (or two bytes
> to the right). If our CL is 64 and the meta is present, then... ah
> right, 64 to the left and 64 starting from data to the right.
> 
> > I don't mind prefetching xdp->data_meta, but (1) I tried to keep the 
> > change minimal as current behavior was data area I kept that. (2) 
> > xdp->data starts on a cacheline and we know NIC hardware have touched 
> > that, it is not a full-cache-miss due to DDIO/DCA it is known to be in 
> > L3 cache (gain is around 2-3 ns in my machine for data prefetch).
> > Given this is only a 2.5 Gbit/s driver/HW I doubt this make any difference.
> 
> Code constistency at least. On 10+ Gbps we prefetch meta, and I plan
> to continue doing this in my series.
> 
> > Tony is it worth resending a V2 of this patch?
> 
> Tony, you can take it as it is if you want, I'll correct it later in
> mine. Up to you.

My "fixup" looks like (in case of v2 needed or so):

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index b516f1b301b4..142c57b7a451 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1726,7 +1726,7 @@ static struct sk_buff *igc_build_skb(struct igc_ring *rx_ring,
 	struct sk_buff *skb;
 
 	/* prefetch first cache line of first page */
-	net_prefetch(xdp->data);
+	net_prefetch(xdp->data_meta);
 
 	/* build an skb around the page buffer */
 	skb = build_skb(xdp->data_hard_start, truesize);
@@ -1756,10 +1756,11 @@ static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring,
 	struct sk_buff *skb;
 
 	/* prefetch first cache line of first page */
-	net_prefetch(va);
+	net_prefetch(xdp->data_meta);
 
 	/* allocate a skb to store the frags */
-	skb = napi_alloc_skb(&rx_ring->q_vector->napi, IGC_RX_HDR_LEN + metasize);
+	skb = napi_alloc_skb(&rx_ring->q_vector->napi,
+			     IGC_RX_HDR_LEN + metasize);
 	if (unlikely(!skb))
 		return NULL;
 
@@ -2363,7 +2364,8 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
 		if (!skb) {
 			xdp_init_buff(&xdp, truesize, &rx_ring->xdp_rxq);
 			xdp_prepare_buff(&xdp, pktbuf - igc_rx_offset(rx_ring),
-					 igc_rx_offset(rx_ring) + pkt_offset, size, true);
+					 igc_rx_offset(rx_ring) + pkt_offset,
+					 size, true);
 
 			skb = igc_xdp_run_prog(adapter, &xdp);
 		}

> Reviewed-by: Alexander Lobakin <alexandr.lobakin@...el.com>
> 
> > >>   
> > >>   	/* build an skb around the page buffer */
> > >> -	skb = build_skb(va - IGC_SKB_PAD, truesize);
> > >> +	skb = build_skb(xdp->data_hard_start, truesize);
> > >>   	if (unlikely(!skb))
> > >>   		return NULL;
> > >>   
> > >>   	/* update pointers within the skb to store the data */
> > >> -	skb_reserve(skb, IGC_SKB_PAD);
> > >> +	skb_reserve(skb, xdp->data - xdp->data_hard_start);
> > >>   	__skb_put(skb, size);
> > >> +	if (metasize)
> > >> +		skb_metadata_set(skb, metasize);
> > >>   
> > >>   	igc_rx_buffer_flip(rx_buffer, truesize);
> > >>   	return skb;
> > >> @@ -1746,6 +1748,7 @@ static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring,
> > >>   					 struct xdp_buff *xdp,
> > >>   					 ktime_t timestamp)
> > >>   {
> > >> +	unsigned int metasize = xdp->data - xdp->data_meta;
> > >>   	unsigned int size = xdp->data_end - xdp->data;
> > >>   	unsigned int truesize = igc_get_rx_frame_truesize(rx_ring, size);
> > >>   	void *va = xdp->data;
> > >> @@ -1756,7 +1759,7 @@ static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring,
> > >>   	net_prefetch(va);
> > > 
> > > ...here as well.
> > >
> 
> Thanks,
> Al

Al