Re: [PATCH net-next 5/5] ixgbe: xsk: add TX multi-buffer support

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Fri, Jul 25, 2025 at 06:09:09PM +0800, Jason Xing wrote:
> Hi Maciej,
> 
> On Fri, Jul 25, 2025 at 6:00 PM Maciej Fijalkowski
> <maciej.fijalkowski@intel.com> wrote:
> >
> > On Sun, Jul 20, 2025 at 05:11:23PM +0800, Jason Xing wrote:
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > Use the common interface to see if the desc is the end of packets. If
> > > so, set IXGBE_TXD_CMD_EOP bit instead of setting for all preceding
> > > descriptors. This is also how i40e driver did in commit a92b96c4ae10
> > > ("i40e: xsk: add TX multi-buffer support").
> > >
> > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > ---
> > >  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++++
> > >  drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c  | 4 +++-
> > >  2 files changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > > index a59fd8f74b5e..c34737065f9e 100644
> > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > > @@ -52,6 +52,8 @@
> > >  #include "ixgbe_txrx_common.h"
> > >  #include "devlink/devlink.h"
> > >
> > > +#define IXGBE_MAX_BUFFER_TXD 4
> > > +
> > >  char ixgbe_driver_name[] = "ixgbe";
> > >  static const char ixgbe_driver_string[] =
> > >                             "Intel(R) 10 Gigabit PCI Express Network Driver";
> > > @@ -11805,6 +11807,8 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > >       netdev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
> > >                              NETDEV_XDP_ACT_XSK_ZEROCOPY;
> > >
> > > +     netdev->xdp_zc_max_segs = IXGBE_MAX_BUFFER_TXD;
> >
> > Hi Jason,
> >
> > nack to this as you would allow fragmented frames on Rx side which is not
> > supported even with your patchset.
> 
> I'm not sure about this one, to be honest when I observed no
> performance impact with this patch. How could we support the idea of
> this patch, I wonder? Do we need to correspondingly adjust the
> hardware? Sorry that I wasn't able to find such information in the
> datasheet :(

setting xdp_zc_max_segs will cause xsk control path to allow multi-buffer
traffic for zero-copy driver. ixgbe_clean_rx_irq_zc() is not adjusted for
that.

> 
> >
> > Generally ixgbe needs some love, i have several patches in my backlog plus
> > I think Larysa will be focusing on this driver.
> 
> Though ixgbe is an old driver, we still have thousands of machines
> running with this driver. Looking forward to your patch then.

I am taking a note here and will try to raise the priority around ixgbe
work, thanks!

> 
> Thanks,
> Jason
> 
> >
> > please stick to enabling xsk batching on tx side.
> >
> > > +
> > >       /* MTU range: 68 - 9710 */
> > >       netdev->min_mtu = ETH_MIN_MTU;
> > >       netdev->max_mtu = IXGBE_MAX_JUMBO_FRAME_SIZE - (ETH_HLEN + ETH_FCS_LEN);
> > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > > index 9fe2c4bf8bc5..3d9fa4f2403e 100644
> > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
> > > @@ -424,7 +424,9 @@ static void ixgbe_xmit_pkt(struct ixgbe_ring *xdp_ring, struct xdp_desc *desc,
> > >       cmd_type = IXGBE_ADVTXD_DTYP_DATA |
> > >                  IXGBE_ADVTXD_DCMD_DEXT |
> > >                  IXGBE_ADVTXD_DCMD_IFCS;
> > > -     cmd_type |= desc[i].len | IXGBE_TXD_CMD_EOP;
> > > +     cmd_type |= desc[i].len;
> > > +     if (xsk_is_eop_desc(&desc[i]))
> > > +             cmd_type |= IXGBE_TXD_CMD_EOP;
> > >       tx_desc->read.cmd_type_len = cpu_to_le32(cmd_type);
> > >       tx_desc->read.olinfo_status =
> > >               cpu_to_le32(desc[i].len << IXGBE_ADVTXD_PAYLEN_SHIFT);
> > > --
> > > 2.41.3
> > >