[linux-next] net: record times of netdev_budget exhausted

  From: Yang Yang <yang.yang29@zte.com>

A long time ago time_squeeze was used to only record netdev_budget
exhausted[1]. Then we added netdev_budget_usecs to enable softirq
tuning[2]. And when polling elapsed netdev_budget_usecs, it's also
record by time_squeeze.
For tuning netdev_budget and netdev_budget_usecs respectively, we'd
better distinguish netdev_budget exhausted from netdev_budget_usecs
elapsed, so add a new recorder to record netdev_budget exhausted.

[1] commit 1da177e4c3f4("Linux-2.6.12-rc2")
[2] commit 7acf8a1e8a28("Replace 2 jiffies with sysctl netdev_budget_usecs to enable softirq tuning")

Signed-off-by: Yang Yang <yang.yang29@zte.com>
---
 include/linux/netdevice.h |  1 +
 net/core/dev.c            | 11 +++++++----
 net/core/net-procfs.c     |  5 +++--
 3 files changed, 11 insertions(+), 6 deletions(-)
  

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20221202]

url:    https://github.com/intel-lab-lkp/linux/commits/yang-yang29-zte-com-cn/net-record-times-of-netdev_budget-exhausted/20221203-161326
patch link:    https://lore.kernel.org/r/202212031612057505056%40zte.com.cn
patch subject: [PATCH linux-next] net: record times of netdev_budget exhausted
config: m68k-allyesconfig
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/94f9d928cabc8715256c20e909b9e730620f4ed0
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review yang-yang29-zte-com-cn/net-record-times-of-netdev_budget-exhausted/20221203-161326
        git checkout 94f9d928cabc8715256c20e909b9e730620f4ed0
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash net/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   net/core/net-procfs.c: In function 'softnet_seq_show':
   net/core/net-procfs.c:177:60: error: expected ')' before 'sd'
     177 |                    softnet_backlog_len(sd), (int)seq->index
         |                                                            ^
         |                                                            )
     178 |                    sd->budget_exhaust);
         |                    ~~                                       
   net/core/net-procfs.c:171:19: note: to match this '('
     171 |         seq_printf(seq,
         |                   ^
>> net/core/net-procfs.c:172:89: warning: format '%x' expects a matching 'unsigned int' argument [-Wformat=]
     172 |                    "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x\n",
         |                                                                                      ~~~^
         |                                                                                         |
         |                                                                                         unsigned int


vim +172 net/core/net-procfs.c

   166	
   167		/* the index is the CPU id owing this sd. Since offline CPUs are not
   168		 * displayed, it would be othrwise not trivial for the user-space
   169		 * mapping the data a specific CPU
   170		 */
   171		seq_printf(seq,
 > 172			   "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x\n",
   173			   sd->processed, sd->dropped, sd->time_squeeze, 0,
   174			   0, 0, 0, 0, /* was fastroute */
   175			   0,	/* was cpu_collision */
   176			   sd->received_rps, flow_limit_count,
   177			   softnet_backlog_len(sd), (int)seq->index
   178			   sd->budget_exhaust);
   179		return 0;
   180	}
   181
  

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on next-20221202]

url:    https://github.com/intel-lab-lkp/linux/commits/yang-yang29-zte-com-cn/net-record-times-of-netdev_budget-exhausted/20221203-161326
patch link:    https://lore.kernel.org/r/202212031612057505056%40zte.com.cn
patch subject: [PATCH linux-next] net: record times of netdev_budget exhausted
config: x86_64-randconfig-a012
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/94f9d928cabc8715256c20e909b9e730620f4ed0
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review yang-yang29-zte-com-cn/net-record-times-of-netdev_budget-exhausted/20221203-161326
        git checkout 94f9d928cabc8715256c20e909b9e730620f4ed0
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> net/core/net-procfs.c:178:6: error: expected ')'
                      sd->budget_exhaust);
                      ^
   net/core/net-procfs.c:171:12: note: to match this '('
           seq_printf(seq,
                     ^
   1 error generated.


vim +178 net/core/net-procfs.c

   166	
   167		/* the index is the CPU id owing this sd. Since offline CPUs are not
   168		 * displayed, it would be othrwise not trivial for the user-space
   169		 * mapping the data a specific CPU
   170		 */
   171		seq_printf(seq,
   172			   "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x\n",
   173			   sd->processed, sd->dropped, sd->time_squeeze, 0,
   174			   0, 0, 0, 0, /* was fastroute */
   175			   0,	/* was cpu_collision */
   176			   sd->received_rps, flow_limit_count,
   177			   softnet_backlog_len(sd), (int)seq->index
 > 178			   sd->budget_exhaust);
   179		return 0;
   180	}
   181
  

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on next-20221202]

