linux-kernel - Re: [PATCH net-next] net: introduce budget

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL+tcoAwodpnE2NjMLPhBbmHUvmKMgSykqx0EQ4YZaQHjrx0Hw@mail.gmail.com>
Date:   Mon, 13 Mar 2023 10:05:18 +0800
From:   Jason Xing <kerneljasonxing@...il.com>
To:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, ast@...nel.org, daniel@...earbox.net,
        hawk@...nel.org, john.fastabend@...il.com
Cc:     kuniyu@...zon.com, liuhangbin@...il.com, xiangxia.m.yue@...il.com,
        jiri@...dia.com, andy.ren@...cruise.com, bpf@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next] net: introduce budget_squeeze to help us tune rx behavior

On Sun, Mar 12, 2023 at 12:36 AM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> From: Jason Xing <kernelxing@...cent.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@...cent.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 softirq_action *h)
>         unsigned long time_limit = jiffies +
>                 usecs_to_jiffies(READ_ONCE(netdev_budget_usecs));
>         int budget = READ_ONCE(netdev_budget);
> +       bool is_continue = 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 softirq_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 <= 0 ||
> -                            time_after_eq(jiffies, time_limit))) {
> +               if (unlikely(budget <= 0)) {
> +                       sd->budget_squeeze++;
> +                       is_continue = false;
> +               }
> +               if (unlikely(time_after_eq(jiffies, time_limit))) {
>                         sd->time_squeeze++;
> -                       break;
> +                       is_continue = 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, void *v)
>          */
>         seq_printf(seq,
>                    "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %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
>