Jason Xing wrote: > On Fri, Jun 20, 2025 at 9:58 PM Willem de Bruijn > <willemdebruijn.kernel@xxxxxxxxx> wrote: > > > > Willem de Bruijn wrote: > > > Jason Xing wrote: > > > > On Thu, Jun 19, 2025 at 9:53 PM Willem de Bruijn > > > > <willemdebruijn.kernel@xxxxxxxxx> wrote: > > > > > > > > > > Jason Xing wrote: > > > > > > From: Jason Xing <kernelxing@xxxxxxxxxxx> > > > > > > > > > > > > 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@xxxxxxxxxxx> > > > > > > --- > > > > > > V3 > > > > > > Link: https://lore.kernel.org/all/20250618065553.96822-1-kerneljasonxing@xxxxxxxxx/ > > > > > > 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@xxxxxxxxx/ > > > > > > 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.
