diff mbox series

[net-next,1/5] net: lan966x: Add XDP_PACKET_HEADROOM

Message ID 20221113111559.1028030-2-horatiu.vultur@microchip.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: lan966x: Extend xdp support | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 77 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Horatiu Vultur Nov. 13, 2022, 11:15 a.m. UTC 
Update the page_pool params to allocate XDP_PACKET_HEADROOM space as
headroom for all received frames.
This is needed for when the XDP_TX and XDP_REDIRECT are implemented.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../ethernet/microchip/lan966x/lan966x_fdma.c | 22 +++++++++++++------
 .../ethernet/microchip/lan966x/lan966x_xdp.c  |  5 +++--
 2 files changed, 18 insertions(+), 9 deletions(-)

Comments

Alexander Lobakin Nov. 14, 2022, 2:16 p.m. UTC | #1 
From: Horatiu Vultur <horatiu.vultur@microchip.com>
Date: Sun, 13 Nov 2022 12:15:55 +0100

> Update the page_pool params to allocate XDP_PACKET_HEADROOM space as
> headroom for all received frames.
> This is needed for when the XDP_TX and XDP_REDIRECT are implemented.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---

[...]

> @@ -466,7 +472,8 @@ static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
>  
>  	skb_mark_for_recycle(skb);
>  
> -	skb_put(skb, FDMA_DCB_STATUS_BLOCKL(db->status));
> +	skb_put(skb, FDMA_DCB_STATUS_BLOCKL(db->status) + XDP_PACKET_HEADROOM);
> +	skb_pull(skb, XDP_PACKET_HEADROOM);

These two must be:

+	skb_reserve(skb, XDP_PACKET_HEADROOM);
 	skb_put(skb, FDMA_DCB_STATUS_BLOCKL(db->status));

i.e. you only need to add reserve, and that's it. It's not only
faster, but also moves ::tail, which is essential.

>  
>  	lan966x_ifh_get_timestamp(skb->data, &timestamp);
>  

[...]

> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> @@ -44,8 +44,9 @@ int lan966x_xdp_run(struct lan966x_port *port, struct page *page, u32 data_len)
>  
>  	xdp_init_buff(&xdp, PAGE_SIZE << lan966x->rx.page_order,
>  		      &port->xdp_rxq);
> -	xdp_prepare_buff(&xdp, page_address(page), IFH_LEN_BYTES,
> -			 data_len - IFH_LEN_BYTES, false);
> +	xdp_prepare_buff(&xdp, page_address(page),
> +			 IFH_LEN_BYTES + XDP_PACKET_HEADROOM,
> +			 data_len - IFH_LEN_BYTES - XDP_PACKET_HEADROOM, false);

Are you sure you need to substract %XDP_PACKET_HEADROOM from
@data_len? I think @data_len is the frame length, so headroom is not
included?

