lkml.org 
[lkml]   [2022]   [May]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro
    From
    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.

    \
     
     \ /
      Last update: 2022-05-20 13:45    [W:3.041 / U:0.192 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site