[net-next,1/5] net: lan966x: Add XDP_PACKET_HEADROOM
Commit Message
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
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, ×tamp);
>
[...]
> --- 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
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, ×tamp);
> >
>
> [...]
>
> > --- 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
@@ -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, ×tamp);
@@ -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)
@@ -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: