The 11/22/2022 23:27, Maciej Fijalkowski wrote:
>
> On Tue, Nov 22, 2022 at 10:44:12PM +0100, Horatiu Vultur wrote:
> > Extend lan966x XDP support with the action XDP_TX. In this case when the
> > received buffer needs to execute XDP_TX, the buffer will be moved to the
> > TX buffers. So a new RX buffer will be allocated.
> > When the TX finish with the frame, it would give back the buffer to the
> > page pool.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
> > ---
...
> >
> > struct lan966x_port;
> > @@ -176,6 +178,7 @@ struct lan966x_tx_dcb_buf {
> > dma_addr_t dma_addr;
> > struct net_device *dev;
> > struct sk_buff *skb;
> > + struct xdp_frame *xdpf;
>
> Couldn't you make an union out of skb and xdpf? I'd say these two are
> mutually exclusive, no? I believe this would simplify some things.
Yes, skb and xdpf are mutually exclusive.
Also Alexander Lobakin mention something similar and I was not sure.
Now that I have tried it I can see it that is more clear that skb and
xdpf are mutually exclusive and also reduce the size of the struct.
So I will update this in the next series.
>
> > u32 len;
> > u32 used : 1;
> > u32 ptp : 1;
> > @@ -360,6 +363,8 @@ bool lan966x_hw_offload(struct lan966x *lan966x, u32 port, struct sk_buff *skb);
> >
> > void lan966x_ifh_get_src_port(void *ifh, u64 *src_port);
> > void lan966x_ifh_get_timestamp(void *ifh, u64 *timestamp);
> > +void lan966x_ifh_set_bypass(void *ifh, u64 bypass);
> > +void lan966x_ifh_set_port(void *ifh, u64 bypass);
> >
> > void lan966x_stats_get(struct net_device *dev,
> > struct rtnl_link_stats64 *stats);
> > @@ -460,6 +465,9 @@ u32 lan966x_ptp_get_period_ps(void);
> > int lan966x_ptp_gettime64(struct ptp_clock_info *ptp, struct timespec64 *ts);
> >
> > int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev);
> > +int lan966x_fdma_xmit_xdpf(struct lan966x_port *port,
> > + struct xdp_frame *frame,
> > + struct page *page);
> > int lan966x_fdma_change_mtu(struct lan966x *lan966x);
> > void lan966x_fdma_netdev_init(struct lan966x *lan966x, struct net_device *dev);
> > void lan966x_fdma_netdev_deinit(struct lan966x *lan966x, struct net_device *dev);
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> > index a99657154cca4..e7998fef7048c 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> > @@ -54,6 +54,7 @@ int lan966x_xdp_run(struct lan966x_port *port, struct page *page, u32 data_len)
> > {
> > struct bpf_prog *xdp_prog = port->xdp_prog;
> > struct lan966x *lan966x = port->lan966x;
> > + struct xdp_frame *xdpf;
> > struct xdp_buff xdp;
> > u32 act;
> >
> > @@ -66,6 +67,13 @@ int lan966x_xdp_run(struct lan966x_port *port, struct page *page, u32 data_len)
> > switch (act) {
> > case XDP_PASS:
> > return FDMA_PASS;
> > + case XDP_TX:
> > + xdpf = xdp_convert_buff_to_frame(&xdp);
> > + if (!xdpf)
> > + return FDMA_DROP;
>
> I would generally challenge the need for xdp_frame in XDP_TX path. You
> probably would be good to go with calling directly
> page_pool_put_full_page() on cleaning side. This frame is not going to be
> redirected so I don't see the need for carrying additional info. I'm
> bringing this up as I was observing performance improvement on ice driver
> when I decided to operate directly on xdp_buff for XDP_TX.
Thanks for suggestion. I definetly see your point.
I would prefer for now to keep this like it is. Because I think in the
near future I should do a proper investigation to see where the
performance of the FDMA can be improved. And this will
definetly be on the TODO.
>
> But it's of course up to you.
>
> > +
> > + return lan966x_fdma_xmit_xdpf(port, xdpf, page) ?
> > + FDMA_DROP : FDMA_TX;
> > default:
> > bpf_warn_invalid_xdp_action(port->dev, xdp_prog, act);
> > fallthrough;
> > --
> > 2.38.0
> >
--
/Horatiu
