workqueue: Control the frequency of intensive warning through cmdline

  When CONFIG_WQ_CPU_INTENSIVE_REPORT is set, the kernel will report
the work functions which violate the intensive_threshold_us repeatedly.
And now, only when the violate times exceed 4 and is a power of 2,
the kernel warning could be triggered.

However, sometimes we want to print the warning every time when the work
executed too long. Because sometimes, even if a long work execution time
occurs only once, it may cause other work to be delayed for a long time.

In order to freely control the frequency of printing, a boot argument
is added so that the user can control the warning to be printed
only after a certain number of work timeouts.

Default, it would print warning every 4 times.

Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  9 +++++++++
 kernel/workqueue.c                              | 10 ++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)
  

On 2/18/24 23:46, Xuewen Yan wrote:
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 31b3a25680d0..599fc59fcf70 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -7225,6 +7225,15 @@
>  			threshold repeatedly. They are likely good
>  			candidates for using WQ_UNBOUND workqueues instead.
>  
> +	workqueue.cpu_intensive_warning_per_count=

	workqueue.cpu_intensive_warning_per_count=<uint>
or
	                                          <integer>

> +			If CONFIG_WQ_CPU_INTENSIVE_REPORT is set, the kernel
> +			will report the work functions which violate the
> +			intensive_threshold_us repeatedly. In order to prevent
> +			the kernel log from being printed too frequently.

			                                      frequently,
			control

> +			Control the frequency.
> +
> +			Default, it will print one warning per 4 times.

			By default,

> +
  

Hello,

On Mon, Feb 19, 2024 at 03:46:34PM +0800, Xuewen Yan wrote:
> +#ifdef CONFIG_WQ_CPU_INTENSIVE_REPORT
> +static unsigned int wq_cpu_intensive_warning_per_count = 4;
> +module_param_named(cpu_intensive_warning_per_count, wq_cpu_intensive_warning_per_count, uint, 0644);
> +#endif

wq_cpu_intensive_warning_nth is probably shorter and more idiomatic.

> @@ -1202,7 +1206,7 @@ static void wq_cpu_intensive_report(work_func_t func)
>  		 * exponentially.
>  		 */
>  		cnt = atomic64_inc_return_relaxed(&ent->cnt);
> -		if (cnt >= 4 && is_power_of_2(cnt))
> +		if (wq_cpu_intensive_warning_per_count && !(cnt % wq_cpu_intensive_warning_per_count))

But aren't you mostly interested in the first report? Note that these events
can be very high frequency and reporting every nth event can lead to a lot
of constant warnings. Wouldn't it make sense to keep the exponential backoff
while allowing adjusting the initial threshold?

Thanks.
  

Hi Tejun

Thanks for the reply!

On Wed, Feb 21, 2024 at 12:46 AM Tejun Heo <tj@kernel.org> wrote:
>
> Hello,
>
> On Mon, Feb 19, 2024 at 03:46:34PM +0800, Xuewen Yan wrote:
> > +#ifdef CONFIG_WQ_CPU_INTENSIVE_REPORT
> > +static unsigned int wq_cpu_intensive_warning_per_count = 4;
> > +module_param_named(cpu_intensive_warning_per_count, wq_cpu_intensive_warning_per_count, uint, 0644);
> > +#endif
>
> wq_cpu_intensive_warning_nth is probably shorter and more idiomatic.
>
> > @@ -1202,7 +1206,7 @@ static void wq_cpu_intensive_report(work_func_t func)
> >                * exponentially.
> >                */
> >               cnt = atomic64_inc_return_relaxed(&ent->cnt);
> > -             if (cnt >= 4 && is_power_of_2(cnt))
> > +             if (wq_cpu_intensive_warning_per_count && !(cnt % wq_cpu_intensive_warning_per_count))
>
> But aren't you mostly interested in the first report? Note that these events
> can be very high frequency and reporting every nth event can lead to a lot
> of constant warnings. Wouldn't it make sense to keep the exponential backoff
> while allowing adjusting the initial threshold?

