Message-ID: <CABKxMyM_jGBWK1g8Hb145PEBui_p1RCg-uGm5Sjtb4injVD3Jw@mail.gmail.com>
Date: Tue, 17 Oct 2023 14:54:57 +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: [PATCH v2 net-next] xsk: Avoid starving xsk at the end of the list

Magnus Karlsson <magnus.karlsson@...il.com> 于2023年10月16日周一 17:13写道：
>
> On Mon, 16 Oct 2023 at 10:54, 黄杰 <huangjie.albert@...edance.com> wrote:
> >
> > 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
>
> There is a define TX_BATCH_SIZE 32 that controls the max number of
> packets a sendto() call can send before it exits. It is used in
> __xsk_generic_xmit().

OK,I got it . I missed the logic here. I will reconsider the logic in this part.
Thanks
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
>
> OK, then let us not consider it and try to make your current approach work.
>
> >
> > > > +
> > > >                 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