From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
To: Tejun Heo <tj@kernel.org>
Cc: LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro
Date: Fri, 20 May 2022 20:43:41 +0900 [thread overview]
Message-ID: <1a1634ac-db0e-a44c-b286-a3aba55ad695@I-love.SAKURA.ne.jp> (raw)
In-Reply-To: <Yod3S8jmle+LYlES@slm.duckdns.org>
On 2022/05/20 20:11, Tejun Heo wrote:
>>> It kinda bothers me that this causes a build failure. It'd be better if we
>>> can trigger #warning instead. I'm not sure whether there'd be a clean way to
>>> do it tho. Maybe just textual matching would provide similar coverage? How
>>> did you test this?
>>
>> This does not cause a build failure, for this wrapping happens only if
>> flush_workqueue() appears between "#define flush_workqueue(wq)" and
>> "#undef flush_workqueue". Only flush_scheduled_work() in include/linux/workqueue.h
>> calls flush_workqueue(system_wq), and flush_scheduled_work() is defined
>> before the "#define flush_workqueue(wq)" is defined.
>
> What I mean is that if there's a file which didn't get tested or another
> pull request which raced and that thing flushes one of the system_wq's,
> it'll trigger a build error instead of a warning, which is a bit of an
> overkill.
All flush_workqueue(system_*_wq) users are gone in linux-next.git, and this patch
is for preventing new flush_workqueue(system_*_wq) users from coming in.
Therefore, triggering a build error (by sending this patch to linux.git right
before 5.19-rc1 in order to make sure that developers will not use
flush_workqueue(system_*_wq) again) is what this patch is for.
We will also remove flush_scheduled_work() after
all flush_scheduled_work() users are gone.
>
>> And use of #warning directive breaks building with -Werror option.
>
> If the user wants to fail build on warnings, sure. That's different from
> kernel failing to build in a way which may require non-trivial changes to
> fix.
How can #warning directive be utilized inside #define or inline function, for
we can't do like
#define flush_workqueue(wq) \
#if wq == "system_wq" \
#warning Please avoid flushing system_wq. \
#endif \
__flush_workqueue(wq)
or
static inline void flush_workqueue(struct workqueue_struct *wq)
{
#if wq == "system_wq"
#warning Please avoid flushing system_wq.
#endif
__flush_workqueue(wq);
}
. We can use BUiLD_BUG_ON() but I don't think we can use #warning directive.
>
>>> Maybe rename the function to __flush_workqueue() instead of undef'ing the
>>> macro?
>>
>> I prefer not adding __ prefix, for flush_workqueue() is meant as a public function.
>> For easier life of kernel message parsers, I don't feel reason to dare to rename.
>
> You mean the WARN_ON messages? Given how they never trigger, I doubt there's
> much to break. Maybe some kprobe users? But they can survive.
WARN_ON() by passing system-wide workqueues should not happen.
But backtrace of a warning message while inside __flush_workqueue() will be
still possible.
next prev parent reply other threads:[~2022-05-20 11:43 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-24 23:31 [PATCH] checkpatch: warn about flushing system-wide workqueues Tetsuo Handa
2022-04-24 23:45 ` Joe Perches
2022-04-25 0:33 ` Tetsuo Handa
2022-05-05 13:42 ` Tetsuo Handa
2022-05-05 15:48 ` Joe Perches
2022-05-05 17:32 ` Tejun Heo
2022-05-05 23:29 ` Tetsuo Handa
2022-05-12 16:46 ` Tejun Heo
2022-05-16 1:32 ` [PATCH v2] workqueue: Wrap flush_workqueue() using a macro Tetsuo Handa
2022-05-16 5:00 ` [PATCH v3] " Tetsuo Handa
2022-05-16 7:18 ` Joe Perches
2022-05-16 8:34 ` Rasmus Villemoes
2022-05-20 8:01 ` Tejun Heo
2022-05-20 9:51 ` Tetsuo Handa
2022-05-20 11:11 ` Tejun Heo
2022-05-20 11:43 ` Tetsuo Handa [this message]
2022-05-20 17:10 ` Tejun Heo
2022-05-21 1:14 ` Tetsuo Handa
2022-05-21 4:57 ` Tejun Heo
2022-05-21 11:37 ` Tetsuo Handa
2022-05-23 19:04 ` Tejun Heo
2022-05-24 10:51 ` Tetsuo Handa
2022-05-27 6:21 ` [PATCH v4] workqueue: Wrap flush_workqueue() using an inline function Tetsuo Handa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1a1634ac-db0e-a44c-b286-a3aba55ad695@I-love.SAKURA.ne.jp \
--to=penguin-kernel@i-love.sakura.ne.jp \
--cc=linux-kernel@vger.kernel.org \
--cc=tj@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).