linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 net-next 0/2] add some detailed data when reading softnet_stat
@ 2023-03-14  3:05 Jason Xing
  2023-03-14  3:05 ` [PATCH v2 net-next 1/2] net-sysfs: display two backlog queue len separately Jason Xing
  2023-03-14  3:05 ` [PATCH v2 net-next 2/2] net: introduce budget_squeeze to help us tune rx behavior Jason Xing
  0 siblings, 2 replies; 8+ messages in thread
From: Jason Xing @ 2023-03-14  3:05 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni, ast, daniel, hawk, john.fastabend,
	stephen, simon.horman, sinquersw
  Cc: bpf, netdev, linux-kernel, kerneljasonxing, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 net-next 1/2] net-sysfs: display two backlog queue len separately
  2023-03-14  3:05 [PATCH v2 net-next 0/2] add some detailed data when reading softnet_stat Jason Xing
@ 2023-03-14  3:05 ` Jason Xing
  2023-03-14 11:37   ` Simon Horman
  2023-03-14 14:59   ` Eric Dumazet
  2023-03-14  3:05 ` [PATCH v2 net-next 2/2] net: introduce budget_squeeze to help us tune rx behavior Jason Xing
  1 sibling, 2 replies; 8+ messages in thread
From: Jason Xing @ 2023-03-14  3:05 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni, ast, daniel, hawk, john.fastabend,
	stephen, simon.horman, sinquersw
  Cc: bpf, netdev, linux-kernel, kerneljasonxing, 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>
---
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 | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/net/core/net-procfs.c b/net/core/net-procfs.c
index 1ec23bf8b05c..2809b663e78d 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,15 @@ 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),	/* keep it untouched */
+		   (int)seq->index,
+		   softnet_input_pkt_queue_len(sd), softnet_process_queue_len(sd));
 	return 0;
 }
 
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 net-next 2/2] net: introduce budget_squeeze to help us tune rx behavior
  2023-03-14  3:05 [PATCH v2 net-next 0/2] add some detailed data when reading softnet_stat Jason Xing
  2023-03-14  3:05 ` [PATCH v2 net-next 1/2] net-sysfs: display two backlog queue len separately Jason Xing
@ 2023-03-14  3:05 ` Jason Xing
  2023-03-14 12:03   ` Simon Horman
  1 sibling, 1 reply; 8+ messages in thread
From: Jason Xing @ 2023-03-14  3:05 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni, ast, daniel, hawk, john.fastabend,
	stephen, simon.horman, sinquersw
  Cc: bpf, netdev, linux-kernel, kerneljasonxing, 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>
---
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     |  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..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 2809b663e78d..25810ee46a04 100644
--- a/net/core/net-procfs.c
+++ b/net/core/net-procfs.c
@@ -179,14 +179,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,
+		   sd->time_squeeze + sd->budget_squeeze, /* keep it untouched */
+		   0,
 		   0, 0, 0, 0, /* was fastroute */
 		   0,	/* was cpu_collision */
 		   sd->received_rps, flow_limit_count,
 		   softnet_backlog_len(sd),	/* keep it untouched */
 		   (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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 net-next 1/2] net-sysfs: display two backlog queue len separately
  2023-03-14  3:05 ` [PATCH v2 net-next 1/2] net-sysfs: display two backlog queue len separately Jason Xing
@ 2023-03-14 11:37   ` Simon Horman
  2023-03-14 14:59   ` Eric Dumazet
  1 sibling, 0 replies; 8+ messages in thread
From: Simon Horman @ 2023-03-14 11:37 UTC (permalink / raw)
  To: Jason Xing
  Cc: davem, edumazet, kuba, pabeni, ast, daniel, hawk, john.fastabend,
	stephen, sinquersw, bpf, netdev, linux-kernel, Jason Xing

On Tue, Mar 14, 2023 at 11:05:31AM +0800, Jason Xing 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>
> ---
> 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 | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/net/core/net-procfs.c b/net/core/net-procfs.c
> index 1ec23bf8b05c..2809b663e78d 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,15 @@ 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),	/* keep it untouched */

I'm not sure the comment on the line above buys
us much outside of the context of development of this patch.

Likewise in patch 2/2.

That not withstanding, this looks good to me.

Reviewed-by: Simon Horman <simon.horman@corigine.com>


> +		   (int)seq->index,
> +		   softnet_input_pkt_queue_len(sd), softnet_process_queue_len(sd));
>  	return 0;
>  }
>  
> -- 
> 2.37.3
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 net-next 2/2] net: introduce budget_squeeze to help us tune rx behavior
  2023-03-14  3:05 ` [PATCH v2 net-next 2/2] net: introduce budget_squeeze to help us tune rx behavior Jason Xing
@ 2023-03-14 12:03   ` Simon Horman
  2023-03-14 12:31     ` Jason Xing
  0 siblings, 1 reply; 8+ messages in thread
From: Simon Horman @ 2023-03-14 12:03 UTC (permalink / raw)
  To: Jason Xing
  Cc: davem, edumazet, kuba, pabeni, ast, daniel, hawk, john.fastabend,
	stephen, sinquersw, bpf, netdev, linux-kernel, Jason Xing

On Tue, Mar 14, 2023 at 11:05:32AM +0800, Jason Xing 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>

As per my comment on patch 1/2, I'd drop the "/* keep it untouched */"
comment.

That notwithstanding:

Reviewed-by: Simon Horman <simon.horman@corigine.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     |  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..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 2809b663e78d..25810ee46a04 100644
> --- a/net/core/net-procfs.c
> +++ b/net/core/net-procfs.c
> @@ -179,14 +179,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,
> +		   sd->time_squeeze + sd->budget_squeeze, /* keep it untouched */
> +		   0,
>  		   0, 0, 0, 0, /* was fastroute */
>  		   0,	/* was cpu_collision */
>  		   sd->received_rps, flow_limit_count,
>  		   softnet_backlog_len(sd),	/* keep it untouched */
>  		   (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
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 net-next 2/2] net: introduce budget_squeeze to help us tune rx behavior
  2023-03-14 12:03   ` Simon Horman
