Message-ID: <6855dedf90dec_1ca4329463@willemb.c.googlers.com.notmuch>
Date: Fri, 20 Jun 2025 18:21:19 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: davem@...emloft.net, 
 edumazet@...gle.com, 
 kuba@...nel.org, 
 pabeni@...hat.com, 
 bjorn@...nel.org, 
 magnus.karlsson@...el.com, 
 maciej.fijalkowski@...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, 
 bpf@...r.kernel.org, 
 netdev@...r.kernel.org, 
 Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v3] net: xsk: introduce XDP_MAX_TX_BUDGET
 set/getsockopt

Jason Xing wrote:
> On Fri, Jun 20, 2025 at 9:58 PM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > Willem de Bruijn wrote:
> > > Jason Xing wrote:
> > > > On Thu, Jun 19, 2025 at 9:53 PM Willem de Bruijn
> > > > <willemdebruijn.kernel@...il.com> wrote:
> > > > >
> > > > > Jason Xing wrote:
> > > > > > From: Jason Xing <kernelxing@...cent.com>
> > > > > >
> > > > > > The patch does the following things:
> > > > > > - Add XDP_MAX_TX_BUDGET socket option.
> > > > > > - Unify TX_BATCH_SIZE and MAX_PER_SOCKET_BUDGET into single one
> > > > > >   tx_budget_spent.
> > > > > > - tx_budget_spent is set to 32 by default in the initialization phase.
> > > > > >   It's a per-socket granular control.
> > > > > >
> > > > > > 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 syscall. 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 frequencies of
> > > > > > sendto(). Besides, applications leveraging this setsockopt can adjust
> > > > > > its proper value in time after noticing the upper bound issue happening.
> > > > > >
> > > > > > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > > > > > ---
> > > > > > 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
> > > > >
> > > > > +1 on an XSK setsockopt only
> > > >
> > > > May I ask why only setsockopt? In tradition, dev_tx_weight can be read
> > > > and written through running sysctl. I think they are the same?
> > >
> > > This is not dev_tx_weight, which is per device.
> > >
> > > This is a per-socket choice. The reason for adding it that you gave,
> > > a specific application that is known to be able to batch more than 32,
> > > can tune this configurable in the application.
> 
> I was thinking a pair is needed like some existing options I'm
> familiar with like TCP_RTO_MAX_MS. As I said, it's just a feeling.
> 
> Okay, I have no strong opinion on this. I will remove it then.
> 
> > >
> > > I see no immediately need to set this at a per netns or global level.
> > > If so, the extra cacheline space in those structs is not warranted.
> > >
> > > > >
> > > > > >
> > > > > > 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.
> > > > > > ---
> > > > > >  include/net/xdp_sock.h            |  3 ++-
> > > > > >  include/uapi/linux/if_xdp.h       |  1 +
> > > > > >  net/xdp/xsk.c                     | 36 +++++++++++++++++++++++++------
> > > > > >  tools/include/uapi/linux/if_xdp.h |  1 +
> > > > > >  4 files changed, 34 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> > > > > > index e8bd6ddb7b12..8eecafad92c0 100644
> > > > > > --- a/include/net/xdp_sock.h
> > > > > > +++ b/include/net/xdp_sock.h
> > > > > > @@ -65,11 +65,12 @@ struct xdp_sock {
> > > > > >       struct xsk_queue *tx ____cacheline_aligned_in_smp;
> > > > > >       struct list_head tx_list;
> > > > > >       /* record the number of tx descriptors sent by this xsk and
> > > > > > -      * when it exceeds MAX_PER_SOCKET_BUDGET, an opportunity needs
> > > > > > +      * when it exceeds max_tx_budget, an opportunity needs
> > > > > >        * to be given to other xsks for sending tx descriptors, thereby
> > > > > >        * preventing other XSKs from being starved.
> > > > > >        */
> > > > > >       u32 tx_budget_spent;
> > > > > > +     u32 max_tx_budget;
> > > > >
> > > > > This probably does not need to be a u32?
> > > >
> > > > From what I've known, it's not possible to set a very large value like
> > > > 1000 which probably brings side effects.
> > > >
> > > > But it seems we'd better not limit the use of this max_tx_budget? We
> > > > don't know what the best fit for various use cases is. If the type
> > > > needs to be downsized to a smaller one like u16, another related
> > > > consideration is that dev_tx_weight deserves the same treatment?
> > >
> > > If the current constant is 32, is U16_MAX really a limiting factor.
> > > See also the next point.
> > >
> > > > Or let me adjust to 'int' then?
> > > > > > @@ -1437,6 +1436,18 @@ 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;
> > > > > > +
> > > > > > +             WRITE_ONCE(xs->max_tx_budget, budget);
> > > > >
> > > > > Sanitize input: bounds check
> > > >
> > > > Thanks for catching this.
> > > >
> > > > I will change it like this:
> > > >     WRITE_ONCE(xs->max_tx_budget, min_t(int, budget, INT_MAX));?
> > >
> > > INT_MAX is not a valid upper bound. The current constant is 32.
> > > I would expect an upper bound to perhaps be a few orders larger.
> >
> > And this would need a clamp to also set a lower bound greater than 0.
> 
> Sorry, I don't fully follow here. I'm worried if I understand it in
> the wrong way.
> 
> In this patch, max_tx_budget is u32. If we're doing this this:
>         case XDP_MAX_TX_BUDGET:
>         {
>                 unsigned int budget;
> 
>                 if (optlen != sizeof(budget))    // this line can
> filter out those unmatched numbers, right?
>                         return -EINVAL;
>                 if (copy_from_sockptr(&budget, optval,
> sizeof(budget)))
>                         return -EFAULT;
> 
>                 WRITE_ONCE(xs->max_tx_budget, budget);
> 
>                 return 0;
>         }
> , I'm thinking, is it sufficient because u32 makes sure of zero as its
> lower bound?

The issue is that 0 is not a valid budget.

__xsk_generic_xmit will not make any progress as it will break out
immediately at the start of the while loop.

Powered by blists - more mailing lists