[PATCH v3 net-next 0/2] add some detailed data when reading softnet_stat

[PATCH v3 net-next 0/2] add some detailed data when reading softnet_stat

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Adding more detailed display of softnet_data when cating
/proc/net/softnet_stat, which could help users understand more about
which can be the bottlneck and then tune.

Based on what we've dicussed in the previous mails, we could implement it
in different ways, like put those display into separate sysfs file or add
some tracepoints. Still I chose to touch the legacy file to print more
useful data without changing some old data, say, length of backlog queues
and time_squeeze.

After this, we wouldn't alter the behavior some user-space tools get used
to meanwhile we could show more data.

Jason Xing (2):
  net-sysfs: display two backlog queue len separately
  net: introduce budget_squeeze to help us tune rx behavior

 include/linux/netdevice.h |  1 +
 net/core/dev.c            | 12 ++++++++----
 net/core/net-procfs.c     | 25 ++++++++++++++++++++-----
 3 files changed, 29 insertions(+), 9 deletions(-)

-- 
2.37.3

[PATCH v3 net-next 1/2] net-sysfs: display two backlog queue len separately

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Sometimes we need to know which one of backlog queue can be exactly
long enough to cause some latency when debugging this part is needed.
Thus, we can then separate the display of both.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
---
v3: drop the comment suggested by Simon
Link: https://lore.kernel.org/lkml/20230314030532.9238-2-kerneljasonxing@gmail.com/

v2: keep the total len of backlog queues untouched as Eric said
Link: https://lore.kernel.org/lkml/20230311151756.83302-1-kerneljasonxing@gmail.com/
---
 net/core/net-procfs.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/net/core/net-procfs.c b/net/core/net-procfs.c
index 1ec23bf8b05c..8056f39da8a1 100644
--- a/net/core/net-procfs.c
+++ b/net/core/net-procfs.c
@@ -115,10 +115,19 @@ static int dev_seq_show(struct seq_file *seq, void *v)
 	return 0;
 }
 
+static u32 softnet_input_pkt_queue_len(struct softnet_data *sd)
+{
+	return skb_queue_len_lockless(&sd->input_pkt_queue);
+}
+
+static u32 softnet_process_queue_len(struct softnet_data *sd)
+{
+	return skb_queue_len_lockless(&sd->process_queue);
+}
+
 static u32 softnet_backlog_len(struct softnet_data *sd)
 {
-	return skb_queue_len_lockless(&sd->input_pkt_queue) +
-	       skb_queue_len_lockless(&sd->process_queue);
+	return softnet_input_pkt_queue_len(sd) + softnet_process_queue_len(sd);
 }
 
 static struct softnet_data *softnet_get_online(loff_t *pos)
@@ -169,12 +178,14 @@ static int softnet_seq_show(struct seq_file *seq, void *v)
 	 * mapping the data a specific CPU
 	 */
 	seq_printf(seq,
-		   "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x\n",
+		   "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x "
+		   "%08x %08x\n",
 		   sd->processed, sd->dropped, sd->time_squeeze, 0,
 		   0, 0, 0, 0, /* was fastroute */
 		   0,	/* was cpu_collision */
 		   sd->received_rps, flow_limit_count,
-		   softnet_backlog_len(sd), (int)seq->index);
+		   softnet_backlog_len(sd), (int)seq->index,
+		   softnet_input_pkt_queue_len(sd), softnet_process_queue_len(sd));
 	return 0;
 }
 
-- 
2.37.3

[PATCH v3 net-next 2/2] net: introduce budget_squeeze to help us tune rx behavior

[Jason Xing]

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>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
---
v3:
1) drop the comment suggested by Simon
Link: https://lore.kernel.org/lkml/20230314030532.9238-3-kerneljasonxing@gmail.com/

