MIME-Version: 1.0
References: <20230311163614.92296-1-kerneljasonxing@gmail.com>
In-Reply-To: <20230311163614.92296-1-kerneljasonxing@gmail.com>
From:   Jason Xing <kerneljasonxing@gmail.com>
Date:   Mon, 13 Mar 2023 10:05:18 +0800
Message-ID: <CAL+tcoAwodpnE2NjMLPhBbmHUvmKMgSykqx0EQ4YZaQHjrx0Hw@mail.gmail.com>
Subject: Re: [PATCH net-next] net: introduce budget_squeeze to help us tune rx behavior
To:     davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, ast@kernel.org, daniel@iogearbox.net,
        hawk@kernel.org, john.fastabend@gmail.com
Cc:     kuniyu@amazon.com, liuhangbin@gmail.com, xiangxia.m.yue@gmail.com,
        jiri@nvidia.com, andy.ren@getcruise.com, bpf@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Jason Xing <kernelxing@tencent.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, Mar 12, 2023 at 12:36=E2=80=AFAM Jason Xing <kerneljasonxing@gmail.=
com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> When we encounter some performance issue and then get lost on how
> to tune the budget limit and time limit in net_rx_action() function,
> we can separately counting both of them to avoid the confusion.
>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
> note: this commit is based on the link as below:
> https://lore.kernel.org/lkml/20230311151756.83302-1-kerneljasonxing@gmail=
.com/
> ---
>  include/linux/netdevice.h |  1 +
>  net/core/dev.c            | 12 ++++++++----
>  net/core/net-procfs.c     |  9 ++++++---
>  3 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 6a14b7b11766..5736311a2133 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -3157,6 +3157,7 @@ struct softnet_data {
>         /* stats */
>         unsigned int            processed;
>         unsigned int            time_squeeze;
> +       unsigned int            budget_squeeze;
>  #ifdef CONFIG_RPS
>         struct softnet_data     *rps_ipi_list;
>  #endif
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 253584777101..bed7a68fdb5d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6637,6 +6637,7 @@ static __latent_entropy void net_rx_action(struct s=
oftirq_action *h)
>         unsigned long time_limit =3D jiffies +
>                 usecs_to_jiffies(READ_ONCE(netdev_budget_usecs));
>         int budget =3D READ_ONCE(netdev_budget);
> +       bool is_continue =3D true;

I kept thinking during these days, I think it looks not that concise
and elegant and also the name is not that good though the function can
work.

In the next submission, I'm going to choose to use 'while()' instead
of 'for()' suggested by Stephen.

Does anyone else have some advice about this?

Thanks,
Jason

>         LIST_HEAD(list);
>         LIST_HEAD(repoll);
>
> @@ -6644,7 +6645,7 @@ static __latent_entropy void net_rx_action(struct s=
oftirq_action *h)
>         list_splice_init(&sd->poll_list, &list);
>         local_irq_enable();
>
> -       for (;;) {
> +       for (; is_continue;) {
>                 struct napi_struct *n;
>
>                 skb_defer_free_flush(sd);
> @@ -6662,10 +6663,13 @@ static __latent_entropy void net_rx_action(struct=
 softirq_action *h)
>                  * Allow this to run for 2 jiffies since which will allow
>                  * an average latency of 1.5/HZ.
>                  */
> -               if (unlikely(budget <=3D 0 ||
> -                            time_after_eq(jiffies, time_limit))) {
> +               if (unlikely(budget <=3D 0)) {
> +                       sd->budget_squeeze++;
> +                       is_continue =3D false;
> +               }
> +               if (unlikely(time_after_eq(jiffies, time_limit))) {
>                         sd->time_squeeze++;
> -                       break;
> +                       is_continue =3D false;
>                 }
>         }
>
> diff --git a/net/core/net-procfs.c b/net/core/net-procfs.c
> index 97a304e1957a..4d1a499d7c43 100644
> --- a/net/core/net-procfs.c
> +++ b/net/core/net-procfs.c
> @@ -174,14 +174,17 @@ static int softnet_seq_show(struct seq_file *seq, v=
oid *v)
>          */
>         seq_printf(seq,
>                    "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08=
x %08x %08x "
> -                  "%08x %08x\n",
> -                  sd->processed, sd->dropped, sd->time_squeeze, 0,
> +                  "%08x %08x %08x %08x\n",
> +                  sd->processed, sd->dropped,
> +                  0, /* was old way to count time squeeze */
> +                  0,
>                    0, 0, 0, 0, /* was fastroute */
>                    0,   /* was cpu_collision */
>                    sd->received_rps, flow_limit_count,
>                    0,   /* was len of two backlog queues */
>                    (int)seq->index,
> -                  softnet_input_pkt_queue_len(sd), softnet_process_queue=
_len(sd));
> +                  softnet_input_pkt_queue_len(sd), softnet_process_queue=
_len(sd),
> +                  sd->time_squeeze, sd->budget_squeeze);
>         return 0;
>  }
>
> --
> 2.37.3
>