url:    https://github.com/intel-lab-lkp/linux/commits/yang-yang29-zte-com-cn/net-record-times-of-netdev_budget-exhausted/20221203-161326
patch link:    https://lore.kernel.org/r/202212031612057505056%40zte.com.cn
patch subject: [PATCH linux-next] net: record times of netdev_budget exhausted
config: x86_64-randconfig-a013
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/94f9d928cabc8715256c20e909b9e730620f4ed0
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review yang-yang29-zte-com-cn/net-record-times-of-netdev_budget-exhausted/20221203-161326
        git checkout 94f9d928cabc8715256c20e909b9e730620f4ed0
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   net/core/net-procfs.c: In function 'softnet_seq_show':
>> net/core/net-procfs.c:177:60: error: expected ')' before 'sd'
     177 |                    softnet_backlog_len(sd), (int)seq->index
         |                                                            ^
         |                                                            )
     178 |                    sd->budget_exhaust);
         |                    ~~                                       
   net/core/net-procfs.c:171:19: note: to match this '('
     171 |         seq_printf(seq,
         |                   ^
   net/core/net-procfs.c:172:89: warning: format '%x' expects a matching 'unsigned int' argument [-Wformat=]
     172 |                    "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x\n",
         |                                                                                      ~~~^
         |                                                                                         |
         |                                                                                         unsigned int


vim +177 net/core/net-procfs.c

   166	
   167		/* the index is the CPU id owing this sd. Since offline CPUs are not
   168		 * displayed, it would be othrwise not trivial for the user-space
   169		 * mapping the data a specific CPU
   170		 */
   171		seq_printf(seq,
   172			   "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x\n",
   173			   sd->processed, sd->dropped, sd->time_squeeze, 0,
   174			   0, 0, 0, 0, /* was fastroute */
   175			   0,	/* was cpu_collision */
   176			   sd->received_rps, flow_limit_count,
 > 177			   softnet_backlog_len(sd), (int)seq->index
   178			   sd->budget_exhaust);
   179		return 0;
   180	}
   181
  

On Sat, 3 Dec 2022 16:12:05 +0800 (CST) yang.yang29@zte.com.cn wrote:
> A long time ago time_squeeze was used to only record netdev_budget
> exhausted[1]. Then we added netdev_budget_usecs to enable softirq
> tuning[2]. And when polling elapsed netdev_budget_usecs, it's also
> record by time_squeeze.
> For tuning netdev_budget and netdev_budget_usecs respectively, we'd
> better distinguish netdev_budget exhausted from netdev_budget_usecs
> elapsed, so add a new recorder to record netdev_budget exhausted.

You're tuning netdev_budget and netdev_budget_usecs ?
You need to say more because I haven't seen anyone do that before.

time_squeeze is extremely noisy and annoyingly useless,
we need to understand exactly what you're doing before
we accept any changes to this core piece of code.
  

On Tue, 6 Dec 2022 09:53:05 +0800 (CST) kuba@kernel.org wrote:
> time_squeeze is extremely noisy and annoyingly useless,
> we need to understand exactly what you're doing before
> we accept any changes to this core piece of code.

The author of "Replace 2 jiffies with sysctl netdev_budget_usecs
to enable softirq tuning" is Matthew Whitehead, he said this in
git log: Constants used for tuning are generally a bad idea, especially
as hardware changes over time...For example, a very fast machine
might tune this to 1000 microseconds, while my regression testing
486DX-25 needs it to be 4000 microseconds on a nearly idle network
to prevent time_squeeze from being incremented.

And on my systems there are huge packets on the intranet, and we
came accross with lots of time_squeeze. The idea is that, netdev_budget*
are selections between throughput and real-time. If we care throughput
and not care real-time so much, we may want bigger netdev_budget*.

In this scenario, we want to tune netdev_budget* and see their effect
separately.

By the way, if netdev_budget* are useless, should they be deleted?

Thanks.
  

On Tue, 6 Dec 2022 10:35:07 +0800 (CST) yang.yang29@zte.com.cn wrote:
> The author of "Replace 2 jiffies with sysctl netdev_budget_usecs
> to enable softirq tuning" is Matthew Whitehead, he said this in
> git log: Constants used for tuning are generally a bad idea, especially
> as hardware changes over time...For example, a very fast machine
> might tune this to 1000 microseconds, while my regression testing
> 486DX-25 needs it to be 4000 microseconds on a nearly idle network
> to prevent time_squeeze from being incremented.

Let's just ignore that on the basis that it mentions prehistoric HW ;)

> And on my systems there are huge packets on the intranet, and we
> came accross with lots of time_squeeze. The idea is that, netdev_budget*
> are selections between throughput and real-time. If we care throughput
> and not care real-time so much, we may want bigger netdev_budget*.

But are you seeing actual performance wins in terms of throughput 
or latency? 