Yes, it makes sense to keep the exponential backoff,
but this may result in not being able to see the warning when the
threshold is exceeded for the first time.

Or what do you think about changing it to the following?

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 7b482a26d741..e783b68ce597 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1202,7 +1202,8 @@ static void wq_cpu_intensive_report(work_func_t func)
                 * exponentially.
                 */
                cnt = atomic64_inc_return_relaxed(&ent->cnt);
-               if (cnt >= 4 && is_power_of_2(cnt))
+               if (cnt == wq_cpu_intensive_warning_nth ||
+                   (cnt > wq_cpu_intensive_warning_nth && is_power_of_2(cnt)))
                        printk_deferred(KERN_WARNING "workqueue: %ps
hogged CPU for >%luus %llu times, consider switching to WQ_UNBOUND\n",
                                        ent->func, wq_cpu_intensive_thresh_us,
                                        atomic64_read(&ent->cnt));

When the cnt reaches the threshold for the first time, a warning can
be printed immediately.
When it exceeds the threshold, keep the exponential backoff.

Thanks!
BR
--
xuewen

>
> Thanks.
>
> --
> tejun
  

Hello,

On Wed, Feb 21, 2024 at 10:01:17AM +0800, Xuewen Yan wrote:
>                 cnt = atomic64_inc_return_relaxed(&ent->cnt);
> -               if (cnt >= 4 && is_power_of_2(cnt))
> +               if (cnt == wq_cpu_intensive_warning_nth ||
> +                   (cnt > wq_cpu_intensive_warning_nth && is_power_of_2(cnt)))

If we do this the nth name doesn't really make sense. Maybe something like
wq_cpu_intensive_warning_thresh is better? Also, something like the
following might be more predictable. Let's say
wq_cpu_intensive_warning_thresh of 0 disables the warnings and it's
initialized to 4 by default.

	if (cnt >= wq_cpu_intensive_warning_thresh &&
	    is_power_of_2(cnt + 1 - wq_cpu_intensive_warning_thresh))

Thanks.
  

Hi Tejun

On Wed, Feb 21, 2024 at 1:44 PM Tejun Heo <tj@kernel.org> wrote:
>
> Hello,
>
> On Wed, Feb 21, 2024 at 10:01:17AM +0800, Xuewen Yan wrote:
> >                 cnt = atomic64_inc_return_relaxed(&ent->cnt);
> > -               if (cnt >= 4 && is_power_of_2(cnt))
> > +               if (cnt == wq_cpu_intensive_warning_nth ||
> > +                   (cnt > wq_cpu_intensive_warning_nth && is_power_of_2(cnt)))
>
> If we do this the nth name doesn't really make sense. Maybe something like
> wq_cpu_intensive_warning_thresh is better? Also, something like the
> following might be more predictable. Let's say
> wq_cpu_intensive_warning_thresh of 0 disables the warnings and it's
> initialized to 4 by default.
>
>         if (cnt >= wq_cpu_intensive_warning_thresh &&
>             is_power_of_2(cnt + 1 - wq_cpu_intensive_warning_thresh))
>

This way looks simpler, but it could not disable the warnings, but I
think this is okay, because even if the threshold is set to 0, the
warning will only be printed when 1, 3, 7, 15....

I will send patch-v2 later as you suggested:)

Thanks.
BR

--
xuewen
  

Hello,

On Wed, Feb 21, 2024 at 07:00:55PM +0800, Xuewen Yan wrote:
> This way looks simpler, but it could not disable the warnings, but I

Yeah, I meant that it'd make sense if the value 0 disables the warning.

Thanks.
  

Hi Tejun

On Thu, Feb 22, 2024 at 1:44 AM Tejun Heo <tj@kernel.org> wrote:
>
> Hello,
>
> On Wed, Feb 21, 2024 at 07:00:55PM +0800, Xuewen Yan wrote:
> > This way looks simpler, but it could not disable the warnings, but I
>
> Yeah, I meant that it'd make sense if the value 0 disables the warning.

Okay, I will add it.

Thanks!

--
xuewen