From: Jason Xing <[email protected]>
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
From: Jason Xing <[email protected]>
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 <[email protected]>
---
v2: keep the total len of backlog queues untouched as Eric said
Link: https://lore.kernel.org/lkml/[email protected]/
---
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
From: Jason Xing <[email protected]>
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 <[email protected]>
---
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/[email protected]/
---
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
On Tue, Mar 14, 2023 at 11:05:31AM +0800, Jason Xing wrote:
> From: Jason Xing <[email protected]>
>
> 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 <[email protected]>
> ---
> v2: keep the total len of backlog queues untouched as Eric said
> Link: https://lore.kernel.org/lkml/[email protected]/
> ---
> 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 <[email protected]>
> + (int)seq->index,
> + softnet_input_pkt_queue_len(sd), softnet_process_queue_len(sd));
> return 0;
> }
>
> --
> 2.37.3
>
On Tue, Mar 14, 2023 at 11:05:32AM +0800, Jason Xing wrote:
> From: Jason Xing <[email protected]>
>
> 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 <[email protected]>
As per my comment on patch 1/2, I'd drop the "/* keep it untouched */"
comment.
That notwithstanding:
Reviewed-by: Simon Horman <[email protected]>
> ---
> 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/[email protected]/
> ---
> 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
>
On Tue, Mar 14, 2023 at 8:04 PM Simon Horman <[email protected]> wrote:
>
> On Tue, Mar 14, 2023 at 11:05:32AM +0800, Jason Xing wrote:
> > From: Jason Xing <[email protected]>
> >
> > 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 <[email protected]>
>
> 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 <[email protected]>
>
> > ---
> > 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/[email protected]/
> > ---
> > 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
> >
On Mon, Mar 13, 2023 at 8:06 PM Jason Xing <[email protected]> wrote:
>
> From: Jason Xing <[email protected]>
>
> 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 <[email protected]>
> ---
> v2: keep the total len of backlog queues untouched as Eric said
> Link: https://lore.kernel.org/lkml/[email protected]/
> ---
> 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
>
On Tue, Mar 14, 2023 at 10:59 PM Eric Dumazet <[email protected]> wrote:
>
> On Mon, Mar 13, 2023 at 8:06 PM Jason Xing <[email protected]> wrote:
> >
> > From: Jason Xing <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > v2: keep the total len of backlog queues untouched as Eric said
> > Link: https://lore.kernel.org/lkml/[email protected]/
> > ---
> > 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
> >