v2:
1) change the coding style suggested by Stephen and Simon
2) Keep the display of the old data (time_squeeze) untouched suggested
by Kui-Feng
Link: https://lore.kernel.org/lkml/20230311163614.92296-1-kerneljasonxing@gmail.com/
---
 include/linux/netdevice.h |  1 +
 net/core/dev.c            | 12 ++++++++----
 net/core/net-procfs.c     |  8 +++++---
 3 files changed, 14 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..1518a366783b 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 done = false;
 	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 (;;) {
+	while (!done) {
 		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++;
+			done = true;
+		}
+		if (unlikely(time_after_eq(jiffies, time_limit))) {
 			sd->time_squeeze++;
-			break;
+			done = true;
 		}
 	}
 
diff --git a/net/core/net-procfs.c b/net/core/net-procfs.c
index 8056f39da8a1..3b53812a9ac9 100644
--- a/net/core/net-procfs.c
+++ b/net/core/net-procfs.c
@@ -179,13 +179,15 @@ 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,
+		   sd->time_squeeze + sd->budget_squeeze, 0,
 		   0, 0, 0, 0, /* was fastroute */
 		   0,	/* was cpu_collision */
 		   sd->received_rps, flow_limit_count,
 		   softnet_backlog_len(sd), (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

Re: [PATCH v3 net-next 1/2] net-sysfs: display two backlog queue len separately

[Eric Dumazet]

On Tue, Mar 14, 2023 at 6:14 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> Sometimes we need to know which one of backlog queue can be exactly
> long enough to cause some latency when debugging this part is needed.
> Thus, we can then separate the display of both.
>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> ---
> v3: drop the comment suggested by Simon
> Link: https://lore.kernel.org/lkml/20230314030532.9238-2-kerneljasonxing@gmail.com/
>
> v2: keep the total len of backlog queues untouched as Eric said
> Link: https://lore.kernel.org/lkml/20230311151756.83302-1-kerneljasonxing@gmail.com/
> ---
>  net/core/net-procfs.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/net/core/net-procfs.c b/net/core/net-procfs.c
> index 1ec23bf8b05c..8056f39da8a1 100644
> --- a/net/core/net-procfs.c
> +++ b/net/core/net-procfs.c
> @@ -115,10 +115,19 @@ static int dev_seq_show(struct seq_file *seq, void *v)
>         return 0;
>  }
>
> +static u32 softnet_input_pkt_queue_len(struct softnet_data *sd)
> +{
> +       return skb_queue_len_lockless(&sd->input_pkt_queue);
> +}
> +
> +static u32 softnet_process_queue_len(struct softnet_data *sd)
> +{
> +       return skb_queue_len_lockless(&sd->process_queue);
> +}
> +
>  static u32 softnet_backlog_len(struct softnet_data *sd)
>  {
> -       return skb_queue_len_lockless(&sd->input_pkt_queue) +
> -              skb_queue_len_lockless(&sd->process_queue);
> +       return softnet_input_pkt_queue_len(sd) + softnet_process_queue_len(sd);
>  }
>
>  static struct softnet_data *softnet_get_online(loff_t *pos)
> @@ -169,12 +178,14 @@ static int softnet_seq_show(struct seq_file *seq, void *v)
>          * mapping the data a specific CPU
>          */
>         seq_printf(seq,
> -                  "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x\n",
> +                  "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x "
> +                  "%08x %08x\n",
>                    sd->processed, sd->dropped, sd->time_squeeze, 0,
>                    0, 0, 0, 0, /* was fastroute */
>                    0,   /* was cpu_collision */
>                    sd->received_rps, flow_limit_count,
> -                  softnet_backlog_len(sd), (int)seq->index);
> +                  softnet_backlog_len(sd), (int)seq->index,
> +                  softnet_input_pkt_queue_len(sd), softnet_process_queue_len(sd));
>         return 0;


It is customary to wait ~24 hours between each version, so that
everybody gets a chance to comment,
and to avoid polluting mailing lists with too many messages/day.

(I see you are including lkml@, which seems unnecessary for this kind of patch)

Please address the feedback I gave for v2.

Thanks.

Re: [PATCH v3 net-next 1/2] net-sysfs: display two backlog queue len separately

