Message-ID: <CAL+tcoD90Vrx__82uMYM2oAX_-kXqR6bi4U0=aXG8PpwPFJ1cg@mail.gmail.com>
Date: Thu, 3 Jul 2025 22:25:51 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, bjorn@...nel.org, magnus.karlsson@...el.com, 
	jonathan.lemon@...il.com, sdf@...ichev.me, ast@...nel.org, 
	daniel@...earbox.net, hawk@...nel.org, john.fastabend@...il.com, joe@...a.to, 
	willemdebruijn.kernel@...il.com, bpf@...r.kernel.org, netdev@...r.kernel.org, 
	Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v6] net: xsk: introduce XDP_MAX_TX_BUDGET set/getsockopt

On Thu, Jul 3, 2025 at 9:09 PM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> Hi Maciej,
>
> Thanks for the review.
>
> On Thu, Jul 3, 2025 at 8:26 PM Maciej Fijalkowski
> <maciej.fijalkowski@...el.com> wrote:
> >
> > On Fri, Jun 27, 2025 at 07:01:21PM +0800, Jason Xing wrote:
> > > From: Jason Xing <kernelxing@...cent.com>
> > >
> > > This patch provides a setsockopt method to let applications leverage to
> > > adjust how many descs to be handled at most in one send syscall. It
> > > mitigates the situation where the default value (32) that is too small
> > > leads to higher frequency of triggering send syscall.
> > >
> > > Considering the prosperity/complexity the applications have, there is no
> > > absolutely ideal suggestion fitting all cases. So keep 32 as its default
> > > value like before.
> > >
> > > The patch does the following things:
> > > - Add XDP_MAX_TX_BUDGET socket option.
> > > - Convert TX_BATCH_SIZE to tx_budget_spent.
> > > - Set tx_budget_spent to 32 by default in the initialization phase as a
> > >   per-socket granular control. 32 is also the min value for
> > >   tx_budget_spent.
> > > - Set the range of tx_budget_spent as [32, xs->tx->nentries].
> > >
> > > The idea behind this comes out of real workloads in production. We use a
> > > user-level stack with xsk support to accelerate sending packets and
> > > minimize triggering syscalls. When the packets are aggregated, it's not
> > > hard to hit the upper bound (namely, 32). The moment user-space stack
> > > fetches the -EAGAIN error number passed from sendto(), it will loop to try
> > > again until all the expected descs from tx ring are sent out to the driver.
> > > Enlarging the XDP_MAX_TX_BUDGET value contributes to less frequency of
> > > sendto() and higher throughput/PPS.
> > >
> > > Here is what I did in production, along with some numbers as follows:
> > > For one application I saw lately, I suggested using 128 as max_tx_budget
> > > because I saw two limitations without changing any default configuration:
> > > 1) XDP_MAX_TX_BUDGET, 2) socket sndbuf which is 212992 decided by
> > > net.core.wmem_default. As to XDP_MAX_TX_BUDGET, the scenario behind
> > > this was I counted how many descs are transmitted to the driver at one
> > > time of sendto() based on [1] patch and then I calculated the
> > > possibility of hitting the upper bound. Finally I chose 128 as a
> > > suitable value because 1) it covers most of the cases, 2) a higher
> > > number would not bring evident results. After twisting the parameters,
> > > a stable improvement of around 4% for both PPS and throughput and less
> > > resources consumption were found to be observed by strace -c -p xxx:
> > > 1) %time was decreased by 7.8%
> > > 2) error counter was decreased from 18367 to 572
> > >
> > > [1]: https://lore.kernel.org/all/20250619093641.70700-1-kerneljasonxing@gmail.com/
> > >
> > > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > > ---
> > > v6
> > > Link: https://lore.kernel.org/all/20250625123527.98209-1-kerneljasonxing@gmail.com/
> > > 1. use [32, xs->tx->nentries] range
> > > 2. Since setsockopt may generate a different value, add getsockopt to help
> > >    application know what value takes effect finally.
> > >
> > > v5
> > > Link: https://lore.kernel.org/all/20250623021345.69211-1-kerneljasonxing@gmail.com/
> > > 1. remove changes around zc mode
> > >
> > > v4
> > > Link: https://lore.kernel.org/all/20250619090440.65509-1-kerneljasonxing@gmail.com/
> > > 1. remove getsockopt as it seems no real use case.
> > > 2. adjust the position of max_tx_budget to make sure it stays with other
> > > read-most fields in one cacheline.
> > > 3. set one as the lower bound of max_tx_budget
> > > 4. add more descriptions/performance data in Doucmentation and commit message.
> > >
> > > V3
> > > Link: https://lore.kernel.org/all/20250618065553.96822-1-kerneljasonxing@gmail.com/
> > > 1. use a per-socket control (suggested by Stanislav)
> > > 2. unify both definitions into one
> > > 3. support setsockopt and getsockopt
> > > 4. add more description in commit message
> > >
> > > V2
> > > Link: https://lore.kernel.org/all/20250617002236.30557-1-kerneljasonxing@gmail.com/
> > > 1. use a per-netns sysctl knob
> > > 2. use sysctl_xsk_max_tx_budget to unify both definitions.
> > > ---
> > >  Documentation/networking/af_xdp.rst |  9 +++++++
> > >  include/net/xdp_sock.h              |  1 +
> > >  include/uapi/linux/if_xdp.h         |  1 +
> > >  net/xdp/xsk.c                       | 39 ++++++++++++++++++++++++++---
> > >  tools/include/uapi/linux/if_xdp.h   |  1 +
> > >  5 files changed, 47 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/networking/af_xdp.rst b/Documentation/networking/af_xdp.rst
> > > index dceeb0d763aa..253afee00162 100644
> > > --- a/Documentation/networking/af_xdp.rst
> > > +++ b/Documentation/networking/af_xdp.rst
> > > @@ -442,6 +442,15 @@ is created by a privileged process and passed to a non-privileged one.
> > >  Once the option is set, kernel will refuse attempts to bind that socket
> > >  to a different interface.  Updating the value requires CAP_NET_RAW.
> > >
> > > +XDP_MAX_TX_BUDGET setsockopt
> > > +----------------------------
> > > +
> > > +This setsockopt sets the maximum number of descriptors that can be handled
> > > +and passed to the driver at one send syscall. It is applied in the non-zero
> >
> > just 'copy mode' would be enough IMHO.
>
> Okay.
>
> >
> > > +copy mode to allow application to tune the per-socket maximum iteration for
> > > +better throughput and less frequency of send syscall.
> > > +Allowed range is [32, xs->tx->nentries].
> > > +
> > >  XDP_STATISTICS getsockopt
> > >  -------------------------
> > >
> > > diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> > > index e8bd6ddb7b12..ce587a225661 100644
> > > --- a/include/net/xdp_sock.h
> > > +++ b/include/net/xdp_sock.h
> > > @@ -84,6 +84,7 @@ struct xdp_sock {
> > >       struct list_head map_list;
> > >       /* Protects map_list */
> > >       spinlock_t map_list_lock;
> > > +     u32 max_tx_budget;
> > >       /* Protects multiple processes in the control path */
> > >       struct mutex mutex;
> > >       struct xsk_queue *fq_tmp; /* Only as tmp storage before bind */
> > > diff --git a/include/uapi/linux/if_xdp.h b/include/uapi/linux/if_xdp.h
> > > index 44f2bb93e7e6..07c6d21c2f1c 100644
> > > --- a/include/uapi/linux/if_xdp.h
> > > +++ b/include/uapi/linux/if_xdp.h
> > > @@ -79,6 +79,7 @@ struct xdp_mmap_offsets {
> > >  #define XDP_UMEM_COMPLETION_RING     6
> > >  #define XDP_STATISTICS                       7
> > >  #define XDP_OPTIONS                  8
> > > +#define XDP_MAX_TX_BUDGET            9
> > >
> > >  struct xdp_umem_reg {
> > >       __u64 addr; /* Start of packet data area */
> > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > > index 72c000c0ae5f..41efe7b27b0e 100644
> > > --- a/net/xdp/xsk.c
> > > +++ b/net/xdp/xsk.c
> > > @@ -33,8 +33,8 @@
> > >  #include "xdp_umem.h"
> > >  #include "xsk.h"
> > >
> > > -#define TX_BATCH_SIZE 32
> > > -#define MAX_PER_SOCKET_BUDGET (TX_BATCH_SIZE)
> > > +#define TX_BUDGET_SIZE 32
> > > +#define MAX_PER_SOCKET_BUDGET 32
> >
> > that could have stayed as it was before?
>
> Sorry, I don't think so because one definition has nothing to do with
> the other one. Why do we need to bind them together?
>
> >
> > >
> > >  void xsk_set_rx_need_wakeup(struct xsk_buff_pool *pool)
> > >  {
> > > @@ -779,7 +779,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> > >  static int __xsk_generic_xmit(struct sock *sk)
> > >  {
> > >       struct xdp_sock *xs = xdp_sk(sk);
> > > -     u32 max_batch = TX_BATCH_SIZE;
> > > +     u32 max_budget = READ_ONCE(xs->max_tx_budget);
> >
> > you're breaking RCT here...
>
> What's the RCT? Never heard of this abbreviation.
>
> > maybe you could READ_ONCE() the budget right
> > before the while() loop and keep the declaration only at the top of func?
> >
> > also nothing wrong with keeping @max_batch as a name for this budget.
> >
> > >       bool sent_frame = false;
> > >       struct xdp_desc desc;
> > >       struct sk_buff *skb;
> > > @@ -797,7 +797,7 @@ static int __xsk_generic_xmit(struct sock *sk)
> > >               goto out;
> > >
> > >       while (xskq_cons_peek_desc(xs->tx, &desc, xs->pool)) {
> > > -             if (max_batch-- == 0) {
> > > +             if (max_budget-- == 0) {
> > >                       err = -EAGAIN;
> > >                       goto out;
> > >               }
> > > @@ -1437,6 +1437,21 @@ static int xsk_setsockopt(struct socket *sock, int level, int optname,
> > >               mutex_unlock(&xs->mutex);
> > >               return err;
> > >       }
> > > +     case XDP_MAX_TX_BUDGET:
> > > +     {
> > > +             unsigned int budget;
> > > +
> > > +             if (optlen != sizeof(budget))
> > > +                     return -EINVAL;
> > > +             if (copy_from_sockptr(&budget, optval, sizeof(budget)))
> > > +                     return -EFAULT;
> > > +             if (!xs->tx || xs->tx->nentries < TX_BUDGET_SIZE)
> > > +                     return -EACCES;
> > > +
> > > +             WRITE_ONCE(xs->max_tx_budget,
> > > +                        clamp(budget, TX_BUDGET_SIZE, xs->tx->nentries));
> >
> > I think it would be better to throw errno when budget set by user is
> > bigger than nentries. you do it for min case, let's do it for max case as
> > well and skip the clamp() altogether?
>
> Okay.
>
> After this, do you think it's necessary to keep the getsockopt()?
>
> >
> > this is rather speculative that someone would ever set such a big budget
> > but silently setting a lower value than provided might be confusing to me.
> > i'd rather get an error thrown at my face and find out the valid range.
>
> On point.
>
> >
> > > +             return 0;
> > > +     }
> > >       default:
> > >               break;
> > >       }
> > > @@ -1588,6 +1603,21 @@ static int xsk_getsockopt(struct socket *sock, int level, int optname,
> > >
> > >               return 0;
> > >       }
> > > +     case XDP_MAX_TX_BUDGET:
> > > +     {
> > > +             unsigned int budget;
> > > +
> > > +             if (len < sizeof(budget))
> > > +                     return -EINVAL;
> > > +
> > > +             budget = READ_ONCE(xs->max_tx_budget);
> > > +             if (copy_to_user(optval, &budget, sizeof(budget)))
> > > +                     return -EFAULT;
> > > +             if (put_user(sizeof(budget), optlen))
> > > +                     return -EFAULT;
> > > +
> > > +             return 0;
> > > +     }
> > >       default:
> > >               break;
> > >       }
> > > @@ -1734,6 +1764,7 @@ static int xsk_create(struct net *net, struct socket *sock, int protocol,
> > >
> > >       xs = xdp_sk(sk);
> > >       xs->state = XSK_READY;
> > > +     xs->max_tx_budget = TX_BUDGET_SIZE;
> > >       mutex_init(&xs->mutex);
> > >
> > >       INIT_LIST_HEAD(&xs->map_list);
> > > diff --git a/tools/include/uapi/linux/if_xdp.h b/tools/include/uapi/linux/if_xdp.h
> > > index 44f2bb93e7e6..07c6d21c2f1c 100644
> > > --- a/tools/include/uapi/linux/if_xdp.h
> > > +++ b/tools/include/uapi/linux/if_xdp.h
> > > @@ -79,6 +79,7 @@ struct xdp_mmap_offsets {
> > >  #define XDP_UMEM_COMPLETION_RING     6
> > >  #define XDP_STATISTICS                       7
> > >  #define XDP_OPTIONS                  8
> > > +#define XDP_MAX_TX_BUDGET            9
> >
> > could we have 'SKB' in name?
>
> Like XDP_MAX_TX_BUDGET_SKB ?

XDP_MAX_TX_SKB_BUDGET sounds better, I assume.

Thanks,
Jason

Powered by blists - more mailing lists