>  	act = bpf_prog_run_xdp(xdp_prog, &xdp);
>  	switch (act) {
>  	case XDP_PASS:
> -- 
> 2.38.0

Thanks,
Olek
Horatiu Vultur Nov. 14, 2022, 7:39 p.m. UTC | #2 
The 11/14/2022 15:16, Alexander Lobakin wrote:
> [Some people who received this message don't often get email from alexandr.lobakin@intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> From: Horatiu Vultur <horatiu.vultur@microchip.com>
> Date: Sun, 13 Nov 2022 12:15:55 +0100

Hi Olek,

> 
> > Update the page_pool params to allocate XDP_PACKET_HEADROOM space as
> > headroom for all received frames.
> > This is needed for when the XDP_TX and XDP_REDIRECT are implemented.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> 
> [...]
> 
> > @@ -466,7 +472,8 @@ static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
> >
> >       skb_mark_for_recycle(skb);
> >
> > -     skb_put(skb, FDMA_DCB_STATUS_BLOCKL(db->status));
> > +     skb_put(skb, FDMA_DCB_STATUS_BLOCKL(db->status) + XDP_PACKET_HEADROOM);
> > +     skb_pull(skb, XDP_PACKET_HEADROOM);
> 
> These two must be:
> 
> +       skb_reserve(skb, XDP_PACKET_HEADROOM);
>         skb_put(skb, FDMA_DCB_STATUS_BLOCKL(db->status));
> 
> i.e. you only need to add reserve, and that's it. It's not only
> faster, but also moves ::tail, which is essential.

Thanks for the suggestion. I will do that in the next version.

> 
> >
> >       lan966x_ifh_get_timestamp(skb->data, &timestamp);
> >
> 
> [...]
> 
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> > @@ -44,8 +44,9 @@ int lan966x_xdp_run(struct lan966x_port *port, struct page *page, u32 data_len)
> >
> >       xdp_init_buff(&xdp, PAGE_SIZE << lan966x->rx.page_order,
> >                     &port->xdp_rxq);
> > -     xdp_prepare_buff(&xdp, page_address(page), IFH_LEN_BYTES,
> > -                      data_len - IFH_LEN_BYTES, false);
> > +     xdp_prepare_buff(&xdp, page_address(page),
> > +                      IFH_LEN_BYTES + XDP_PACKET_HEADROOM,
> > +                      data_len - IFH_LEN_BYTES - XDP_PACKET_HEADROOM, false);
> 
> Are you sure you need to substract %XDP_PACKET_HEADROOM from
> @data_len? I think @data_len is the frame length, so headroom is not
> included?

Actually @data_len contains also the XDP_PACKET_HEADROOM. I am calling
lan966x_xdp_run like this:

---
return lan966x_xdp_run(port, page,
		       FDMA_DCB_STATUS_BLOCKL(db->status) + XDP_PACKET_HEADROOM);
---

But from what I can see, that doesn't make too much sense.
Because I am adding here the XDP_PACKET_HEADROOM and then remove it in
xdp_prepare_buff. So it would be better not to add it at all.

> 
> >       act = bpf_prog_run_xdp(xdp_prog, &xdp);
> >       switch (act) {
> >       case XDP_PASS:
> > --
> > 2.38.0
> 
> Thanks,
> Olek
diff mbox series

Patch

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
index 5fbbd479cfb06..dc1ac340e1fb3 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
@@ -1,5 +1,7 @@ 
 // SPDX-License-Identifier: GPL-2.0+
 
+#include <linux/bpf.h>
+
 #include "lan966x_main.h"
 
 static int lan966x_fdma_channel_active(struct lan966x *lan966x)
@@ -16,7 +18,7 @@  static struct page *lan966x_fdma_rx_alloc_page(struct lan966x_rx *rx,
 	if (unlikely(!page))
 		return NULL;
 
-	db->dataptr = page_pool_get_dma_addr(page);
+	db->dataptr = page_pool_get_dma_addr(page) + XDP_PACKET_HEADROOM;
 
 	return page;
 }
@@ -72,7 +74,7 @@  static int lan966x_fdma_rx_alloc_page_pool(struct lan966x_rx *rx)
 		.nid = NUMA_NO_NODE,
 		.dev = lan966x->dev,
 		.dma_dir = DMA_FROM_DEVICE,
-		.offset = 0,
+		.offset = XDP_PACKET_HEADROOM,
 		.max_len = rx->max_mtu -
 			   SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
 	};
@@ -432,11 +434,13 @@  static int lan966x_fdma_rx_check_frame(struct lan966x_rx *rx, u64 *src_port)
 	if (unlikely(!page))
 		return FDMA_ERROR;
 
-	dma_sync_single_for_cpu(lan966x->dev, (dma_addr_t)db->dataptr,
+	dma_sync_single_for_cpu(lan966x->dev,
+				(dma_addr_t)db->dataptr + XDP_PACKET_HEADROOM,
 				FDMA_DCB_STATUS_BLOCKL(db->status),
 				DMA_FROM_DEVICE);
 
-	lan966x_ifh_get_src_port(page_address(page), src_port);
+	lan966x_ifh_get_src_port(page_address(page) + XDP_PACKET_HEADROOM,
+				 src_port);
 	if (WARN_ON(*src_port >= lan966x->num_phys_ports))
 		return FDMA_ERROR;
 
@@ -444,7 +448,9 @@  static int lan966x_fdma_rx_check_frame(struct lan966x_rx *rx, u64 *src_port)
 	if (!lan966x_xdp_port_present(port))
 		return FDMA_PASS;
 
-	return lan966x_xdp_run(port, page, FDMA_DCB_STATUS_BLOCKL(db->status));
+	return lan966x_xdp_run(port, page,
+			       FDMA_DCB_STATUS_BLOCKL(db->status) +
+			       XDP_PACKET_HEADROOM);
 }
 
 static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
@@ -466,7 +472,8 @@  static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx,
 
 	skb_mark_for_recycle(skb);
 
-	skb_put(skb, FDMA_DCB_STATUS_BLOCKL(db->status));
+	skb_put(skb, FDMA_DCB_STATUS_BLOCKL(db->status) + XDP_PACKET_HEADROOM);
+	skb_pull(skb, XDP_PACKET_HEADROOM);
 
 	lan966x_ifh_get_timestamp(skb->data, &timestamp);
 
@@ -786,7 +793,8 @@  static int lan966x_fdma_get_max_frame(struct lan966x *lan966x)
 	return lan966x_fdma_get_max_mtu(lan966x) +
 	       IFH_LEN_BYTES +
 	       SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) +
-	       VLAN_HLEN * 2;
+	       VLAN_HLEN * 2 +
+	       XDP_PACKET_HEADROOM;
 }
 
 int lan966x_fdma_change_mtu(struct lan966x *lan966x)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
index e77d9f2aad2b4..bab447e79273f 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
@@ -44,8 +44,9 @@  int lan966x_xdp_run(struct lan966x_port *port, struct page *page, u32 data_len)
 
 	xdp_init_buff(&xdp, PAGE_SIZE << lan966x->rx.page_order,
 		      &port->xdp_rxq);
-	xdp_prepare_buff(&xdp, page_address(page), IFH_LEN_BYTES,
-			 data_len - IFH_LEN_BYTES, false);
+	xdp_prepare_buff(&xdp, page_address(page),
+			 IFH_LEN_BYTES + XDP_PACKET_HEADROOM,
+			 data_len - IFH_LEN_BYTES - XDP_PACKET_HEADROOM, false);
 	act = bpf_prog_run_xdp(xdp_prog, &xdp);
 	switch (act) {
 	case XDP_PASS: