Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

[Tejun Heo <tj@kernel.org>]

Hello,

On Fri, May 20, 2022 at 08:43:41PM +0900, Tetsuo Handa wrote:
> 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.

Are we fully sure? Also, there can be other changes in flight which aren't
covered. It's just not nice in general to intentionally trigger build
failures without an easy way to remediate it.

> 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.

What I'm trying to say is that, if we can trigger build warnings, that'd be
a better way to go about it.

> We will also remove flush_scheduled_work() after
> all flush_scheduled_work() users are gone.

Yeah, that'd be great.

> >> 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.

Hmm.... there's __compiletime_warning() which uses gcc's __warning__
attribute, so we can define a function which is tagged with it and call it
on constant_p match. Would that work?

> >>> 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.

I think it'd be fine to rename the function.

Thanks.

-- 
tejun