[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANn89iJaGs6pzTkkzW6eXDtKTcxCHVhz3MdRTQpW12zqY+7+jw@mail.gmail.com>
Date: Thu, 30 Mar 2023 13:44:16 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Jason Xing <kerneljasonxing@...il.com>,
"David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Jason Xing <kernelxing@...cent.com>, netdev@...r.kernel.org,
eric.dumazet@...il.com
Subject: Re: [PATCH net-next 4/4] net: optimize ____napi_schedule() to avoid
extra NET_RX_SOFTIRQ
On Thu, Mar 30, 2023 at 1:39 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On Thu, 2023-03-30 at 17:50 +0800, Jason Xing wrote:
> > On Wed, Mar 29, 2023 at 7:53 AM Eric Dumazet <edumazet@...gle.com> wrote:
> > >
> > > ____napi_schedule() adds a napi into current cpu softnet_data poll_list,
> > > then raises NET_RX_SOFTIRQ to make sure net_rx_action() will process it.
> > >
> > > Idea of this patch is to not raise NET_RX_SOFTIRQ when being called indirectly
> > > from net_rx_action(), because we can process poll_list from this point,
> > > without going to full softirq loop.
> > >
> > > This needs a change in net_rx_action() to make sure we restart
> > > its main loop if sd->poll_list was updated without NET_RX_SOFTIRQ
> > > being raised.
> > >
> > > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > > Cc: Jason Xing <kernelxing@...cent.com>
> > > ---
> > > net/core/dev.c | 22 ++++++++++++++++++----
> > > 1 file changed, 18 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index f34ce93f2f02e7ec71f5e84d449fa99b7a882f0c..0c4b21291348d4558f036fb05842dab023f65dc3 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -4360,7 +4360,11 @@ static inline void ____napi_schedule(struct softnet_data *sd,
> > > }
> > >
> > > list_add_tail(&napi->poll_list, &sd->poll_list);
> > > - __raise_softirq_irqoff(NET_RX_SOFTIRQ);
> > > + /* If not called from net_rx_action()
> > > + * we have to raise NET_RX_SOFTIRQ.
> > > + */
> > > + if (!sd->in_net_rx_action)
> > > + __raise_softirq_irqoff(NET_RX_SOFTIRQ);
> > > }
> > >
> > > #ifdef CONFIG_RPS
> > > @@ -6648,6 +6652,7 @@ static __latent_entropy void net_rx_action(struct softirq_action *h)
> > > LIST_HEAD(list);
> > > LIST_HEAD(repoll);
> > >
> > > +start:
> > > sd->in_net_rx_action = true;
> > > local_irq_disable();
> > > list_splice_init(&sd->poll_list, &list);
> > > @@ -6659,9 +6664,18 @@ static __latent_entropy void net_rx_action(struct softirq_action *h)
> > > skb_defer_free_flush(sd);
> > >
> > > if (list_empty(&list)) {
> > > - sd->in_net_rx_action = false;
> > > - if (!sd_has_rps_ipi_waiting(sd) && list_empty(&repoll))
> > > - goto end;
> > > + if (list_empty(&repoll)) {
> > > + sd->in_net_rx_action = false;
> > > + barrier();
> > > + /* We need to check if ____napi_schedule()
> > > + * had refilled poll_list while
> > > + * sd->in_net_rx_action was true.
> > > + */
> > > + if (!list_empty(&sd->poll_list))
> > > + goto start;
> >
> > I noticed that since we decide to go back and restart this loop, it
> > would be better to check the time_limit. More than that,
> > skb_defer_free_flush() can consume some time which is supposed to take
> > into account.
>
> Note that we can have a __napi_schedule() invocation with sd-
> >in_net_rx_action only after executing the napi_poll() call below and
> thus after the related time check (that is - after performing at least
> one full iteration of the main for(;;) loop).
>
> I don't think another check right here is needed.
I was about to say the same thing.
Back to READ_ONCE() and WRITE_ONCE(), I originally had them in my tree,
then simply realized they were not needed and confusing as a matter of fact.
barrier() is the right thing here, and only one is needed, because
others are implicit,
because before hitting reads, we must call out of line functions.
Powered by blists - more mailing lists