Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 23 Nov 2022 21:19:55 +0100
From:   Horatiu Vultur <horatiu.vultur@microchip.com>
To:     Maciej Fijalkowski <maciej.fijalkowski@intel.com>
CC:     <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <bpf@vger.kernel.org>, <davem@davemloft.net>,
        <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
        <ast@kernel.org>, <daniel@iogearbox.net>, <hawk@kernel.org>,
        <john.fastabend@gmail.com>, <alexandr.lobakin@intel.com>,
        <UNGLinuxDriver@microchip.com>
Subject: Re: [PATCH net-next v4 6/7] net: lan966x: Add support for XDP_TX
Message-ID: <20221123201955.koaobohzf6kcm4ho@soft-dev3-1>
References: <20221122214413.3446006-1-horatiu.vultur@microchip.com>
 <20221122214413.3446006-7-horatiu.vultur@microchip.com>
 <Y31Mu/hAxrmbn7Ws@boxer>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <Y31Mu/hAxrmbn7Ws@boxer>
Precedence: bulk

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@microchip.com>
> > ---
...
> >
> >  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