[Jason Xing]

On Tue, Mar 14, 2023 at 11:15 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Mar 14, 2023 at 6:14 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Sometimes we need to know which one of backlog queue can be exactly
> > long enough to cause some latency when debugging this part is needed.
> > Thus, we can then separate the display of both.
> >
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > Reviewed-by: Simon Horman <simon.horman@corigine.com>
> > ---
> > v3: drop the comment suggested by Simon
> > Link: https://lore.kernel.org/lkml/20230314030532.9238-2-kerneljasonxing@gmail.com/
> >
> > v2: keep the total len of backlog queues untouched as Eric said
> > Link: https://lore.kernel.org/lkml/20230311151756.83302-1-kerneljasonxing@gmail.com/
> > ---
> >  net/core/net-procfs.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/core/net-procfs.c b/net/core/net-procfs.c
> > index 1ec23bf8b05c..8056f39da8a1 100644
> > --- a/net/core/net-procfs.c
> > +++ b/net/core/net-procfs.c
> > @@ -115,10 +115,19 @@ static int dev_seq_show(struct seq_file *seq, void *v)
> >         return 0;
> >  }
> >
> > +static u32 softnet_input_pkt_queue_len(struct softnet_data *sd)
> > +{
> > +       return skb_queue_len_lockless(&sd->input_pkt_queue);
> > +}
> > +
> > +static u32 softnet_process_queue_len(struct softnet_data *sd)
> > +{
> > +       return skb_queue_len_lockless(&sd->process_queue);
> > +}
> > +
> >  static u32 softnet_backlog_len(struct softnet_data *sd)
> >  {
> > -       return skb_queue_len_lockless(&sd->input_pkt_queue) +
> > -              skb_queue_len_lockless(&sd->process_queue);
> > +       return softnet_input_pkt_queue_len(sd) + softnet_process_queue_len(sd);
> >  }
> >
> >  static struct softnet_data *softnet_get_online(loff_t *pos)
> > @@ -169,12 +178,14 @@ static int softnet_seq_show(struct seq_file *seq, void *v)
> >          * mapping the data a specific CPU
> >          */
> >         seq_printf(seq,
> > -                  "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x\n",
> > +                  "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x "
> > +                  "%08x %08x\n",
> >                    sd->processed, sd->dropped, sd->time_squeeze, 0,
> >                    0, 0, 0, 0, /* was fastroute */
> >                    0,   /* was cpu_collision */
> >                    sd->received_rps, flow_limit_count,
> > -                  softnet_backlog_len(sd), (int)seq->index);
> > +                  softnet_backlog_len(sd), (int)seq->index,
> > +                  softnet_input_pkt_queue_len(sd), softnet_process_queue_len(sd));
> >         return 0;
>
>
[...]
> It is customary to wait ~24 hours between each version, so that
> everybody gets a chance to comment,
> and to avoid polluting mailing lists with too many messages/day.

Thanks for your reminder.

>
> (I see you are including lkml@, which seems unnecessary for this kind of patch)

Yes, I alway do the get_maintainers.pl to check before I submit. So
I'll remove the lkml@.

>
> Please address the feedback I gave for v2.

Sure :)

Thanks,
Jason

>
> Thanks.

Re: [PATCH v3 net-next 2/2] net: introduce budget_squeeze to help us tune rx behavior

[Jakub Kicinski]

On Tue, 14 Mar 2023 21:14:27 +0800 Jason Xing wrote:
> 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.

More details please, we can't tell whether your solution makes sense 
if we don't know what your problem is.

Re: [PATCH v3 net-next 2/2] net: introduce budget_squeeze to help us tune rx behavior

[Jason Xing]

On Wed, Mar 15, 2023 at 1:09 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 14 Mar 2023 21:14:27 +0800 Jason Xing wrote:
> > 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.
>
[...]
> More details please, we can't tell whether your solution makes sense
> if we don't know what your problem is.

Roger that. I'll write more details into the commit message.

Thanks,
Jason