[PATCH] workqueue: Control the frequency of intensive warning through cmdline

[PATCH] workqueue: Control the frequency of intensive warning through cmdline

[Xuewen Yan]

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(-)

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=
+			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.
+			Control the frequency.
+
+			Default, it will print one warning per 4 times.
+
 	workqueue.power_efficient
 			Per-cpu workqueues are generally preferred because
 			they show better performance thanks to cache
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 7b482a26d741..8e8cccf5329a 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -359,6 +359,10 @@ static const char *wq_affn_names[WQ_AFFN_NR_TYPES] = {
  */
 static unsigned long wq_cpu_intensive_thresh_us = ULONG_MAX;
 module_param_named(cpu_intensive_thresh_us, wq_cpu_intensive_thresh_us, ulong, 0644);
+#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
 
 /* see the comment above the definition of WQ_POWER_EFFICIENT */
 static bool wq_power_efficient = IS_ENABLED(CONFIG_WQ_POWER_EFFICIENT_DEFAULT);
@@ -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))
 			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));
@@ -1231,10 +1235,12 @@ static void wq_cpu_intensive_report(work_func_t func)
 
 	ent = &wci_ents[wci_nr_ents++];
 	ent->func = func;
-	atomic64_set(&ent->cnt, 1);
+	atomic64_set(&ent->cnt, 0);
 	hash_add_rcu(wci_hash, &ent->hash_node, (unsigned long)func);
 
 	raw_spin_unlock(&wci_lock);
+
+	goto restart;
 }
 
 #else	/* CONFIG_WQ_CPU_INTENSIVE_REPORT */
-- 
2.25.1

Re: [PATCH] workqueue: Control the frequency of intensive warning through cmdline

[Randy Dunlap]

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,

> +

-- 
#Randy

Re: [PATCH] workqueue: Control the frequency of intensive warning through cmdline

[Tejun Heo]

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.

-- 
tejun

Re: [PATCH] workqueue: Control the frequency of intensive warning through cmdline

[Xuewen Yan]

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

Re: [PATCH] workqueue: Control the frequency of intensive warning through cmdline

[Tejun Heo]

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.

-- 
tejun

Re: [PATCH] workqueue: Control the frequency of intensive warning through cmdline

[Xuewen Yan]

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

Re: [PATCH] workqueue: Control the frequency of intensive warning through cmdline

[Tejun Heo]

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.

-- 
tejun

Re: [PATCH] workqueue: Control the frequency of intensive warning through cmdline

[Xuewen Yan]

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