As I said time_squeeze is very noisy. In my experience it's very
sensitive to issues with jiffies, like someone masking interrupts on
the timekeeper CPU for a long time (which if you use cgroups happens
_a lot_ :/).

Have you tried threaded NAPI? (find files called 'threaded' in sysfs)
It will let you do any such tuning much more flexibly.

> In this scenario, we want to tune netdev_budget* and see their effect
> separately.
>
> By the way, if netdev_budget* are useless, should they be deleted?

Well, we can't be sure if there's really nobody that uses them :(
It's very risky to remove stuff that's exposed to user space.
  

On Tue, 6 Dec 2022 10:47:07 +0800 (CST) kuba@kernel.org wrote:
> But are you seeing actual performance wins in terms of throughput
> or latency? 

I did a test and see 7~8% of performance difference with small and big
netdev_budget. Detail:
1. machine
In qemu. CPU is QEMU TCG CPU version 2.5+.
2. kernel
Linux (none) 5.14.0-rc6+ #91 SMP Tue Dec 6 19:55:14 CST 2022 x86_64 GNU/Linux
3. test condition
Run 5 rt tasks to simulate workload, task is test.sh:
---
#!/bin/bash

while [ 1 ]
do
      ls  > /dev/null
done
---
4. test method
Use ping -f to flood.
# ping -f 192.168.1.201 -w 1800 

With netdev_buget is 500, and netdev_budget_usecs is 2000: 
497913 packets transmitted, 497779 received, 0% packet loss, time 1799992ms
rtt min/avg/max/mdev = 0.181/114.417/1915.343/246.098 ms, pipe 144, ipg/ewma 3.615/0.273 ms

With netdev_budget is 1, and netdev_budget_usecs is 2000: 
457530 packets transmitted, 457528 received, 0% packet loss, time 1799997ms
rtt min/avg/max/mdev = 0.180/123.287/1914.186/253.883 ms, pipe 147, ipg/ewma 3.934/0.301 ms

With small netdev_budget, avg latency increases 7%, packets transmitted
decreases 8%.

> Have you tried threaded NAPI? (find files called 'threaded' in sysfs)

Thanks, we had researched on threaded NAPI, much applaud for it!
But we think someone maynot use it because some kinds of reasons.
One is threaded NAPI is good for control, but maynot good for
throughput, especially for those who not care real-time too much.
Another reason is distribution kernel may too old to support
threaded NAPI?

>Well, we can't be sure if there's really nobody that uses them :(

As we still retain netdev_budget*, and there maybe some using it,
should it be improve? Netdev_budget* are sysctl for administrator,
when administrator adjust them, they may want to see the effect in
a direct or easy way. That's what this patch's purpose.
  

On Wed, Dec 7, 2022 at 8:28 AM <yang.yang29@zte.com.cn> wrote:
>
> On Tue, 6 Dec 2022 10:47:07 +0800 (CST) kuba@kernel.org wrote:
> > But are you seeing actual performance wins in terms of throughput
> > or latency?
>
> I did a test and see 7~8% of performance difference with small and big
> netdev_budget. Detail:
> 1. machine
> In qemu. CPU is QEMU TCG CPU version 2.5+.
> 2. kernel
> Linux (none) 5.14.0-rc6+ #91 SMP Tue Dec 6 19:55:14 CST 2022 x86_64 GNU/Linux
> 3. test condition
> Run 5 rt tasks to simulate workload, task is test.sh:
> ---
> #!/bin/bash
>
> while [ 1 ]
> do
>       ls  > /dev/null
> done
> ---
> 4. test method
> Use ping -f to flood.
> # ping -f 192.168.1.201 -w 1800
>
> With netdev_buget is 500, and netdev_budget_usecs is 2000:
> 497913 packets transmitted, 497779 received, 0% packet loss, time 1799992ms
> rtt min/avg/max/mdev = 0.181/114.417/1915.343/246.098 ms, pipe 144, ipg/ewma 3.615/0.273 ms
>
> With netdev_budget is 1, and netdev_budget_usecs is 2000:
> 457530 packets transmitted, 457528 received, 0% packet loss, time 1799997ms
> rtt min/avg/max/mdev = 0.180/123.287/1914.186/253.883 ms, pipe 147, ipg/ewma 3.934/0.301 ms
>

Sure, but netdev_budget set to 1 is extreme, don't you think ???

Has anyone used such a setting ?

> With small netdev_budget, avg latency increases 7%, packets transmitted
> decreases 8%.
>
> > Have you tried threaded NAPI? (find files called 'threaded' in sysfs)
>
> Thanks, we had researched on threaded NAPI, much applaud for it!
> But we think someone maynot use it because some kinds of reasons.
> One is threaded NAPI is good for control, but maynot good for
> throughput, especially for those who not care real-time too much.
> Another reason is distribution kernel may too old to support
> threaded NAPI?
>
> >Well, we can't be sure if there's really nobody that uses them :(
>
> As we still retain netdev_budget*, and there maybe some using it,
> should it be improve? Netdev_budget* are sysctl for administrator,
> when administrator adjust them, they may want to see the effect in
> a direct or easy way. That's what this patch's purpose.

We prefer not changing /proc file format as much as we can, they are
deprecated/legacy.

Presumably, modern tracing techniques can let you do what you want
without adding new counters.

I think that a per-cpu counter is old-fashioned, and incurs a cost for
the vast majority of users who will
never look at the counters.
  

> Sure, but netdev_budget set to 1 is extreme, don't you think ???

Yes of course, that is just a test to show the difference.

> We prefer not changing /proc file format as much as we can, they are
> deprecated/legacy.

Should we add some explain of the deprecation in code or doc?
As it's deprecated, I think it's NAK for this patch.

> Presumably, modern tracing techniques can let you do what you want
> without adding new counters.

Totally agree.
  

> Presumably, modern tracing techniques can let you do what you want
> without adding new counters.

By the way, should we add a tracepoint likes trace_napi_poll() to make
it easier? Something likes:
        if (unlikely(budget <= 0 ||
                 time_after_eq(jiffies, time_limit))) {
            sd->time_squeeze++;
+            trace_napi_poll(budget, jiffies, time_limit);
            break;
        }
  

On Wed, 7 Dec 2022 16:17:32 +0800 (CST) yang.yang29@zte.com.cn wrote:
> > We prefer not changing /proc file format as much as we can, they are
> > deprecated/legacy.  
> 
> Should we add some explain of the deprecation in code or doc?
> As it's deprecated, I think it's NAK for this patch.

Correct, it is a NAK.
  

On Wed, 7 Dec 2022 20:30:08 +0800 (CST) yang.yang29@zte.com.cn wrote:
> > Presumably, modern tracing techniques can let you do what you want
> > without adding new counters.  
> 
> By the way, should we add a tracepoint likes trace_napi_poll() to make
> it easier? Something likes:
>         if (unlikely(budget <= 0 ||
>                  time_after_eq(jiffies, time_limit))) {
>             sd->time_squeeze++;
> +            trace_napi_poll(budget, jiffies, time_limit);
>             break;
>         }

In my experience - no this is not useful.

Sorry if this is too direct, but it seems to me like you're trying hard
to find something useful to do in this area without a clear use case. 
We have coding tasks which would definitely be useful and which nobody
has time to accomplish. Please ask if you're trying to find something
to do.
  

> In my experience - no this is not useful.

Received, thanks!

> Sorry if this is too direct, but it seems to me like you're trying hard
> to find something useful to do in this area without a clear use case. 
I see maybe this is a too special scenes, not suitable. The motivation
is we see lots of time_squeeze on our working machines, and want to
tuning, but our kernel are not ready to use threaded NAPI. And we
did see performance difference on different netdev_budget* in
preliminary tests.

> We have coding tasks which would definitely be useful and which nobody
> has time to accomplish. Please ask if you're trying to find something
> to do.

We focus on 5G telecom machine, which has huge TIPC packets in the
intranet. If it's related, we are glad to do it with much appreciate of your
indicate!

Thanks.
  

On Thu, 8 Dec 2022 09:12:06 +0800 (CST) yang.yang29@zte.com.cn wrote:
> > Sorry if this is too direct, but it seems to me like you're trying hard
> > to find something useful to do in this area without a clear use case.
>   
> I see maybe this is a too special scenes, not suitable. The motivation
> is we see lots of time_squeeze on our working machines, and want to
> tuning, but our kernel are not ready to use threaded NAPI. And we

Ah, in that cases I indeed misjudged, sorry.

> did see performance difference on different netdev_budget* in
> preliminary tests.

Right, the budget values < 100 are quite impractical. Also as I said
time_squeeze is a terrible metric, if you can find a direct metric
in terms of application latency or max PPS, that's much more valuable.

> > We have coding tasks which would definitely be useful and which nobody
> > has time to accomplish. Please ask if you're trying to find something
> > to do.  
> 
> We focus on 5G telecom machine, which has huge TIPC packets in the
> intranet. If it's related, we are glad to do it with much appreciate of your
> indicate!

Oh, unfortunately most of the tasks we have are around driver
infrastructure.
  

> ime_squeeze is a terrible metric, if you can find a direct metric
> in terms of application latency or max PPS, that's much more valuable.

Actually we are working on measure the latency of packets between
inqueue and dequeue in eBPF way, we add new tracepoint to help
do that. And we are also consider using PSI to measure it. 

We will submit patches when it's ready, thanks!