Received-SPF: pass (google.com: domain of linux-kernel+bounces-73929-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240219074634.2039-1-xuewen.yan@unisoc.com> <ZdTXWrB8dascOIM_@slm.duckdns.org>
In-Reply-To: <ZdTXWrB8dascOIM_@slm.duckdns.org>
From: Xuewen Yan <xuewen.yan94@gmail.com>
Date: Wed, 21 Feb 2024 10:01:17 +0800
Message-ID: <CAB8ipk9R3hRSTTEEBn8nvOcTZGBtaDQfomXiQwji+DTKgLghwQ@mail.gmail.com>
Subject: Re: [PATCH] workqueue: Control the frequency of intensive warning
 through cmdline
To: Tejun Heo <tj@kernel.org>
Cc: Xuewen Yan <xuewen.yan@unisoc.com>, jiangshanlai@gmail.com, corbet@lwn.net, 
	paulmck@kernel.org, rdunlap@infradead.org, peterz@infradead.org, 
	yanjiewtw@gmail.com, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, 
	ke.wang@unisoc.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Tejun

Thanks for the reply!

On Wed, Feb 21, 2024 at 12:46=E2=80=AFAM 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 =3D 4;
> > +module_param_named(cpu_intensive_warning_per_count, wq_cpu_intensive_w=
arning_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 f=
unc)
> >                * exponentially.
> >                */
> >               cnt =3D atomic64_inc_return_relaxed(&ent->cnt);
> > -             if (cnt >=3D 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 eve=
nts
> can be very high frequency and reporting every nth event can lead to a lo=
t
> of constant warnings. Wouldn't it make sense to keep the exponential back=
off
> 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 =3D atomic64_inc_return_relaxed(&ent->cnt);
-               if (cnt >=3D 4 && is_power_of_2(cnt))
+               if (cnt =3D=3D wq_cpu_intensive_warning_nth ||
+                   (cnt > wq_cpu_intensive_warning_nth && is_power_of_2(cn=
t)))
                        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