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

[Horatiu Vultur <horatiu.vultur@microchip.com>]

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

-- 
/Horatiu