* [PATCH net-next 0/2] igc: driver change to support XDP metadata @ 2021-11-15 20:36 ` Jesper Dangaard Brouer 0 siblings, 0 replies; 35+ messages in thread From: Jesper Dangaard Brouer @ 2021-11-15 20:36 UTC (permalink / raw) To: Jesper Dangaard Brouer, netdev Cc: bpf, Jakub Kicinski, David S. Miller, Daniel Borkmann, anthony.l.nguyen, jesse.brandeburg, intel-wired-lan, magnus.karlsson, bjorn Changes to fix and enable XDP metadata to a specific Intel driver igc. Tested with hardware i225 that uses driver igc, while testing AF_XDP access to metadata area. --- Jesper Dangaard Brouer (2): igc: AF_XDP zero-copy metadata adjust breaks SKBs on XDP_PASS igc: enable XDP metadata in driver drivers/net/ethernet/intel/igc/igc_main.c | 33 +++++++++++++++++++---------- 1 file changed, 22 insertions(+), 11 deletions(-) -- ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 0/2] igc: driver change to support XDP metadata @ 2021-11-15 20:36 ` Jesper Dangaard Brouer 0 siblings, 0 replies; 35+ messages in thread From: Jesper Dangaard Brouer @ 2021-11-15 20:36 UTC (permalink / raw) To: intel-wired-lan Changes to fix and enable XDP metadata to a specific Intel driver igc. Tested with hardware i225 that uses driver igc, while testing AF_XDP access to metadata area. --- Jesper Dangaard Brouer (2): igc: AF_XDP zero-copy metadata adjust breaks SKBs on XDP_PASS igc: enable XDP metadata in driver drivers/net/ethernet/intel/igc/igc_main.c | 33 +++++++++++++++++++---------- 1 file changed, 22 insertions(+), 11 deletions(-) -- ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH net-next 1/2] igc: AF_XDP zero-copy metadata adjust breaks SKBs on XDP_PASS 2021-11-15 20:36 ` [Intel-wired-lan] " Jesper Dangaard Brouer @ 2021-11-15 20:36 ` Jesper Dangaard Brouer -1 siblings, 0 replies; 35+ messages in thread From: Jesper Dangaard Brouer @ 2021-11-15 20:36 UTC (permalink / raw) To: Jesper Dangaard Brouer, netdev Cc: bpf, Jakub Kicinski, David S. Miller, Daniel Borkmann, anthony.l.nguyen, jesse.brandeburg, intel-wired-lan, magnus.karlsson, bjorn Driver already implicitly supports XDP metadata access in AF_XDP zero-copy mode, as xsk_buff_pool's xp_alloc() naturally set xdp_buff data_meta equal data. This works fine for XDP and AF_XDP, but if a BPF-prog adjust via bpf_xdp_adjust_meta() and choose to call XDP_PASS, then igc function igc_construct_skb_zc() will construct an invalid SKB packet. The function correctly include the xdp->data_meta area in the memcpy, but forgot to pull header to take metasize into account. Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy") Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- drivers/net/ethernet/intel/igc/igc_main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 8e448288ee26..76b0a7311369 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -2448,8 +2448,10 @@ static struct sk_buff *igc_construct_skb_zc(struct igc_ring *ring, skb_reserve(skb, xdp->data_meta - xdp->data_hard_start); memcpy(__skb_put(skb, totalsize), xdp->data_meta, totalsize); - if (metasize) + if (metasize) { skb_metadata_set(skb, metasize); + __skb_pull(skb, metasize); + } return skb; } ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 1/2] igc: AF_XDP zero-copy metadata adjust breaks SKBs on XDP_PASS @ 2021-11-15 20:36 ` Jesper Dangaard Brouer 0 siblings, 0 replies; 35+ messages in thread From: Jesper Dangaard Brouer @ 2021-11-15 20:36 UTC (permalink / raw) To: intel-wired-lan Driver already implicitly supports XDP metadata access in AF_XDP zero-copy mode, as xsk_buff_pool's xp_alloc() naturally set xdp_buff data_meta equal data. This works fine for XDP and AF_XDP, but if a BPF-prog adjust via bpf_xdp_adjust_meta() and choose to call XDP_PASS, then igc function igc_construct_skb_zc() will construct an invalid SKB packet. The function correctly include the xdp->data_meta area in the memcpy, but forgot to pull header to take metasize into account. Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy") Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- drivers/net/ethernet/intel/igc/igc_main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 8e448288ee26..76b0a7311369 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -2448,8 +2448,10 @@ static struct sk_buff *igc_construct_skb_zc(struct igc_ring *ring, skb_reserve(skb, xdp->data_meta - xdp->data_hard_start); memcpy(__skb_put(skb, totalsize), xdp->data_meta, totalsize); - if (metasize) + if (metasize) { skb_metadata_set(skb, metasize); + __skb_pull(skb, metasize); + } return skb; } ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 1/2] igc: AF_XDP zero-copy metadata adjust breaks SKBs on XDP_PASS 2021-11-15 20:36 ` [Intel-wired-lan] " Jesper Dangaard Brouer @ 2021-11-21 10:32 ` Kraus, NechamaX -1 siblings, 0 replies; 35+ messages in thread From: Kraus, NechamaX @ 2021-11-21 10:32 UTC (permalink / raw) To: Jesper Dangaard Brouer, netdev Cc: intel-wired-lan, bjorn, Jakub Kicinski, bpf, Daniel Borkmann, David S. Miller, magnus.karlsson On 11/15/2021 22:36, Jesper Dangaard Brouer wrote: > Driver already implicitly supports XDP metadata access in AF_XDP > zero-copy mode, as xsk_buff_pool's xp_alloc() naturally set xdp_buff > data_meta equal data. > > This works fine for XDP and AF_XDP, but if a BPF-prog adjust via > bpf_xdp_adjust_meta() and choose to call XDP_PASS, then igc function > igc_construct_skb_zc() will construct an invalid SKB packet. The > function correctly include the xdp->data_meta area in the memcpy, but > forgot to pull header to take metasize into account. > > Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy") > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Tested-by: Nechama Kraus <nechamax.kraus@linux.intel.com> ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 1/2] igc: AF_XDP zero-copy metadata adjust breaks SKBs on XDP_PASS @ 2021-11-21 10:32 ` Kraus, NechamaX 0 siblings, 0 replies; 35+ messages in thread From: Kraus, NechamaX @ 2021-11-21 10:32 UTC (permalink / raw) To: intel-wired-lan On 11/15/2021 22:36, Jesper Dangaard Brouer wrote: > Driver already implicitly supports XDP metadata access in AF_XDP > zero-copy mode, as xsk_buff_pool's xp_alloc() naturally set xdp_buff > data_meta equal data. > > This works fine for XDP and AF_XDP, but if a BPF-prog adjust via > bpf_xdp_adjust_meta() and choose to call XDP_PASS, then igc function > igc_construct_skb_zc() will construct an invalid SKB packet. The > function correctly include the xdp->data_meta area in the memcpy, but > forgot to pull header to take metasize into account. > > Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy") > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Tested-by: Nechama Kraus <nechamax.kraus@linux.intel.com> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 1/2] igc: AF_XDP zero-copy metadata adjust breaks SKBs on XDP_PASS 2021-11-15 20:36 ` [Intel-wired-lan] " Jesper Dangaard Brouer @ 2021-11-26 15:25 ` Maciej Fijalkowski -1 siblings, 0 replies; 35+ messages in thread From: Maciej Fijalkowski @ 2021-11-26 15:25 UTC (permalink / raw) To: Jesper Dangaard Brouer Cc: netdev, intel-wired-lan, bjorn, Jakub Kicinski, bpf, Daniel Borkmann, David S. Miller, magnus.karlsson On Mon, Nov 15, 2021 at 09:36:25PM +0100, Jesper Dangaard Brouer wrote: > Driver already implicitly supports XDP metadata access in AF_XDP > zero-copy mode, as xsk_buff_pool's xp_alloc() naturally set xdp_buff > data_meta equal data. > > This works fine for XDP and AF_XDP, but if a BPF-prog adjust via > bpf_xdp_adjust_meta() and choose to call XDP_PASS, then igc function > igc_construct_skb_zc() will construct an invalid SKB packet. The > function correctly include the xdp->data_meta area in the memcpy, but > forgot to pull header to take metasize into account. > > Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy") > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Great catch. Will take a look at other ZC enabled Intel drivers if they are affected as well. Thanks! > --- > drivers/net/ethernet/intel/igc/igc_main.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 8e448288ee26..76b0a7311369 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -2448,8 +2448,10 @@ static struct sk_buff *igc_construct_skb_zc(struct igc_ring *ring, > > skb_reserve(skb, xdp->data_meta - xdp->data_hard_start); > memcpy(__skb_put(skb, totalsize), xdp->data_meta, totalsize); > - if (metasize) > + if (metasize) { > skb_metadata_set(skb, metasize); > + __skb_pull(skb, metasize); > + } > > return skb; > } > > > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 1/2] igc: AF_XDP zero-copy metadata adjust breaks SKBs on XDP_PASS @ 2021-11-26 15:25 ` Maciej Fijalkowski 0 siblings, 0 replies; 35+ messages in thread From: Maciej Fijalkowski @ 2021-11-26 15:25 UTC (permalink / raw) To: intel-wired-lan On Mon, Nov 15, 2021 at 09:36:25PM +0100, Jesper Dangaard Brouer wrote: > Driver already implicitly supports XDP metadata access in AF_XDP > zero-copy mode, as xsk_buff_pool's xp_alloc() naturally set xdp_buff > data_meta equal data. > > This works fine for XDP and AF_XDP, but if a BPF-prog adjust via > bpf_xdp_adjust_meta() and choose to call XDP_PASS, then igc function > igc_construct_skb_zc() will construct an invalid SKB packet. The > function correctly include the xdp->data_meta area in the memcpy, but > forgot to pull header to take metasize into account. > > Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy") > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Great catch. Will take a look at other ZC enabled Intel drivers if they are affected as well. Thanks! > --- > drivers/net/ethernet/intel/igc/igc_main.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 8e448288ee26..76b0a7311369 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -2448,8 +2448,10 @@ static struct sk_buff *igc_construct_skb_zc(struct igc_ring *ring, > > skb_reserve(skb, xdp->data_meta - xdp->data_hard_start); > memcpy(__skb_put(skb, totalsize), xdp->data_meta, totalsize); > - if (metasize) > + if (metasize) { > skb_metadata_set(skb, metasize); > + __skb_pull(skb, metasize); > + } > > return skb; > } > > > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan at osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 1/2] igc: AF_XDP zero-copy metadata adjust breaks SKBs on XDP_PASS 2021-11-26 15:25 ` Maciej Fijalkowski @ 2021-11-26 15:32 ` Jesper Dangaard Brouer -1 siblings, 0 replies; 35+ messages in thread From: Jesper Dangaard Brouer @ 2021-11-26 15:32 UTC (permalink / raw) To: Maciej Fijalkowski Cc: brouer, netdev, intel-wired-lan, bjorn, Jakub Kicinski, bpf, Daniel Borkmann, David S. Miller, magnus.karlsson On 26/11/2021 16.25, Maciej Fijalkowski wrote: > On Mon, Nov 15, 2021 at 09:36:25PM +0100, Jesper Dangaard Brouer wrote: >> Driver already implicitly supports XDP metadata access in AF_XDP >> zero-copy mode, as xsk_buff_pool's xp_alloc() naturally set xdp_buff >> data_meta equal data. >> >> This works fine for XDP and AF_XDP, but if a BPF-prog adjust via >> bpf_xdp_adjust_meta() and choose to call XDP_PASS, then igc function >> igc_construct_skb_zc() will construct an invalid SKB packet. The >> function correctly include the xdp->data_meta area in the memcpy, but >> forgot to pull header to take metasize into account. >> >> Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy") >> Signed-off-by: Jesper Dangaard Brouer<brouer@redhat.com> > Acked-by: Maciej Fijalkowski<maciej.fijalkowski@intel.com> > > Great catch. Will take a look at other ZC enabled Intel drivers if they > are affected as well. Thanks a lot for taking this task!!! :-) --Jesper ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 1/2] igc: AF_XDP zero-copy metadata adjust breaks SKBs on XDP_PASS @ 2021-11-26 15:32 ` Jesper Dangaard Brouer 0 siblings, 0 replies; 35+ messages in thread From: Jesper Dangaard Brouer @ 2021-11-26 15:32 UTC (permalink / raw) To: intel-wired-lan On 26/11/2021 16.25, Maciej Fijalkowski wrote: > On Mon, Nov 15, 2021 at 09:36:25PM +0100, Jesper Dangaard Brouer wrote: >> Driver already implicitly supports XDP metadata access in AF_XDP >> zero-copy mode, as xsk_buff_pool's xp_alloc() naturally set xdp_buff >> data_meta equal data. >> >> This works fine for XDP and AF_XDP, but if a BPF-prog adjust via >> bpf_xdp_adjust_meta() and choose to call XDP_PASS, then igc function >> igc_construct_skb_zc() will construct an invalid SKB packet. The >> function correctly include the xdp->data_meta area in the memcpy, but >> forgot to pull header to take metasize into account. >> >> Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy") >> Signed-off-by: Jesper Dangaard Brouer<brouer@redhat.com> > Acked-by: Maciej Fijalkowski<maciej.fijalkowski@intel.com> > > Great catch. Will take a look at other ZC enabled Intel drivers if they > are affected as well. Thanks a lot for taking this task!!! :-) --Jesper ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 1/2] igc: AF_XDP zero-copy metadata adjust breaks SKBs on XDP_PASS 2021-11-26 15:32 ` Jesper Dangaard Brouer @ 2021-11-26 15:54 ` Alexander Lobakin -1 siblings, 0 replies; 35+ messages in thread From: Alexander Lobakin @ 2021-11-26 15:54 UTC (permalink / raw) To: Jesper Dangaard Brouer Cc: Alexander Lobakin, Maciej Fijalkowski, brouer, netdev, intel-wired-lan, bjorn, Jakub Kicinski, bpf, Daniel Borkmann, David S. Miller, magnus.karlsson From: Jesper Dangaard Brouer <jbrouer@redhat.com> Date: Fri, 26 Nov 2021 16:32:47 +0100 > On 26/11/2021 16.25, Maciej Fijalkowski wrote: > > On Mon, Nov 15, 2021 at 09:36:25PM +0100, Jesper Dangaard Brouer wrote: > >> Driver already implicitly supports XDP metadata access in AF_XDP > >> zero-copy mode, as xsk_buff_pool's xp_alloc() naturally set xdp_buff > >> data_meta equal data. > >> > >> This works fine for XDP and AF_XDP, but if a BPF-prog adjust via > >> bpf_xdp_adjust_meta() and choose to call XDP_PASS, then igc function > >> igc_construct_skb_zc() will construct an invalid SKB packet. The > >> function correctly include the xdp->data_meta area in the memcpy, but > >> forgot to pull header to take metasize into account. > >> > >> Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy") > >> Signed-off-by: Jesper Dangaard Brouer<brouer@redhat.com> > > Acked-by: Maciej Fijalkowski<maciej.fijalkowski@intel.com> > > > > Great catch. Will take a look at other ZC enabled Intel drivers if they > > are affected as well. They are. We'll cover them in a separate series, much thanks for revealing that (: > Thanks a lot for taking this task!!! :-) > --Jesper Al ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 1/2] igc: AF_XDP zero-copy metadata adjust breaks SKBs on XDP_PASS @ 2021-11-26 15:54 ` Alexander Lobakin 0 siblings, 0 replies; 35+ messages in thread From: Alexander Lobakin @ 2021-11-26 15:54 UTC (permalink / raw) To: intel-wired-lan From: Jesper Dangaard Brouer <jbrouer@redhat.com> Date: Fri, 26 Nov 2021 16:32:47 +0100 > On 26/11/2021 16.25, Maciej Fijalkowski wrote: > > On Mon, Nov 15, 2021 at 09:36:25PM +0100, Jesper Dangaard Brouer wrote: > >> Driver already implicitly supports XDP metadata access in AF_XDP > >> zero-copy mode, as xsk_buff_pool's xp_alloc() naturally set xdp_buff > >> data_meta equal data. > >> > >> This works fine for XDP and AF_XDP, but if a BPF-prog adjust via > >> bpf_xdp_adjust_meta() and choose to call XDP_PASS, then igc function > >> igc_construct_skb_zc() will construct an invalid SKB packet. The > >> function correctly include the xdp->data_meta area in the memcpy, but > >> forgot to pull header to take metasize into account. > >> > >> Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy") > >> Signed-off-by: Jesper Dangaard Brouer<brouer@redhat.com> > > Acked-by: Maciej Fijalkowski<maciej.fijalkowski@intel.com> > > > > Great catch. Will take a look at other ZC enabled Intel drivers if they > > are affected as well. They are. We'll cover them in a separate series, much thanks for revealing that (: > Thanks a lot for taking this task!!! :-) > --Jesper Al ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH net-next 2/2] igc: enable XDP metadata in driver 2021-11-15 20:36 ` [Intel-wired-lan] " Jesper Dangaard Brouer @ 2021-11-15 20:36 ` Jesper Dangaard Brouer -1 siblings, 0 replies; 35+ messages in thread From: Jesper Dangaard Brouer @ 2021-11-15 20:36 UTC (permalink / raw) To: Jesper Dangaard Brouer, netdev Cc: bpf, Jakub Kicinski, David S. Miller, Daniel Borkmann, anthony.l.nguyen, jesse.brandeburg, intel-wired-lan, magnus.karlsson, bjorn Enabling the XDP bpf_prog access to data_meta area is a very small change. Hint passing 'true' to xdp_prepare_buff(). The SKB layers can also access data_meta area, which required more driver changes to support. Reviewers, notice the igc driver have two different functions that can create SKBs, depending on driver config. Hint for testers, ethtool priv-flags legacy-rx enables the function igc_construct_skb() ethtool --set-priv-flags DEV legacy-rx on Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- drivers/net/ethernet/intel/igc/igc_main.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 76b0a7311369..b516f1b301b4 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -1718,24 +1718,26 @@ static void igc_add_rx_frag(struct igc_ring *rx_ring, static struct sk_buff *igc_build_skb(struct igc_ring *rx_ring, struct igc_rx_buffer *rx_buffer, - union igc_adv_rx_desc *rx_desc, - unsigned int size) + struct xdp_buff *xdp) { - void *va = page_address(rx_buffer->page) + rx_buffer->page_offset; + unsigned int size = xdp->data_end - xdp->data; unsigned int truesize = igc_get_rx_frame_truesize(rx_ring, size); + unsigned int metasize = xdp->data - xdp->data_meta; struct sk_buff *skb; /* prefetch first cache line of first page */ - net_prefetch(va); + net_prefetch(xdp->data); /* 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); /* allocate a skb to store the frags */ - skb = napi_alloc_skb(&rx_ring->q_vector->napi, IGC_RX_HDR_LEN); + skb = napi_alloc_skb(&rx_ring->q_vector->napi, IGC_RX_HDR_LEN + metasize); if (unlikely(!skb)) return NULL; @@ -1769,7 +1772,13 @@ static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring, headlen = eth_get_headlen(skb->dev, va, IGC_RX_HDR_LEN); /* align pull length to size of long to optimize memcpy performance */ - memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long))); + memcpy(__skb_put(skb, headlen + metasize), xdp->data_meta, + ALIGN(headlen + metasize, sizeof(long))); + + if (metasize) { + skb_metadata_set(skb, metasize); + __skb_pull(skb, metasize); + } /* update all of the pointers */ size -= headlen; @@ -2354,7 +2363,7 @@ 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, false); + igc_rx_offset(rx_ring) + pkt_offset, size, true); skb = igc_xdp_run_prog(adapter, &xdp); } @@ -2378,7 +2387,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget) } else if (skb) igc_add_rx_frag(rx_ring, rx_buffer, skb, size); else if (ring_uses_build_skb(rx_ring)) - skb = igc_build_skb(rx_ring, rx_buffer, rx_desc, size); + skb = igc_build_skb(rx_ring, rx_buffer, &xdp); else skb = igc_construct_skb(rx_ring, rx_buffer, &xdp, timestamp); ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 2/2] igc: enable XDP metadata in driver @ 2021-11-15 20:36 ` Jesper Dangaard Brouer 0 siblings, 0 replies; 35+ messages in thread From: Jesper Dangaard Brouer @ 2021-11-15 20:36 UTC (permalink / raw) To: intel-wired-lan Enabling the XDP bpf_prog access to data_meta area is a very small change. Hint passing 'true' to xdp_prepare_buff(). The SKB layers can also access data_meta area, which required more driver changes to support. Reviewers, notice the igc driver have two different functions that can create SKBs, depending on driver config. Hint for testers, ethtool priv-flags legacy-rx enables the function igc_construct_skb() ethtool --set-priv-flags DEV legacy-rx on Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- drivers/net/ethernet/intel/igc/igc_main.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 76b0a7311369..b516f1b301b4 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -1718,24 +1718,26 @@ static void igc_add_rx_frag(struct igc_ring *rx_ring, static struct sk_buff *igc_build_skb(struct igc_ring *rx_ring, struct igc_rx_buffer *rx_buffer, - union igc_adv_rx_desc *rx_desc, - unsigned int size) + struct xdp_buff *xdp) { - void *va = page_address(rx_buffer->page) + rx_buffer->page_offset; + unsigned int size = xdp->data_end - xdp->data; unsigned int truesize = igc_get_rx_frame_truesize(rx_ring, size); + unsigned int metasize = xdp->data - xdp->data_meta; struct sk_buff *skb; /* prefetch first cache line of first page */ - net_prefetch(va); + net_prefetch(xdp->data); /* 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); /* allocate a skb to store the frags */ - skb = napi_alloc_skb(&rx_ring->q_vector->napi, IGC_RX_HDR_LEN); + skb = napi_alloc_skb(&rx_ring->q_vector->napi, IGC_RX_HDR_LEN + metasize); if (unlikely(!skb)) return NULL; @@ -1769,7 +1772,13 @@ static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring, headlen = eth_get_headlen(skb->dev, va, IGC_RX_HDR_LEN); /* align pull length to size of long to optimize memcpy performance */ - memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long))); + memcpy(__skb_put(skb, headlen + metasize), xdp->data_meta, + ALIGN(headlen + metasize, sizeof(long))); + + if (metasize) { + skb_metadata_set(skb, metasize); + __skb_pull(skb, metasize); + } /* update all of the pointers */ size -= headlen; @@ -2354,7 +2363,7 @@ 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, false); + igc_rx_offset(rx_ring) + pkt_offset, size, true); skb = igc_xdp_run_prog(adapter, &xdp); } @@ -2378,7 +2387,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget) } else if (skb) igc_add_rx_frag(rx_ring, rx_buffer, skb, size); else if (ring_uses_build_skb(rx_ring)) - skb = igc_build_skb(rx_ring, rx_buffer, rx_desc, size); + skb = igc_build_skb(rx_ring, rx_buffer, &xdp); else skb = igc_construct_skb(rx_ring, rx_buffer, &xdp, timestamp); ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next 2/2] igc: enable XDP metadata in driver 2021-11-15 20:36 ` [Intel-wired-lan] " Jesper Dangaard Brouer @ 2021-11-21 10:33 ` Kraus, NechamaX -1 siblings, 0 replies; 35+ messages in thread From: Kraus, NechamaX @ 2021-11-21 10:33 UTC (permalink / raw) To: Jesper Dangaard Brouer, netdev Cc: intel-wired-lan, bjorn, Jakub Kicinski, bpf, Daniel Borkmann, David S. Miller, magnus.karlsson On 11/15/2021 22:36, Jesper Dangaard Brouer wrote: > Enabling the XDP bpf_prog access to data_meta area is a very small > change. Hint passing 'true' to xdp_prepare_buff(). > > The SKB layers can also access data_meta area, which required more > driver changes to support. Reviewers, notice the igc driver have two > different functions that can create SKBs, depending on driver config. > > Hint for testers, ethtool priv-flags legacy-rx enables > the function igc_construct_skb() > > ethtool --set-priv-flags DEV legacy-rx on > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) Tested-by: Nechama Kraus <nechamax.kraus@linux.intel.com> ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 2/2] igc: enable XDP metadata in driver @ 2021-11-21 10:33 ` Kraus, NechamaX 0 siblings, 0 replies; 35+ messages in thread From: Kraus, NechamaX @ 2021-11-21 10:33 UTC (permalink / raw) To: intel-wired-lan On 11/15/2021 22:36, Jesper Dangaard Brouer wrote: > Enabling the XDP bpf_prog access to data_meta area is a very small > change. Hint passing 'true' to xdp_prepare_buff(). > > The SKB layers can also access data_meta area, which required more > driver changes to support. Reviewers, notice the igc driver have two > different functions that can create SKBs, depending on driver config. > > Hint for testers, ethtool priv-flags legacy-rx enables > the function igc_construct_skb() > > ethtool --set-priv-flags DEV legacy-rx on > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) Tested-by: Nechama Kraus <nechamax.kraus@linux.intel.com> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next 2/2] igc: enable XDP metadata in driver 2021-11-15 20:36 ` [Intel-wired-lan] " Jesper Dangaard Brouer @ 2021-11-26 16:16 ` Alexander Lobakin -1 siblings, 0 replies; 35+ messages in thread From: Alexander Lobakin @ 2021-11-26 16:16 UTC (permalink / raw) To: Jesper Dangaard Brouer Cc: Alexander Lobakin, bpf, Jakub Kicinski, David S. Miller, Daniel Borkmann, anthony.l.nguyen, jesse.brandeburg, intel-wired-lan, magnus.karlsson, bjorn, netdev From: Jesper Dangaard Brouer <brouer@redhat.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(). > > The SKB layers can also access data_meta area, which required more > driver changes to support. Reviewers, notice the igc driver have two > different functions that can create SKBs, depending on driver config. > > Hint for testers, ethtool priv-flags legacy-rx enables > the function igc_construct_skb() > > ethtool --set-priv-flags DEV legacy-rx on > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 76b0a7311369..b516f1b301b4 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -1718,24 +1718,26 @@ static void igc_add_rx_frag(struct igc_ring *rx_ring, > > static struct sk_buff *igc_build_skb(struct igc_ring *rx_ring, > struct igc_rx_buffer *rx_buffer, > - union igc_adv_rx_desc *rx_desc, > - unsigned int size) > + struct xdp_buff *xdp) > { > - void *va = page_address(rx_buffer->page) + rx_buffer->page_offset; > + unsigned int size = xdp->data_end - xdp->data; > unsigned int truesize = igc_get_rx_frame_truesize(rx_ring, size); > + unsigned int metasize = xdp->data - xdp->data_meta; > struct sk_buff *skb; > > /* prefetch first cache line of first page */ > - net_prefetch(va); > + net_prefetch(xdp->data); I'd prefer prefetching xdp->data_meta here. GRO layer accesses it. Maximum meta size for now is 32, so at least 96 bytes of the frame will stil be prefetched. > > /* 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. > > /* allocate a skb to store the frags */ > - skb = napi_alloc_skb(&rx_ring->q_vector->napi, IGC_RX_HDR_LEN); > + skb = napi_alloc_skb(&rx_ring->q_vector->napi, IGC_RX_HDR_LEN + metasize); > if (unlikely(!skb)) > return NULL; > > @@ -1769,7 +1772,13 @@ static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring, > headlen = eth_get_headlen(skb->dev, va, IGC_RX_HDR_LEN); > > /* align pull length to size of long to optimize memcpy performance */ > - memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long))); > + memcpy(__skb_put(skb, headlen + metasize), xdp->data_meta, > + ALIGN(headlen + metasize, sizeof(long))); > + > + if (metasize) { > + skb_metadata_set(skb, metasize); > + __skb_pull(skb, metasize); > + } > > /* update all of the pointers */ > size -= headlen; > @@ -2354,7 +2363,7 @@ 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, false); > + igc_rx_offset(rx_ring) + pkt_offset, size, true); > > skb = igc_xdp_run_prog(adapter, &xdp); > } > @@ -2378,7 +2387,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget) > } else if (skb) > igc_add_rx_frag(rx_ring, rx_buffer, skb, size); > else if (ring_uses_build_skb(rx_ring)) > - skb = igc_build_skb(rx_ring, rx_buffer, rx_desc, size); > + skb = igc_build_skb(rx_ring, rx_buffer, &xdp); > else > skb = igc_construct_skb(rx_ring, rx_buffer, &xdp, > timestamp); Thanks! Al ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 2/2] igc: enable XDP metadata in driver @ 2021-11-26 16:16 ` Alexander Lobakin 0 siblings, 0 replies; 35+ messages in thread From: Alexander Lobakin @ 2021-11-26 16:16 UTC (permalink / raw) To: intel-wired-lan From: Jesper Dangaard Brouer <brouer@redhat.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(). > > The SKB layers can also access data_meta area, which required more > driver changes to support. Reviewers, notice the igc driver have two > different functions that can create SKBs, depending on driver config. > > Hint for testers, ethtool priv-flags legacy-rx enables > the function igc_construct_skb() > > ethtool --set-priv-flags DEV legacy-rx on > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 76b0a7311369..b516f1b301b4 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -1718,24 +1718,26 @@ static void igc_add_rx_frag(struct igc_ring *rx_ring, > > static struct sk_buff *igc_build_skb(struct igc_ring *rx_ring, > struct igc_rx_buffer *rx_buffer, > - union igc_adv_rx_desc *rx_desc, > - unsigned int size) > + struct xdp_buff *xdp) > { > - void *va = page_address(rx_buffer->page) + rx_buffer->page_offset; > + unsigned int size = xdp->data_end - xdp->data; > unsigned int truesize = igc_get_rx_frame_truesize(rx_ring, size); > + unsigned int metasize = xdp->data - xdp->data_meta; > struct sk_buff *skb; > > /* prefetch first cache line of first page */ > - net_prefetch(va); > + net_prefetch(xdp->data); I'd prefer prefetching xdp->data_meta here. GRO layer accesses it. Maximum meta size for now is 32, so at least 96 bytes of the frame will stil be prefetched. > > /* 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. > > /* allocate a skb to store the frags */ > - skb = napi_alloc_skb(&rx_ring->q_vector->napi, IGC_RX_HDR_LEN); > + skb = napi_alloc_skb(&rx_ring->q_vector->napi, IGC_RX_HDR_LEN + metasize); > if (unlikely(!skb)) > return NULL; > > @@ -1769,7 +1772,13 @@ static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring, > headlen = eth_get_headlen(skb->dev, va, IGC_RX_HDR_LEN); > > /* align pull length to size of long to optimize memcpy performance */ > - memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long))); > + memcpy(__skb_put(skb, headlen + metasize), xdp->data_meta, > + ALIGN(headlen + metasize, sizeof(long))); > + > + if (metasize) { > + skb_metadata_set(skb, metasize); > + __skb_pull(skb, metasize); > + } > > /* update all of the pointers */ > size -= headlen; > @@ -2354,7 +2363,7 @@ 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, false); > + igc_rx_offset(rx_ring) + pkt_offset, size, true); > > skb = igc_xdp_run_prog(adapter, &xdp); > } > @@ -2378,7 +2387,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget) > } else if (skb) > igc_add_rx_frag(rx_ring, rx_buffer, skb, size); > else if (ring_uses_build_skb(rx_ring)) > - skb = igc_build_skb(rx_ring, rx_buffer, rx_desc, size); > + skb = igc_build_skb(rx_ring, rx_buffer, &xdp); > else > skb = igc_construct_skb(rx_ring, rx_buffer, &xdp, > timestamp); Thanks! Al ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next 2/2] igc: enable XDP metadata in driver 2021-11-26 16:16 ` [Intel-wired-lan] " Alexander Lobakin @ 2021-11-29 14:39 ` Jesper Dangaard Brouer -1 siblings, 0 replies; 35+ messages in thread From: Jesper Dangaard Brouer @ 2021-11-29 14:39 UTC (permalink / raw) To: Alexander Lobakin Cc: brouer, bpf, Jakub Kicinski, David S. Miller, Daniel Borkmann, anthony.l.nguyen, jesse.brandeburg, intel-wired-lan, magnus.karlsson, bjorn, netdev On 26/11/2021 17.16, Alexander Lobakin wrote: > From: Jesper Dangaard Brouer <brouer@redhat.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(). >> >> The SKB layers can also access data_meta area, which required more >> driver changes to support. Reviewers, notice the igc driver have two >> different functions that can create SKBs, depending on driver config. >> >> Hint for testers, ethtool priv-flags legacy-rx enables >> the function igc_construct_skb() >> >> ethtool --set-priv-flags DEV legacy-rx on >> >> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> >> --- >> drivers/net/ethernet/intel/igc/igc_main.c | 29 +++++++++++++++++++---------- >> 1 file changed, 19 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >> index 76b0a7311369..b516f1b301b4 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_main.c >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >> @@ -1718,24 +1718,26 @@ static void igc_add_rx_frag(struct igc_ring *rx_ring, >> >> static struct sk_buff *igc_build_skb(struct igc_ring *rx_ring, >> struct igc_rx_buffer *rx_buffer, >> - union igc_adv_rx_desc *rx_desc, >> - unsigned int size) >> + struct xdp_buff *xdp) >> { >> - void *va = page_address(rx_buffer->page) + rx_buffer->page_offset; >> + unsigned int size = xdp->data_end - xdp->data; >> unsigned int truesize = igc_get_rx_frame_truesize(rx_ring, size); >> + unsigned int metasize = xdp->data - xdp->data_meta; >> struct sk_buff *skb; >> >> /* prefetch first cache line of first page */ >> - net_prefetch(va); >> + net_prefetch(xdp->data); > > I'd prefer prefetching xdp->data_meta here. GRO layer accesses it. > Maximum meta size for now is 32, so at least 96 bytes of the frame > will stil be prefetched. Prefetch works for "full" cachelines. Intel CPUs often prefect two cache-lines, when doing this, thus I guess we still get xdp->data. 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. Tony is it worth resending a V2 of this patch? >> >> /* 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. > ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 2/2] igc: enable XDP metadata in driver @ 2021-11-29 14:39 ` Jesper Dangaard Brouer 0 siblings, 0 replies; 35+ messages in thread From: Jesper Dangaard Brouer @ 2021-11-29 14:39 UTC (permalink / raw) To: intel-wired-lan On 26/11/2021 17.16, Alexander Lobakin wrote: > From: Jesper Dangaard Brouer <brouer@redhat.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(). >> >> The SKB layers can also access data_meta area, which required more >> driver changes to support. Reviewers, notice the igc driver have two >> different functions that can create SKBs, depending on driver config. >> >> Hint for testers, ethtool priv-flags legacy-rx enables >> the function igc_construct_skb() >> >> ethtool --set-priv-flags DEV legacy-rx on >> >> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> >> --- >> drivers/net/ethernet/intel/igc/igc_main.c | 29 +++++++++++++++++++---------- >> 1 file changed, 19 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >> index 76b0a7311369..b516f1b301b4 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_main.c >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >> @@ -1718,24 +1718,26 @@ static void igc_add_rx_frag(struct igc_ring *rx_ring, >> >> static struct sk_buff *igc_build_skb(struct igc_ring *rx_ring, >> struct igc_rx_buffer *rx_buffer, >> - union igc_adv_rx_desc *rx_desc, >> - unsigned int size) >> + struct xdp_buff *xdp) >> { >> - void *va = page_address(rx_buffer->page) + rx_buffer->page_offset; >> + unsigned int size = xdp->data_end - xdp->data; >> unsigned int truesize = igc_get_rx_frame_truesize(rx_ring, size); >> + unsigned int metasize = xdp->data - xdp->data_meta; >> struct sk_buff *skb; >> >> /* prefetch first cache line of first page */ >> - net_prefetch(va); >> + net_prefetch(xdp->data); > > I'd prefer prefetching xdp->data_meta here. GRO layer accesses it. > Maximum meta size for now is 32, so at least 96 bytes of the frame > will stil be prefetched. Prefetch works for "full" cachelines. Intel CPUs often prefect two cache-lines, when doing this, thus I guess we still get xdp->data. 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. Tony is it worth resending a V2 of this patch? >> >> /* 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. > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next 2/2] igc: enable XDP metadata in driver 2021-11-29 14:39 ` [Intel-wired-lan] " Jesper Dangaard Brouer @ 2021-11-29 14:53 ` Alexander Lobakin -1 siblings, 0 replies; 35+ messages in thread From: Alexander Lobakin @ 2021-11-29 14:53 UTC (permalink / raw) To: Jesper Dangaard Brouer Cc: Alexander Lobakin, brouer, bpf, Jakub Kicinski, David S. Miller, Daniel Borkmann, anthony.l.nguyen, jesse.brandeburg, intel-wired-lan, magnus.karlsson, bjorn, netdev From: Jesper Dangaard Brouer <jbrouer@redhat.com> Date: Mon, 29 Nov 2021 15:39:04 +0100 > On 26/11/2021 17.16, Alexander Lobakin wrote: > > From: Jesper Dangaard Brouer <brouer@redhat.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(). > >> > >> The SKB layers can also access data_meta area, which required more > >> driver changes to support. Reviewers, notice the igc driver have two > >> different functions that can create SKBs, depending on driver config. > >> > >> Hint for testers, ethtool priv-flags legacy-rx enables > >> the function igc_construct_skb() > >> > >> ethtool --set-priv-flags DEV legacy-rx on > >> > >> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > >> --- > >> drivers/net/ethernet/intel/igc/igc_main.c | 29 +++++++++++++++++++---------- > >> 1 file changed, 19 insertions(+), 10 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > >> index 76b0a7311369..b516f1b301b4 100644 > >> --- a/drivers/net/ethernet/intel/igc/igc_main.c > >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c > >> @@ -1718,24 +1718,26 @@ static void igc_add_rx_frag(struct igc_ring *rx_ring, > >> > >> static struct sk_buff *igc_build_skb(struct igc_ring *rx_ring, > >> struct igc_rx_buffer *rx_buffer, > >> - union igc_adv_rx_desc *rx_desc, > >> - unsigned int size) > >> + struct xdp_buff *xdp) > >> { > >> - void *va = page_address(rx_buffer->page) + rx_buffer->page_offset; > >> + unsigned int size = xdp->data_end - xdp->data; > >> unsigned int truesize = igc_get_rx_frame_truesize(rx_ring, size); > >> + unsigned int metasize = xdp->data - xdp->data_meta; > >> struct sk_buff *skb; > >> > >> /* prefetch first cache line of first page */ > >> - net_prefetch(va); > >> + net_prefetch(xdp->data); > > > > I'd prefer prefetching xdp->data_meta here. GRO layer accesses it. > > Maximum meta size for now is 32, so at least 96 bytes of the frame > > will stil be prefetched. > > 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. Reviewed-by: Alexander Lobakin <alexandr.lobakin@intel.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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 2/2] igc: enable XDP metadata in driver @ 2021-11-29 14:53 ` Alexander Lobakin 0 siblings, 0 replies; 35+ messages in thread From: Alexander Lobakin @ 2021-11-29 14:53 UTC (permalink / raw) To: intel-wired-lan From: Jesper Dangaard Brouer <jbrouer@redhat.com> Date: Mon, 29 Nov 2021 15:39:04 +0100 > On 26/11/2021 17.16, Alexander Lobakin wrote: > > From: Jesper Dangaard Brouer <brouer@redhat.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(). > >> > >> The SKB layers can also access data_meta area, which required more > >> driver changes to support. Reviewers, notice the igc driver have two > >> different functions that can create SKBs, depending on driver config. > >> > >> Hint for testers, ethtool priv-flags legacy-rx enables > >> the function igc_construct_skb() > >> > >> ethtool --set-priv-flags DEV legacy-rx on > >> > >> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > >> --- > >> drivers/net/ethernet/intel/igc/igc_main.c | 29 +++++++++++++++++++---------- > >> 1 file changed, 19 insertions(+), 10 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > >> index 76b0a7311369..b516f1b301b4 100644 > >> --- a/drivers/net/ethernet/intel/igc/igc_main.c > >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c > >> @@ -1718,24 +1718,26 @@ static void igc_add_rx_frag(struct igc_ring *rx_ring, > >> > >> static struct sk_buff *igc_build_skb(struct igc_ring *rx_ring, > >> struct igc_rx_buffer *rx_buffer, > >> - union igc_adv_rx_desc *rx_desc, > >> - unsigned int size) > >> + struct xdp_buff *xdp) > >> { > >> - void *va = page_address(rx_buffer->page) + rx_buffer->page_offset; > >> + unsigned int size = xdp->data_end - xdp->data; > >> unsigned int truesize = igc_get_rx_frame_truesize(rx_ring, size); > >> + unsigned int metasize = xdp->data - xdp->data_meta; > >> struct sk_buff *skb; > >> > >> /* prefetch first cache line of first page */ > >> - net_prefetch(va); > >> + net_prefetch(xdp->data); > > > > I'd prefer prefetching xdp->data_meta here. GRO layer accesses it. > > Maximum meta size for now is 32, so at least 96 bytes of the frame > > will stil be prefetched. > > 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. Reviewed-by: Alexander Lobakin <alexandr.lobakin@intel.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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next 2/2] igc: enable XDP metadata in driver 2021-11-29 14:53 ` [Intel-wired-lan] " Alexander Lobakin @ 2021-11-29 18:13 ` Alexander Lobakin -1 siblings, 0 replies; 35+ messages in thread From: Alexander Lobakin @ 2021-11-29 18:13 UTC (permalink / raw) To: Jesper Dangaard Brouer Cc: Alexander Lobakin, brouer, bpf, Jakub Kicinski, David S. Miller, Daniel Borkmann, anthony.l.nguyen, jesse.brandeburg, intel-wired-lan, magnus.karlsson, bjorn, netdev From: Alexander Lobakin <alexandr.lobakin@intel.com> Date: Mon, 29 Nov 2021 15:53:03 +0100 > From: Jesper Dangaard Brouer <jbrouer@redhat.com> > Date: Mon, 29 Nov 2021 15:39:04 +0100 > > > On 26/11/2021 17.16, Alexander Lobakin wrote: > > > From: Jesper Dangaard Brouer <brouer@redhat.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@intel.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 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 2/2] igc: enable XDP metadata in driver @ 2021-11-29 18:13 ` Alexander Lobakin 0 siblings, 0 replies; 35+ messages in thread From: Alexander Lobakin @ 2021-11-29 18:13 UTC (permalink / raw) To: intel-wired-lan From: Alexander Lobakin <alexandr.lobakin@intel.com> Date: Mon, 29 Nov 2021 15:53:03 +0100 > From: Jesper Dangaard Brouer <jbrouer@redhat.com> > Date: Mon, 29 Nov 2021 15:39:04 +0100 > > > On 26/11/2021 17.16, Alexander Lobakin wrote: > > > From: Jesper Dangaard Brouer <brouer@redhat.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@intel.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 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH net-next 2/2] igc: enable XDP metadata in driver 2021-11-29 18:13 ` [Intel-wired-lan] " Alexander Lobakin @ 2021-11-29 19:03 ` Nguyen, Anthony L -1 siblings, 0 replies; 35+ messages in thread From: Nguyen, Anthony L @ 2021-11-29 19:03 UTC (permalink / raw) To: Lobakin, Alexandr, jbrouer Cc: borkmann, Karlsson, Magnus, davem, bjorn, brouer, Brandeburg, Jesse, kuba, intel-wired-lan, bpf, netdev On Mon, 2021-11-29 at 19:13 +0100, Alexander Lobakin wrote: > From: Alexander Lobakin <alexandr.lobakin@intel.com> > Date: Mon, 29 Nov 2021 15:53:03 +0100 > > > From: Jesper Dangaard Brouer <jbrouer@redhat.com> > > Date: Mon, 29 Nov 2021 15:39:04 +0100 > > > > > On 26/11/2021 17.16, Alexander Lobakin wrote: > > > > From: Jesper Dangaard Brouer <brouer@redhat.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 > > > 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): Thanks Al. If Jesper is ok with this, I'll incorporate it in before sending the pull request to netdev. Otherwise, you can do it as follow on in the other series you previously referenced. Thanks, Tony > 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); > } ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 2/2] igc: enable XDP metadata in driver @ 2021-11-29 19:03 ` Nguyen, Anthony L 0 siblings, 0 replies; 35+ messages in thread From: Nguyen, Anthony L @ 2021-11-29 19:03 UTC (permalink / raw) To: intel-wired-lan On Mon, 2021-11-29 at 19:13 +0100, Alexander Lobakin wrote: > From: Alexander Lobakin <alexandr.lobakin@intel.com> > Date: Mon, 29 Nov 2021 15:53:03 +0100 > > > From: Jesper Dangaard Brouer <jbrouer@redhat.com> > > Date: Mon, 29 Nov 2021 15:39:04 +0100 > > > > > On 26/11/2021 17.16, Alexander Lobakin wrote: > > > > From: Jesper Dangaard Brouer <brouer@redhat.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 > > > 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): Thanks Al. If Jesper is ok with this, I'll incorporate it in before sending the pull request to netdev. Otherwise, you can do it as follow on in the other series you previously referenced. Thanks, Tony > 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); > ????????????????} ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next 2/2] igc: enable XDP metadata in driver 2021-11-29 19:03 ` [Intel-wired-lan] " Nguyen, Anthony L @ 2021-11-30 11:25 ` Jesper Dangaard Brouer -1 siblings, 0 replies; 35+ messages in thread From: Jesper Dangaard Brouer @ 2021-11-30 11:25 UTC (permalink / raw) To: Nguyen, Anthony L, Lobakin, Alexandr, jbrouer Cc: brouer, borkmann, Karlsson, Magnus, davem, bjorn, Brandeburg, Jesse, kuba, intel-wired-lan, bpf, netdev On 29/11/2021 20.03, Nguyen, Anthony L wrote: > On Mon, 2021-11-29 at 19:13 +0100, Alexander Lobakin wrote: >> From: Alexander Lobakin <alexandr.lobakin@intel.com> >> Date: Mon, 29 Nov 2021 15:53:03 +0100 >> >>> From: Jesper Dangaard Brouer <jbrouer@redhat.com> >>> Date: Mon, 29 Nov 2021 15:39:04 +0100 >>> >>>> On 26/11/2021 17.16, Alexander Lobakin wrote: >>>>> From: Jesper Dangaard Brouer <brouer@redhat.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 ] [ snip ] >> >>> >>>> 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): > > Thanks Al. If Jesper is ok with this, I'll incorporate it in before > sending the pull request to netdev. Otherwise, you can do it as follow > on in the other series you previously referenced. I'm fine with you incorporating this change. Thanks! :-) --Jesper ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 2/2] igc: enable XDP metadata in driver @ 2021-11-30 11:25 ` Jesper Dangaard Brouer 0 siblings, 0 replies; 35+ messages in thread From: Jesper Dangaard Brouer @ 2021-11-30 11:25 UTC (permalink / raw) To: intel-wired-lan On 29/11/2021 20.03, Nguyen, Anthony L wrote: > On Mon, 2021-11-29 at 19:13 +0100, Alexander Lobakin wrote: >> From: Alexander Lobakin <alexandr.lobakin@intel.com> >> Date: Mon, 29 Nov 2021 15:53:03 +0100 >> >>> From: Jesper Dangaard Brouer <jbrouer@redhat.com> >>> Date: Mon, 29 Nov 2021 15:39:04 +0100 >>> >>>> On 26/11/2021 17.16, Alexander Lobakin wrote: >>>>> From: Jesper Dangaard Brouer <brouer@redhat.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 ] [ snip ] >> >>> >>>> 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): > > Thanks Al. If Jesper is ok with this, I'll incorporate it in before > sending the pull request to netdev. Otherwise, you can do it as follow > on in the other series you previously referenced. I'm fine with you incorporating this change. Thanks! :-) --Jesper ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next 0/2] igc: driver change to support XDP metadata 2021-11-15 20:36 ` [Intel-wired-lan] " Jesper Dangaard Brouer @ 2021-11-29 14:10 ` Alexander Lobakin -1 siblings, 0 replies; 35+ messages in thread From: Alexander Lobakin @ 2021-11-29 14:10 UTC (permalink / raw) To: Jesper Dangaard Brouer Cc: Alexander Lobakin, netdev, bpf, Jakub Kicinski, David S. Miller, Daniel Borkmann, anthony.l.nguyen, jesse.brandeburg, intel-wired-lan, magnus.karlsson, bjorn, Maciej Fijalkowski From: Jesper Dangaard Brouer <brouer@redhat.com> Date: Mon, 15 Nov 2021 21:36:20 +0100 > Changes to fix and enable XDP metadata to a specific Intel driver igc. > Tested with hardware i225 that uses driver igc, while testing AF_XDP > access to metadata area. Would you mind if I take this your series into my bigger one that takes care of it throughout all the Intel drivers? > --- > > Jesper Dangaard Brouer (2): > igc: AF_XDP zero-copy metadata adjust breaks SKBs on XDP_PASS > igc: enable XDP metadata in driver > > > drivers/net/ethernet/intel/igc/igc_main.c | 33 +++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 11 deletions(-) > > -- Thanks, Al ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 0/2] igc: driver change to support XDP metadata @ 2021-11-29 14:10 ` Alexander Lobakin 0 siblings, 0 replies; 35+ messages in thread From: Alexander Lobakin @ 2021-11-29 14:10 UTC (permalink / raw) To: intel-wired-lan From: Jesper Dangaard Brouer <brouer@redhat.com> Date: Mon, 15 Nov 2021 21:36:20 +0100 > Changes to fix and enable XDP metadata to a specific Intel driver igc. > Tested with hardware i225 that uses driver igc, while testing AF_XDP > access to metadata area. Would you mind if I take this your series into my bigger one that takes care of it throughout all the Intel drivers? > --- > > Jesper Dangaard Brouer (2): > igc: AF_XDP zero-copy metadata adjust breaks SKBs on XDP_PASS > igc: enable XDP metadata in driver > > > drivers/net/ethernet/intel/igc/igc_main.c | 33 +++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 11 deletions(-) > > -- Thanks, Al ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next 0/2] igc: driver change to support XDP metadata 2021-11-29 14:10 ` [Intel-wired-lan] " Alexander Lobakin @ 2021-11-29 14:29 ` Jesper Dangaard Brouer -1 siblings, 0 replies; 35+ messages in thread From: Jesper Dangaard Brouer @ 2021-11-29 14:29 UTC (permalink / raw) To: Alexander Lobakin Cc: brouer, netdev, bpf, Jakub Kicinski, David S. Miller, Daniel Borkmann, jesse.brandeburg, intel-wired-lan, magnus.karlsson, bjorn, Maciej Fijalkowski, Tony Nguyen On 29/11/2021 15.10, Alexander Lobakin wrote: > From: Jesper Dangaard Brouer <brouer@redhat.com> > Date: Mon, 15 Nov 2021 21:36:20 +0100 > >> Changes to fix and enable XDP metadata to a specific Intel driver igc. >> Tested with hardware i225 that uses driver igc, while testing AF_XDP >> access to metadata area. > > Would you mind if I take this your series into my bigger one that > takes care of it throughout all the Intel drivers? I have a customer that depend on this fix. They will have to do the backport anyway (to v5.13), but it would bring confidence on their side if the commits appear in an official git-tree before doing the backport (and optimally with a SHA they can refer to). Tony Nguyen have these landed in your git-tree? --JEsper ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 0/2] igc: driver change to support XDP metadata @ 2021-11-29 14:29 ` Jesper Dangaard Brouer 0 siblings, 0 replies; 35+ messages in thread From: Jesper Dangaard Brouer @ 2021-11-29 14:29 UTC (permalink / raw) To: intel-wired-lan On 29/11/2021 15.10, Alexander Lobakin wrote: > From: Jesper Dangaard Brouer <brouer@redhat.com> > Date: Mon, 15 Nov 2021 21:36:20 +0100 > >> Changes to fix and enable XDP metadata to a specific Intel driver igc. >> Tested with hardware i225 that uses driver igc, while testing AF_XDP >> access to metadata area. > > Would you mind if I take this your series into my bigger one that > takes care of it throughout all the Intel drivers? I have a customer that depend on this fix. They will have to do the backport anyway (to v5.13), but it would bring confidence on their side if the commits appear in an official git-tree before doing the backport (and optimally with a SHA they can refer to). Tony Nguyen have these landed in your git-tree? --JEsper ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next 0/2] igc: driver change to support XDP metadata 2021-11-29 14:29 ` [Intel-wired-lan] " Jesper Dangaard Brouer @ 2021-11-29 14:41 ` Alexander Lobakin -1 siblings, 0 replies; 35+ messages in thread From: Alexander Lobakin @ 2021-11-29 14:41 UTC (permalink / raw) To: Jesper Dangaard Brouer Cc: Alexander Lobakin, brouer, netdev, bpf, Jakub Kicinski, David S. Miller, Daniel Borkmann, jesse.brandeburg, intel-wired-lan, magnus.karlsson, bjorn, Maciej Fijalkowski, Tony Nguyen From: Jesper Dangaard Brouer <jbrouer@redhat.com> Date: Mon, 29 Nov 2021 15:29:07 +0100 > On 29/11/2021 15.10, Alexander Lobakin wrote: > > From: Jesper Dangaard Brouer <brouer@redhat.com> > > Date: Mon, 15 Nov 2021 21:36:20 +0100 > > > >> Changes to fix and enable XDP metadata to a specific Intel driver igc. > >> Tested with hardware i225 that uses driver igc, while testing AF_XDP > >> access to metadata area. > > > > Would you mind if I take this your series into my bigger one that > > takes care of it throughout all the Intel drivers? > > I have a customer that depend on this fix. They will have to do the > backport anyway (to v5.13), but it would bring confidence on their side > if the commits appear in an official git-tree before doing the backport > (and optimally with a SHA they can refer to). Yeah, sure, it's totally fine to get them accepted separately, I'll just refer to them in my series. > Tony Nguyen have these landed in your git-tree? Doesn't seem like. The reason might be that you responded to my patch 2/2 comments only now. > --JEsper Al ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-wired-lan] [PATCH net-next 0/2] igc: driver change to support XDP metadata @ 2021-11-29 14:41 ` Alexander Lobakin 0 siblings, 0 replies; 35+ messages in thread From: Alexander Lobakin @ 2021-11-29 14:41 UTC (permalink / raw) To: intel-wired-lan From: Jesper Dangaard Brouer <jbrouer@redhat.com> Date: Mon, 29 Nov 2021 15:29:07 +0100 > On 29/11/2021 15.10, Alexander Lobakin wrote: > > From: Jesper Dangaard Brouer <brouer@redhat.com> > > Date: Mon, 15 Nov 2021 21:36:20 +0100 > > > >> Changes to fix and enable XDP metadata to a specific Intel driver igc. > >> Tested with hardware i225 that uses driver igc, while testing AF_XDP > >> access to metadata area. > > > > Would you mind if I take this your series into my bigger one that > > takes care of it throughout all the Intel drivers? > > I have a customer that depend on this fix. They will have to do the > backport anyway (to v5.13), but it would bring confidence on their side > if the commits appear in an official git-tree before doing the backport > (and optimally with a SHA they can refer to). Yeah, sure, it's totally fine to get them accepted separately, I'll just refer to them in my series. > Tony Nguyen have these landed in your git-tree? Doesn't seem like. The reason might be that you responded to my patch 2/2 comments only now. > --JEsper Al ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH net-next 0/2][pull request] 1GbE Intel Wired LAN Driver Updates 2021-11-30 @ 2021-11-30 17:59 Tony Nguyen 2021-11-30 17:59 ` [PATCH net-next 2/2] igc: enable XDP metadata in driver Tony Nguyen 0 siblings, 1 reply; 35+ messages in thread From: Tony Nguyen @ 2021-11-30 17:59 UTC (permalink / raw) To: davem, kuba Cc: Tony Nguyen, netdev, sasha.neftin, vitaly.lifshits, maciej.fijalkowski, magnus.karlsson, ast, daniel, hawk, john.fastabend, bpf, andrii, kpsingh, kafai, yhs, songliubraving Jesper Dangaard Brouer says: Changes to fix and enable XDP metadata to a specific Intel driver igc. Tested with hardware i225 that uses driver igc, while testing AF_XDP access to metadata area. The following are changes since commit 196073f9c44be0b4758ead11e51bc2875f98df29: net: ixp4xx_hss: drop kfree for memory allocated with devm_kzalloc and are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue 1GbE Jesper Dangaard Brouer (2): igc: AF_XDP zero-copy metadata adjust breaks SKBs on XDP_PASS igc: enable XDP metadata in driver drivers/net/ethernet/intel/igc/igc_main.c | 37 +++++++++++++++-------- 1 file changed, 25 insertions(+), 12 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH net-next 2/2] igc: enable XDP metadata in driver 2021-11-30 17:59 [PATCH net-next 0/2][pull request] 1GbE Intel Wired LAN Driver Updates 2021-11-30 Tony Nguyen @ 2021-11-30 17:59 ` Tony Nguyen 0 siblings, 0 replies; 35+ messages in thread From: Tony Nguyen @ 2021-11-30 17:59 UTC (permalink / raw) To: davem, kuba Cc: Jesper Dangaard Brouer, netdev, anthony.l.nguyen, sasha.neftin, vitaly.lifshits, maciej.fijalkowski, magnus.karlsson, ast, daniel, hawk, john.fastabend, bpf, andrii, kpsingh, kafai, yhs, songliubraving, Nechama Kraus, Alexander Lobakin From: Jesper Dangaard Brouer <brouer@redhat.com> Enabling the XDP bpf_prog access to data_meta area is a very small change. Hint passing 'true' to xdp_prepare_buff(). The SKB layers can also access data_meta area, which required more driver changes to support. Reviewers, notice the igc driver have two different functions that can create SKBs, depending on driver config. Hint for testers, ethtool priv-flags legacy-rx enables the function igc_construct_skb() ethtool --set-priv-flags DEV legacy-rx on Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> Tested-by: Nechama Kraus <nechamax.kraus@linux.intel.com> Reviewed-by: Alexander Lobakin <alexandr.lobakin@intel.com> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> --- drivers/net/ethernet/intel/igc/igc_main.c | 33 +++++++++++++++-------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 76b0a7311369..142c57b7a451 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -1718,24 +1718,26 @@ static void igc_add_rx_frag(struct igc_ring *rx_ring, static struct sk_buff *igc_build_skb(struct igc_ring *rx_ring, struct igc_rx_buffer *rx_buffer, - union igc_adv_rx_desc *rx_desc, - unsigned int size) + struct xdp_buff *xdp) { - void *va = page_address(rx_buffer->page) + rx_buffer->page_offset; + unsigned int size = xdp->data_end - xdp->data; unsigned int truesize = igc_get_rx_frame_truesize(rx_ring, size); + unsigned int metasize = xdp->data - xdp->data_meta; struct sk_buff *skb; /* prefetch first cache line of first page */ - net_prefetch(va); + net_prefetch(xdp->data_meta); /* 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; @@ -1753,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); + skb = napi_alloc_skb(&rx_ring->q_vector->napi, + IGC_RX_HDR_LEN + metasize); if (unlikely(!skb)) return NULL; @@ -1769,7 +1773,13 @@ static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring, headlen = eth_get_headlen(skb->dev, va, IGC_RX_HDR_LEN); /* align pull length to size of long to optimize memcpy performance */ - memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long))); + memcpy(__skb_put(skb, headlen + metasize), xdp->data_meta, + ALIGN(headlen + metasize, sizeof(long))); + + if (metasize) { + skb_metadata_set(skb, metasize); + __skb_pull(skb, metasize); + } /* update all of the pointers */ size -= headlen; @@ -2354,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, false); + igc_rx_offset(rx_ring) + pkt_offset, + size, true); skb = igc_xdp_run_prog(adapter, &xdp); } @@ -2378,7 +2389,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget) } else if (skb) igc_add_rx_frag(rx_ring, rx_buffer, skb, size); else if (ring_uses_build_skb(rx_ring)) - skb = igc_build_skb(rx_ring, rx_buffer, rx_desc, size); + skb = igc_build_skb(rx_ring, rx_buffer, &xdp); else skb = igc_construct_skb(rx_ring, rx_buffer, &xdp, timestamp); -- 2.31.1 ^ permalink raw reply related [flat|nested] 35+ messages in thread
end of thread, other threads:[~2021-11-30 18:17 UTC | newest] Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-15 20:36 [PATCH net-next 0/2] igc: driver change to support XDP metadata Jesper Dangaard Brouer 2021-11-15 20:36 ` [Intel-wired-lan] " Jesper Dangaard Brouer 2021-11-15 20:36 ` [PATCH net-next 1/2] igc: AF_XDP zero-copy metadata adjust breaks SKBs on XDP_PASS Jesper Dangaard Brouer 2021-11-15 20:36 ` [Intel-wired-lan] " Jesper Dangaard Brouer 2021-11-21 10:32 ` Kraus, NechamaX 2021-11-21 10:32 ` Kraus, NechamaX 2021-11-26 15:25 ` Maciej Fijalkowski 2021-11-26 15:25 ` Maciej Fijalkowski 2021-11-26 15:32 ` Jesper Dangaard Brouer 2021-11-26 15:32 ` Jesper Dangaard Brouer 2021-11-26 15:54 ` Alexander Lobakin 2021-11-26 15:54 ` Alexander Lobakin 2021-11-15 20:36 ` [PATCH net-next 2/2] igc: enable XDP metadata in driver Jesper Dangaard Brouer 2021-11-15 20:36 ` [Intel-wired-lan] " Jesper Dangaard Brouer 2021-11-21 10:33 ` Kraus, NechamaX 2021-11-21 10:33 ` Kraus, NechamaX 2021-11-26 16:16 ` Alexander Lobakin 2021-11-26 16:16 ` [Intel-wired-lan] " Alexander Lobakin 2021-11-29 14:39 ` Jesper Dangaard Brouer 2021-11-29 14:39 ` [Intel-wired-lan] " Jesper Dangaard Brouer 2021-11-29 14:53 ` Alexander Lobakin 2021-11-29 14:53 ` [Intel-wired-lan] " Alexander Lobakin 2021-11-29 18:13 ` Alexander Lobakin 2021-11-29 18:13 ` [Intel-wired-lan] " Alexander Lobakin 2021-11-29 19:03 ` Nguyen, Anthony L 2021-11-29 19:03 ` [Intel-wired-lan] " Nguyen, Anthony L 2021-11-30 11:25 ` Jesper Dangaard Brouer 2021-11-30 11:25 ` [Intel-wired-lan] " Jesper Dangaard Brouer 2021-11-29 14:10 ` [PATCH net-next 0/2] igc: driver change to support XDP metadata Alexander Lobakin 2021-11-29 14:10 ` [Intel-wired-lan] " Alexander Lobakin 2021-11-29 14:29 ` Jesper Dangaard Brouer 2021-11-29 14:29 ` [Intel-wired-lan] " Jesper Dangaard Brouer 2021-11-29 14:41 ` Alexander Lobakin 2021-11-29 14:41 ` [Intel-wired-lan] " Alexander Lobakin 2021-11-30 17:59 [PATCH net-next 0/2][pull request] 1GbE Intel Wired LAN Driver Updates 2021-11-30 Tony Nguyen 2021-11-30 17:59 ` [PATCH net-next 2/2] igc: enable XDP metadata in driver Tony Nguyen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.