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]>
Reviewed-by: Simon Horman <[email protected]>
---
v3: drop the comment suggested by Simon
Link: https://lore.kernel.org/lkml/[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 | 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
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]>
Reviewed-by: Simon Horman <[email protected]>
---
v3:
1) drop the comment suggested by Simon
Link: https://lore.kernel.org/lkml/[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 | 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
On Tue, Mar 14, 2023 at 6:14 AM 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]>
> Reviewed-by: Simon Horman <[email protected]>
> ---
> v3: drop the comment suggested by Simon
> Link: https://lore.kernel.org/lkml/[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 | 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.
On Tue, Mar 14, 2023 at 11:15 PM Eric Dumazet <[email protected]> wrote:
>
> On Tue, Mar 14, 2023 at 6:14 AM 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]>
> > Reviewed-by: Simon Horman <[email protected]>
> > ---
> > v3: drop the comment suggested by Simon
> > Link: https://lore.kernel.org/lkml/[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 | 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.
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.
On Wed, Mar 15, 2023 at 1:09 PM Jakub Kicinski <[email protected]> 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