lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CABKxMyMieNNMXFMTRdof1W43ijvZq5e04nOkXFv5djzadXh0xQ@mail.gmail.com> Date: Mon, 16 Oct 2023 16:54:37 +0800 From: 黄杰 <huangjie.albert@...edance.com> To: Magnus Karlsson <magnus.karlsson@...il.com> Cc: Björn Töpel <bjorn@...nel.org>, Magnus Karlsson <magnus.karlsson@...el.com>, Jonathan Lemon <jonathan.lemon@...il.com>, Maciej Fijalkowski <maciej.fijalkowski@...el.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, Jesper Dangaard Brouer <hawk@...nel.org>, John Fastabend <john.fastabend@...il.com>, "open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>, "open list:XDP (eXpress Data Path)" <bpf@...r.kernel.org>, linux-kernel <linux-kernel@...r.kernel.org> Subject: Re: Re: [PATCH v2 net-next] xsk: Avoid starving xsk at the end of the list Magnus Karlsson <magnus.karlsson@...il.com> 于2023年10月16日周一 14:41写道: > > On Mon, 16 Oct 2023 at 05:17, Albert Huang > <huangjie.albert@...edance.com> wrote: > > > > In the previous implementation, when multiple xsk sockets were > > associated with a single xsk_buff_pool, a situation could arise > > where the xsk_tx_list maintained data at the front for one xsk > > socket while starving the xsk sockets at the back of the list. > > This could result in issues such as the inability to transmit packets, > > increased latency, and jitter. To address this problem, we introduced > > a new variable called tx_budget_cache, which limits each xsk to transmit > > a maximum of MAX_XSK_TX_BUDGET tx descriptors. This allocation ensures > > equitable opportunities for subsequent xsk sockets to send tx descriptors. > > The value of MAX_XSK_TX_BUDGET is temporarily set to 16. > > Hi Albert. Yes you are correct that there is nothing hindering this to > happen in the code at the moment, so let us fix it. > thanks. > > Signed-off-by: Albert Huang <huangjie.albert@...edance.com> > > --- > > include/net/xdp_sock.h | 6 ++++++ > > net/xdp/xsk.c | 18 ++++++++++++++++++ > > 2 files changed, 24 insertions(+) > > > > diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h > > index 69b472604b86..f617ff54e38c 100644 > > --- a/include/net/xdp_sock.h > > +++ b/include/net/xdp_sock.h > > @@ -44,6 +44,7 @@ struct xsk_map { > > struct xdp_sock __rcu *xsk_map[]; > > }; > > > > +#define MAX_XSK_TX_BUDGET 16 > > I think something like MAX_PER_SOCKET_BUDGET would be clearer. > OK, this will be considered in the next patch. > > struct xdp_sock { > > /* struct sock must be the first member of struct xdp_sock */ > > struct sock sk; > > @@ -63,6 +64,11 @@ struct xdp_sock { > > > > struct xsk_queue *tx ____cacheline_aligned_in_smp; > > struct list_head tx_list; > > + /* Record the actual number of times xsk has transmitted a tx > > + * descriptor, with a maximum limit not exceeding MAX_XSK_TX_BUDGET > > + */ > > + u32 tx_budget_cache; > > + > > /* Protects generic receive. */ > > spinlock_t rx_lock; > > > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c > > index f5e96e0d6e01..087f2675333c 100644 > > --- a/net/xdp/xsk.c > > +++ b/net/xdp/xsk.c > > @@ -413,16 +413,25 @@ EXPORT_SYMBOL(xsk_tx_release); > > > > bool xsk_tx_peek_desc(struct xsk_buff_pool *pool, struct xdp_desc *desc) > > { > > + u32 xsk_full_count = 0; > > Enough with a bool; > > > struct xdp_sock *xs; > > > > rcu_read_lock(); > > +again: > > list_for_each_entry_rcu(xs, &pool->xsk_tx_list, tx_list) { > > + if (xs->tx_budget_cache >= MAX_XSK_TX_BUDGET) { > > + xsk_full_count++; > > + continue; > > + } > > The problem here is that the fixed MAX_XSK_TX_BUDGET is only useful > for the <= 2 socket case. If I have 3 sockets sharing a > netdev/queue_id, the two first sockets can still starve the third one > since the total budget per send is 32. Why is there a limit of 32? I'm not quite clear on the implications of these, Did I miss something? BR Albert >You need to go through the list > of sockets in the beginning to compute the MAX_XSK_TX_BUDGET to > compute this dynamically before each call. Or cache this value > somehow, in the pool for example. Actually, the refcount in the > buf_pool will tell you how many sockets are sharing the same buf_pool. > Try using that to form MAX_XSK_TX_BUDGET on the fly. > > Another simpler way of accomplishing this would be to just reorder the > list every time. Put the first socket last in the list every time. The > drawback of this is that you need to hold the xsk_tx_list_lock while > doing this so might be slower. The per socket batch size would also be > 32 and you would not receive "fairness" over a single call to > sendto(). Would that be a problem for you? > Yes, I did consider this approach, but I abandoned it because it would lose the performance advantages of lock-free operations(RCU read) thanks Albert > > + > > if (!xskq_cons_peek_desc(xs->tx, desc, pool)) { > > if (xskq_has_descs(xs->tx)) > > xskq_cons_release(xs->tx); > > continue; > > } > > > > + xs->tx_budget_cache++; > > + > > /* This is the backpressure mechanism for the Tx path. > > * Reserve space in the completion queue and only proceed > > * if there is space in it. This avoids having to implement > > @@ -436,6 +445,14 @@ bool xsk_tx_peek_desc(struct xsk_buff_pool *pool, struct xdp_desc *desc) > > return true; > > } > > > > + if (unlikely(xsk_full_count > 0)) { > > + list_for_each_entry_rcu(xs, &pool->xsk_tx_list, tx_list) { > > + xs->tx_budget_cache = 0; > > + } > > + xsk_full_count = 0; > > + goto again; > > + } this section of code only enters when it's unable to acquire any TX descriptors and xsk_full_count > 0. > > + > > out: > > rcu_read_unlock(); > > return false; > > @@ -1230,6 +1247,7 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len) > > xs->zc = xs->umem->zc; > > xs->sg = !!(xs->umem->flags & XDP_UMEM_SG_FLAG); > > xs->queue_id = qid; > > + xs->tx_budget_cache = 0; > > xp_add_xsk(xs->pool, xs); > > > > out_unlock: > > -- > > 2.20.1 > > > >
Powered by blists - more mailing lists