@ 2023-03-14 12:31     ` Jason Xing
  0 siblings, 0 replies; 8+ messages in thread
From: Jason Xing @ 2023-03-14 12:31 UTC (permalink / raw)
  To: Simon Horman
  Cc: davem, edumazet, kuba, pabeni, ast, daniel, hawk, john.fastabend,
	stephen, sinquersw, bpf, netdev, linux-kernel, Jason Xing

On Tue, Mar 14, 2023 at 8:04 PM Simon Horman <simon.horman@corigine.com> wrote:
>
> On Tue, Mar 14, 2023 at 11:05:32AM +0800, Jason Xing 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>
>
> As per my comment on patch 1/2, I'd drop the "/* keep it untouched */"
> comment.

I think you're right. I'll drop this.

Thanks,
Jason

>
> That notwithstanding:
>
> Reviewed-by: Simon Horman <simon.horman@corigine.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     |  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..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 2809b663e78d..25810ee46a04 100644
> > --- a/net/core/net-procfs.c
> > +++ b/net/core/net-procfs.c
> > @@ -179,14 +179,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,
> > +                sd->time_squeeze + sd->budget_squeeze, /* keep it untouched */
> > +                0,
> >                  0, 0, 0, 0, /* was fastroute */
> >                  0,   /* was cpu_collision */
> >                  sd->received_rps, flow_limit_count,
> >                  softnet_backlog_len(sd),     /* keep it untouched */
> >                  (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
> >

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 net-next 1/2] net-sysfs: display two backlog queue len separately
  2023-03-14  3:05 ` [PATCH v2 net-next 1/2] net-sysfs: display two backlog queue len separately Jason Xing
  2023-03-14 11:37   ` Simon Horman
@ 2023-03-14 14:59   ` Eric Dumazet
  2023-03-14 15:44     ` Jason Xing
  1 sibling, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2023-03-14 14:59 UTC (permalink / raw)
  To: Jason Xing
  Cc: davem, kuba, pabeni, ast, daniel, hawk, john.fastabend, stephen,
	simon.horman, sinquersw, bpf, netdev, linux-kernel, Jason Xing

On Mon, Mar 13, 2023 at 8:06 PM 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>
> ---
> 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 | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/net/core/net-procfs.c b/net/core/net-procfs.c
> index 1ec23bf8b05c..2809b663e78d 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);

Reading these variables twice might lead to inconsistency that can
easily be avoided.

I would suggest you cache the values,

u32 len1 = softnet_input_pkt_queue_len(sd);
u32 len2 = softnet_process_queue_len(sd);



>  }
>
>  static struct softnet_data *softnet_get_online(loff_t *pos)
> @@ -169,12 +178,15 @@ 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),     /* keep it untouched */
                    len1 + len2.

> +                  (int)seq->index,
> +                  softnet_input_pkt_queue_len(sd), softnet_process_queue_len(sd));
               len1,  len2);

>         return 0;
>  }
>
> --
> 2.37.3
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 net-next 1/2] net-sysfs: display two backlog queue len separately
  2023-03-14 14:59   ` Eric Dumazet
@ 2023-03-14 15:44     ` Jason Xing
  0 siblings, 0 replies; 8+ messages in thread
From: Jason Xing @ 2023-03-14 15:44 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: davem, kuba, pabeni, ast, daniel, hawk, john.fastabend, stephen,
	simon.horman, sinquersw, bpf, netdev, linux-kernel, Jason Xing

On Tue, Mar 14, 2023 at 10:59 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Mon, Mar 13, 2023 at 8:06 PM 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>
> > ---
> > 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 | 20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/core/net-procfs.c b/net/core/net-procfs.c
> > index 1ec23bf8b05c..2809b663e78d 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);
>
[...]
> Reading these variables twice might lead to inconsistency that can
> easily be avoided.
>
> I would suggest you cache the values,
>
> u32 len1 = softnet_input_pkt_queue_len(sd);
> u32 len2 = softnet_process_queue_len(sd);

Agreed. Thank you, Eric. I should have realized that.

Also, the 2/2 patch which is all about the time_/budget_squeeze should
avoid such inconsistency, I think.

Jason
>
>
>
> >  }
> >
> >  static struct softnet_data *softnet_get_online(loff_t *pos)
> > @@ -169,12 +178,15 @@ 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),     /* keep it untouched */
>                     len1 + len2.
>
> > +                  (int)seq->index,
> > +                  softnet_input_pkt_queue_len(sd), softnet_process_queue_len(sd));
>                len1,  len2);
>
> >         return 0;
> >  }
> >
> > --
> > 2.37.3
> >

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-03-14 15:45 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-14  3:05 [PATCH v2 net-next 0/2] add some detailed data when reading softnet_stat Jason Xing
2023-03-14  3:05 ` [PATCH v2 net-next 1/2] net-sysfs: display two backlog queue len separately Jason Xing
2023-03-14 11:37   ` Simon Horman
2023-03-14 14:59   ` Eric Dumazet
2023-03-14 15:44     ` Jason Xing
2023-03-14  3:05 ` [PATCH v2 net-next 2/2] net: introduce budget_squeeze to help us tune rx behavior Jason Xing
2023-03-14 12:03   ` Simon Horman
2023-03-14 12:31     ` Jason Xing

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).