work item still be scheduled to execute after destroy_workqueue?

work item still be scheduled to execute after destroy_workqueue?

[richard clark]

Hi Lai and Tejun,

Why can the work still be queued to and executed when queueing it to a
wq has been destroyed, for instance, the below code snippet in a
kernel module:
---------------------->8---------------------

struct workqueue_struct *wq0;
#define MAX_ACTIVE_WORKS        (3)

static void work0_func(struct work_struct *work);
static void work1_func(struct work_struct *work);
static void work2_func(struct work_struct *work);

static DECLARE_WORK(w0, work0_func);
static DECLARE_WORK(w1, work1_func);
static DECLARE_WORK(w2, work2_func);

/* work->func */
static void work0_func(struct work_struct *work)
{
        pr_info("+%s begins to sleep\n", __func__);
        /* sleep for 10s */
        schedule_timeout_interruptible(msecs_to_jiffies(10000));
        pr_info("+%s after sleep, begin to queue another work\n", __func__);
        queue_work_on(1, wq0, &w1);
}

/* work->func */
static void work1_func(struct work_struct *work)
{
        pr_info("+%s scheduled\n", __func__);
}

/* work->func */
static void work2_func(struct work_struct *work)
{
        pr_info("+%s scheduled\n", __func__);
}

static int destroy_init(void)
{
        wq0 = alloc_workqueue("percpu_wq0", 0, MAX_ACTIVE_WORKS);
        if (!wq0) {
                pr_err("alloc_workqueue failed\n");
                return -1;
        }
        queue_work_on(1, wq0, &w0);
        pr_info("Begin to destroy wq0...\n");
        destroy_workqueue(wq0);
        pr_info("queue w2 to the wq0 after destroyed...\n");
        queue_work_on(1, wq0, &w2);

        return 0;
}

The output on my x86_64 box is:

[344702.734480] +destroy_init+
[344702.734499] Begin to destroy wq0...
[344702.734516] +work0_func begins to sleep
[344712.791607] +work0_func after sleep, begin to queue another work
[344712.791620] +work1_func scheduled
[344712.791649] queue w2 to the wq0 after destroyed...
[344712.791663] +work2_func scheduled  <------------- work 2 still be scheduled?

Re: work item still be scheduled to execute after destroy_workqueue?

[Lai Jiangshan]

On Mon, Dec 5, 2022 at 2:18 PM richard clark
<richard.xnu.clark@gmail.com> wrote:
>
> Hi Lai and Tejun,
>
> Why can the work still be queued to and executed when queueing it to a
> wq has been destroyed, for instance, the below code snippet in a
> kernel module:
> ---------------------->8---------------------
>
> struct workqueue_struct *wq0;
> #define MAX_ACTIVE_WORKS        (3)
>
> static void work0_func(struct work_struct *work);
> static void work1_func(struct work_struct *work);
> static void work2_func(struct work_struct *work);
>
> static DECLARE_WORK(w0, work0_func);
> static DECLARE_WORK(w1, work1_func);
> static DECLARE_WORK(w2, work2_func);
>
> /* work->func */
> static void work0_func(struct work_struct *work)
> {
>         pr_info("+%s begins to sleep\n", __func__);
>         /* sleep for 10s */
>         schedule_timeout_interruptible(msecs_to_jiffies(10000));
>         pr_info("+%s after sleep, begin to queue another work\n", __func__);
>         queue_work_on(1, wq0, &w1);
> }
>
> /* work->func */
> static void work1_func(struct work_struct *work)
> {
>         pr_info("+%s scheduled\n", __func__);
> }
>
> /* work->func */
> static void work2_func(struct work_struct *work)
> {
>         pr_info("+%s scheduled\n", __func__);
> }
>
> static int destroy_init(void)
> {
>         wq0 = alloc_workqueue("percpu_wq0", 0, MAX_ACTIVE_WORKS);
>         if (!wq0) {
>                 pr_err("alloc_workqueue failed\n");
>                 return -1;
>         }
>         queue_work_on(1, wq0, &w0);
>         pr_info("Begin to destroy wq0...\n");
>         destroy_workqueue(wq0);
>         pr_info("queue w2 to the wq0 after destroyed...\n");
>         queue_work_on(1, wq0, &w2);

Hello, Richard.

Nice spot.

It is illegal to use a destroyed structure in the view of any API.

A destroyed workqueue might be directly freed or kept for a while,
which is up to the code of workqueue.c

Before e2dca7adff8f(workqueue: make the workqueues list RCU walkable),
the workqueue is directly totally freed when destroyed.
After the said commit, the workqueue is held for an RCU grace before
totally freed.  And it is a per-cpu workqueue, and the base ref is
never dropped on per-cpu pwqs, which means it is referencable and
able to be queued items during the period by accident.

Albeit it is illegal to use a destroyed workqueue, it is definitely bad
for workqueue code not to complain noisily about the behavior, so I am
going to set __WQ_DRAINING permanently for the destroyed workqueue, so
the illegal usage of the destroyed workqueue can result WARN().

Thank you for the report.
Lai

>
>         return 0;
> }
>
> The output on my x86_64 box is:
>
> [344702.734480] +destroy_init+
> [344702.734499] Begin to destroy wq0...
> [344702.734516] +work0_func begins to sleep
> [344712.791607] +work0_func after sleep, begin to queue another work
> [344712.791620] +work1_func scheduled
> [344712.791649] queue w2 to the wq0 after destroyed...
> [344712.791663] +work2_func scheduled  <------------- work 2 still be scheduled?

Re: work item still be scheduled to execute after destroy_workqueue?

[richard clark]

Hi Lai,

On Tue, Dec 6, 2022 at 12:13 PM Lai Jiangshan <jiangshanlai@gmail.com> wrote:
>
> On Mon, Dec 5, 2022 at 2:18 PM richard clark
> <richard.xnu.clark@gmail.com> wrote:
> >
> > Hi Lai and Tejun,
> >
> > Why can the work still be queued to and executed when queueing it to a
> > wq has been destroyed, for instance, the below code snippet in a
> > kernel module:
> > ---------------------->8---------------------
> >
> > struct workqueue_struct *wq0;
> > #define MAX_ACTIVE_WORKS        (3)
> >
> > static void work0_func(struct work_struct *work);
> > static void work1_func(struct work_struct *work);
> > static void work2_func(struct work_struct *work);
> >
> > static DECLARE_WORK(w0, work0_func);
> > static DECLARE_WORK(w1, work1_func);
> > static DECLARE_WORK(w2, work2_func);
> >
> > /* work->func */
> > static void work0_func(struct work_struct *work)
> > {
> >         pr_info("+%s begins to sleep\n", __func__);
> >         /* sleep for 10s */
> >         schedule_timeout_interruptible(msecs_to_jiffies(10000));
> >         pr_info("+%s after sleep, begin to queue another work\n", __func__);
> >         queue_work_on(1, wq0, &w1);
> > }
> >
> > /* work->func */
> > static void work1_func(struct work_struct *work)
> > {
> >         pr_info("+%s scheduled\n", __func__);
> > }
> >
> > /* work->func */
> > static void work2_func(struct work_struct *work)
> > {
> >         pr_info("+%s scheduled\n", __func__);
> > }
> >
> > static int destroy_init(void)
> > {
> >         wq0 = alloc_workqueue("percpu_wq0", 0, MAX_ACTIVE_WORKS);
> >         if (!wq0) {
> >                 pr_err("alloc_workqueue failed\n");
> >                 return -1;
> >         }
> >         queue_work_on(1, wq0, &w0);
> >         pr_info("Begin to destroy wq0...\n");
> >         destroy_workqueue(wq0);
> >         pr_info("queue w2 to the wq0 after destroyed...\n");
> >         queue_work_on(1, wq0, &w2);
>
> Hello, Richard.
>
> Nice spot.
>
> It is illegal to use a destroyed structure in the view of any API.
>
> A destroyed workqueue might be directly freed or kept for a while,
> which is up to the code of workqueue.c
>
> Before e2dca7adff8f(workqueue: make the workqueues list RCU walkable),
> the workqueue is directly totally freed when destroyed.
> After the said commit, the workqueue is held for an RCU grace before
> totally freed.  And it is a per-cpu workqueue, and the base ref is
> never dropped on per-cpu pwqs, which means it is referencable and
> able to be queued items during the period by accident.
>
> Albeit it is illegal to use a destroyed workqueue, it is definitely bad
> for workqueue code not to complain noisily about the behavior, so I am
> going to set __WQ_DRAINING permanently for the destroyed workqueue, so
> the illegal usage of the destroyed workqueue can result WARN().
>
A WARN is definitely reasonable and has its benefits. Can I try to
submit the patch and you're nice to review as maintainer?

Thanks,
Richard
>
> Thank you for the report.
> Lai
>
> >
> >         return 0;
> > }
> >
> > The output on my x86_64 box is:
> >
> > [344702.734480] +destroy_init+
> > [344702.734499] Begin to destroy wq0...
> > [344702.734516] +work0_func begins to sleep
> > [344712.791607] +work0_func after sleep, begin to queue another work
> > [344712.791620] +work1_func scheduled
> > [344712.791649] queue w2 to the wq0 after destroyed...
> > [344712.791663] +work2_func scheduled  <------------- work 2 still be scheduled?

Re: work item still be scheduled to execute after destroy_workqueue?

[Lai Jiangshan]

On Tue, Dec 6, 2022 at 12:35 PM richard clark
<richard.xnu.clark@gmail.com> wrote:

> >
> A WARN is definitely reasonable and has its benefits. Can I try to
> submit the patch and you're nice to review as maintainer?
>
> Thanks,
> Richard
> >

Sure, go ahead.

What's in my mind is that the following code is wrapped in a new function:

        mutex_lock(&wq->mutex);
        if (!wq->nr_drainers++)
                wq->flags |= __WQ_DRAINING;
        mutex_unlock(&wq->mutex);


and the new function replaces the open code drain_workqueue() and
is also called in destroy_workqueue() (before calling drain_workqueue()).


__WQ_DRAINING will cause the needed WARN on illegally queuing items on
destroyed workqueue.

Thanks
Lai

Re: work item still be scheduled to execute after destroy_workqueue?

[richard clark]

On Tue, Dec 6, 2022 at 2:23 PM Lai Jiangshan <jiangshanlai@gmail.com> wrote:
>
> On Tue, Dec 6, 2022 at 12:35 PM richard clark
> <richard.xnu.clark@gmail.com> wrote:
>
> > >
> > A WARN is definitely reasonable and has its benefits. Can I try to
> > submit the patch and you're nice to review as maintainer?
> >
> > Thanks,
> > Richard
> > >
>
> Sure, go ahead.
>
> What's in my mind is that the following code is wrapped in a new function:
>
>         mutex_lock(&wq->mutex);
>         if (!wq->nr_drainers++)
>                 wq->flags |= __WQ_DRAINING;
>         mutex_unlock(&wq->mutex);
>
>
> and the new function replaces the open code drain_workqueue() and
> is also called in destroy_workqueue() (before calling drain_workqueue()).
>
Except that, do we need to defer the __WQ_DRAINING clean to the
rcu_call, thus we still have a close-loop of the drainer's count, like
this?

--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c

@@ -3528,6 +3526,9 @@ static void rcu_free_wq(struct rcu_head *rcu)

        else
                free_workqueue_attrs(wq->unbound_attrs);

+       if (!--wq->nr_drainers)
+               wq->flags &= ~__WQ_DRAINING;
+
        kfree(wq);

>
> __WQ_DRAINING will cause the needed WARN on illegally queuing items on
> destroyed workqueue.

I will re-test it if there are no concerns about the above fix...

>
> Thanks
> Lai

Re: work item still be scheduled to execute after destroy_workqueue?

[Lai Jiangshan]

On Tue, Dec 6, 2022 at 5:20 PM richard clark
<richard.xnu.clark@gmail.com> wrote:
>
> On Tue, Dec 6, 2022 at 2:23 PM Lai Jiangshan <jiangshanlai@gmail.com> wrote:
> >
> > On Tue, Dec 6, 2022 at 12:35 PM richard clark
> > <richard.xnu.clark@gmail.com> wrote:
> >
> > > >
> > > A WARN is definitely reasonable and has its benefits. Can I try to
> > > submit the patch and you're nice to review as maintainer?
> > >
> > > Thanks,
> > > Richard
> > > >
> >
> > Sure, go ahead.
> >
> > What's in my mind is that the following code is wrapped in a new function:
> >
> >         mutex_lock(&wq->mutex);
> >         if (!wq->nr_drainers++)
> >                 wq->flags |= __WQ_DRAINING;
> >         mutex_unlock(&wq->mutex);
> >
> >
> > and the new function replaces the open code drain_workqueue() and
> > is also called in destroy_workqueue() (before calling drain_workqueue()).
> >
> Except that, do we need to defer the __WQ_DRAINING clean to the
> rcu_call, thus we still have a close-loop of the drainer's count, like
> this?

No, I don't think we need it. The wq is totally freed in rcu_free_wq.

Or we can just introduce __WQ_DESTROYING.

It seems using __WQ_DESTROYING is better.

>
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
>
> @@ -3528,6 +3526,9 @@ static void rcu_free_wq(struct rcu_head *rcu)
>
>         else
>                 free_workqueue_attrs(wq->unbound_attrs);
>
> +       if (!--wq->nr_drainers)
> +               wq->flags &= ~__WQ_DRAINING;
> +
>         kfree(wq);
>
> >
> > __WQ_DRAINING will cause the needed WARN on illegally queuing items on
> > destroyed workqueue.
>
> I will re-test it if there are no concerns about the above fix...
>
> >
> > Thanks
> > Lai

Re: work item still be scheduled to execute after destroy_workqueue?

[richard clark]

On Wed, Dec 7, 2022 at 10:38 AM Lai Jiangshan <jiangshanlai@gmail.com> wrote:
>
> On Tue, Dec 6, 2022 at 5:20 PM richard clark
> <richard.xnu.clark@gmail.com> wrote:
> >
> > On Tue, Dec 6, 2022 at 2:23 PM Lai Jiangshan <jiangshanlai@gmail.com> wrote:
> > >
> > > On Tue, Dec 6, 2022 at 12:35 PM richard clark
> > > <richard.xnu.clark@gmail.com> wrote:
> > >
> > > > >
> > > > A WARN is definitely reasonable and has its benefits. Can I try to
> > > > submit the patch and you're nice to review as maintainer?
> > > >
> > > > Thanks,
> > > > Richard
> > > > >
> > >
> > > Sure, go ahead.
> > >
> > > What's in my mind is that the following code is wrapped in a new function:
> > >
> > >         mutex_lock(&wq->mutex);
> > >         if (!wq->nr_drainers++)
> > >                 wq->flags |= __WQ_DRAINING;
> > >         mutex_unlock(&wq->mutex);
> > >
> > >
> > > and the new function replaces the open code drain_workqueue() and
> > > is also called in destroy_workqueue() (before calling drain_workqueue()).
> > >
> > Except that, do we need to defer the __WQ_DRAINING clean to the
> > rcu_call, thus we still have a close-loop of the drainer's count, like
> > this?
>
> No, I don't think we need it. The wq is totally freed in rcu_free_wq.
>
> Or we can just introduce __WQ_DESTROYING.
>
> It seems using __WQ_DESTROYING is better.

The wq->flags will be unreliable after kfree(wq), for example, in my
machine, the wq->flags can be 0x7ec1e1a3, 0x37cff1a3 or 0x7fa23da3 ...
after wq be kfreed, consequently the result of queueing a new work
item to a kfreed wq is undetermined, sometimes it's ok because the
queue_work will return directly(e.g, the wq->flags = 0x7ec1e1a3, a
fake __WQ_DRAINING state), sometimes it will trigger a kernel NULL
pointer dereference BUG when the wq->flags = 0x7fa23da3(fake
!__WQ_DRAINING state).

IMO, given the above condition,  we can handle this in 2 phases:
before the rcu_call and after.
a. before rcu_call. Using __WQ_DESTROYING to allow the chained work
queued in or not in destroy_workqueue(...) level, __WQ_DRAINING is
used to make the drain_workqueue(...) still can be standalone. The
code snippet like this:
destroy_workqueue(...)
{
        mutex_lock(&wq->mutex);
        wq->flags |= __WQ_DESTROYING;
        mutex_lock(&wq->mutex);
        ...
}

__queue_work(...)
{
          if (unlikely((wq->flags & __WQ_DESTROYING) || (wq->flags &
__WQ_DRAINING)) &&
                   WARN_ON_ONCE(!is_chained_work(wq)))
         return;
}

b. after rcu_call. What in my mind is:
rcu_free_wq(struct rcu_head *rcu)
{
          ...
          kfree(wq);
          wq = NULL;
}

__queue_work(...)
{
        if (!wq)
                return;
        ...
}

Any comments?

>
> >
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> >
> > @@ -3528,6 +3526,9 @@ static void rcu_free_wq(struct rcu_head *rcu)
> >
> >         else
> >                 free_workqueue_attrs(wq->unbound_attrs);
> >
> > +       if (!--wq->nr_drainers)
> > +               wq->flags &= ~__WQ_DRAINING;
> > +
> >         kfree(wq);
> >
> > >
> > > __WQ_DRAINING will cause the needed WARN on illegally queuing items on
> > > destroyed workqueue.
> >
> > I will re-test it if there are no concerns about the above fix...
> >
> > >
> > > Thanks
> > > Lai

Re: work item still be scheduled to execute after destroy_workqueue?

[Lai Jiangshan]

On Thu, Dec 8, 2022 at 10:44 AM richard clark
<richard.xnu.clark@gmail.com> wrote:
>
> On Wed, Dec 7, 2022 at 10:38 AM Lai Jiangshan <jiangshanlai@gmail.com> wrote:
> >
> > On Tue, Dec 6, 2022 at 5:20 PM richard clark
> > <richard.xnu.clark@gmail.com> wrote:
> > >
> > > On Tue, Dec 6, 2022 at 2:23 PM Lai Jiangshan <jiangshanlai@gmail.com> wrote:
> > > >
> > > > On Tue, Dec 6, 2022 at 12:35 PM richard clark
> > > > <richard.xnu.clark@gmail.com> wrote:
> > > >
> > > > > >
> > > > > A WARN is definitely reasonable and has its benefits. Can I try to
> > > > > submit the patch and you're nice to review as maintainer?
> > > > >
> > > > > Thanks,
> > > > > Richard
> > > > > >
> > > >
> > > > Sure, go ahead.
> > > >
> > > > What's in my mind is that the following code is wrapped in a new function:
> > > >
> > > >         mutex_lock(&wq->mutex);
> > > >         if (!wq->nr_drainers++)
> > > >                 wq->flags |= __WQ_DRAINING;
> > > >         mutex_unlock(&wq->mutex);
> > > >
> > > >
> > > > and the new function replaces the open code drain_workqueue() and
> > > > is also called in destroy_workqueue() (before calling drain_workqueue()).
> > > >
> > > Except that, do we need to defer the __WQ_DRAINING clean to the
> > > rcu_call, thus we still have a close-loop of the drainer's count, like
> > > this?
> >
> > No, I don't think we need it. The wq is totally freed in rcu_free_wq.
> >
> > Or we can just introduce __WQ_DESTROYING.
> >
> > It seems using __WQ_DESTROYING is better.
>
> The wq->flags will be unreliable after kfree(wq), for example, in my
> machine, the wq->flags can be 0x7ec1e1a3, 0x37cff1a3 or 0x7fa23da3 ...
> after wq be kfreed, consequently the result of queueing a new work
> item to a kfreed wq is undetermined, sometimes it's ok because the
> queue_work will return directly(e.g, the wq->flags = 0x7ec1e1a3, a
> fake __WQ_DRAINING state), sometimes it will trigger a kernel NULL
> pointer dereference BUG when the wq->flags = 0x7fa23da3(fake
> !__WQ_DRAINING state).

The whole wq is unreliable after destroy_workqueue().

All we need is just adding something to help identify any
wrong usage while the wq is in RCU grace period.

>
> IMO, given the above condition,  we can handle this in 2 phases:
> before the rcu_call and after.
> a. before rcu_call. Using __WQ_DESTROYING to allow the chained work
> queued in or not in destroy_workqueue(...) level, __WQ_DRAINING is
> used to make the drain_workqueue(...) still can be standalone. The
> code snippet like this:
> destroy_workqueue(...)
> {
>         mutex_lock(&wq->mutex);
>         wq->flags |= __WQ_DESTROYING;
>         mutex_lock(&wq->mutex);

Ok, put it before calling drain_workqueue()

>         ...
> }
>
> __queue_work(...)
> {
>           if (unlikely((wq->flags & __WQ_DESTROYING) || (wq->flags &
> __WQ_DRAINING)) &&
>                    WARN_ON_ONCE(!is_chained_work(wq)))

Ok, combine __WQ_DESTROYING and __WQ_DRAINING together as:
           if (unlikely((wq->flags & (__WQ_DESTROYING | __WQ_DRAINING)) &&


>          return;
> }
>
> b. after rcu_call. What in my mind is:
> rcu_free_wq(struct rcu_head *rcu)
> {
>           ...
>           kfree(wq);
>           wq = NULL;

It is useless code.

> }
>
> __queue_work(...)
> {
>         if (!wq)
>                 return;

It is useless code.

>         ...
> }
>
> Any comments?
>
> >
> > >
> > > --- a/kernel/workqueue.c
> > > +++ b/kernel/workqueue.c
> > >
> > > @@ -3528,6 +3526,9 @@ static void rcu_free_wq(struct rcu_head *rcu)
> > >
> > >         else
> > >                 free_workqueue_attrs(wq->unbound_attrs);
> > >
> > > +       if (!--wq->nr_drainers)
> > > +               wq->flags &= ~__WQ_DRAINING;
> > > +
> > >         kfree(wq);
> > >
> > > >
> > > > __WQ_DRAINING will cause the needed WARN on illegally queuing items on
> > > > destroyed workqueue.
> > >
> > > I will re-test it if there are no concerns about the above fix...
> > >
> > > >
> > > > Thanks
> > > > Lai

Re: work item still be scheduled to execute after destroy_workqueue?

[richard clark]

On Thu, Dec 8, 2022 at 3:46 PM Lai Jiangshan <jiangshanlai@gmail.com> wrote:
>
> On Thu, Dec 8, 2022 at 10:44 AM richard clark
> <richard.xnu.clark@gmail.com> wrote:
> >
> > On Wed, Dec 7, 2022 at 10:38 AM Lai Jiangshan <jiangshanlai@gmail.com> wrote:
> > >
> > > On Tue, Dec 6, 2022 at 5:20 PM richard clark
> > > <richard.xnu.clark@gmail.com> wrote:
> > > >
> > > > On Tue, Dec 6, 2022 at 2:23 PM Lai Jiangshan <jiangshanlai@gmail.com> wrote:
> > > > >
> > > > > On Tue, Dec 6, 2022 at 12:35 PM richard clark
> > > > > <richard.xnu.clark@gmail.com> wrote:
> > > > >
> > > > > > >
> > > > > > A WARN is definitely reasonable and has its benefits. Can I try to
> > > > > > submit the patch and you're nice to review as maintainer?
> > > > > >
> > > > > > Thanks,
> > > > > > Richard
> > > > > > >
> > > > >
> > > > > Sure, go ahead.
> > > > >
> > > > > What's in my mind is that the following code is wrapped in a new function:
> > > > >
> > > > >         mutex_lock(&wq->mutex);
> > > > >         if (!wq->nr_drainers++)
> > > > >                 wq->flags |= __WQ_DRAINING;
> > > > >         mutex_unlock(&wq->mutex);
> > > > >
> > > > >
> > > > > and the new function replaces the open code drain_workqueue() and
> > > > > is also called in destroy_workqueue() (before calling drain_workqueue()).
> > > > >
> > > > Except that, do we need to defer the __WQ_DRAINING clean to the
> > > > rcu_call, thus we still have a close-loop of the drainer's count, like
> > > > this?
> > >
> > > No, I don't think we need it. The wq is totally freed in rcu_free_wq.
> > >
> > > Or we can just introduce __WQ_DESTROYING.
> > >
> > > It seems using __WQ_DESTROYING is better.
> >
> > The wq->flags will be unreliable after kfree(wq), for example, in my
> > machine, the wq->flags can be 0x7ec1e1a3, 0x37cff1a3 or 0x7fa23da3 ...
> > after wq be kfreed, consequently the result of queueing a new work
> > item to a kfreed wq is undetermined, sometimes it's ok because the
> > queue_work will return directly(e.g, the wq->flags = 0x7ec1e1a3, a
> > fake __WQ_DRAINING state), sometimes it will trigger a kernel NULL
> > pointer dereference BUG when the wq->flags = 0x7fa23da3(fake
> > !__WQ_DRAINING state).
>
> The whole wq is unreliable after destroy_workqueue().
>
> All we need is just adding something to help identify any
> wrong usage while the wq is in RCU grace period.
>
OK, understood!
> >
> > IMO, given the above condition,  we can handle this in 2 phases:
> > before the rcu_call and after.
> > a. before rcu_call. Using __WQ_DESTROYING to allow the chained work
> > queued in or not in destroy_workqueue(...) level, __WQ_DRAINING is
> > used to make the drain_workqueue(...) still can be standalone. The
> > code snippet like this:
> > destroy_workqueue(...)
> > {
> >         mutex_lock(&wq->mutex);
> >         wq->flags |= __WQ_DESTROYING;
> >         mutex_lock(&wq->mutex);
>
> Ok, put it before calling drain_workqueue()
>
> >         ...
> > }
> >
> > __queue_work(...)
> > {
> >           if (unlikely((wq->flags & __WQ_DESTROYING) || (wq->flags &
> > __WQ_DRAINING)) &&
> >                    WARN_ON_ONCE(!is_chained_work(wq)))
>
> Ok, combine __WQ_DESTROYING and __WQ_DRAINING together as:
>            if (unlikely((wq->flags & (__WQ_DESTROYING | __WQ_DRAINING)) &&
>
>
> >          return;
> > }
> >
> > b. after rcu_call. What in my mind is:
> > rcu_free_wq(struct rcu_head *rcu)
> > {
> >           ...
> >           kfree(wq);
> >           wq = NULL;
>
> It is useless code.
>
> > }
> >
> > __queue_work(...)
> > {
> >         if (!wq)
> >                 return;
>
> It is useless code.

OK, will remove the above codes in the patch...

>
> >         ...
> > }
> >
> > Any comments?
> >
> > >
> > > >
> > > > --- a/kernel/workqueue.c
> > > > +++ b/kernel/workqueue.c
> > > >
> > > > @@ -3528,6 +3526,9 @@ static void rcu_free_wq(struct rcu_head *rcu)
> > > >
> > > >         else
> > > >                 free_workqueue_attrs(wq->unbound_attrs);
> > > >
> > > > +       if (!--wq->nr_drainers)
> > > > +               wq->flags &= ~__WQ_DRAINING;
> > > > +
> > > >         kfree(wq);
> > > >
> > > > >
> > > > > __WQ_DRAINING will cause the needed WARN on illegally queuing items on
> > > > > destroyed workqueue.
> > > >
> > > > I will re-test it if there are no concerns about the above fix...
> > > >
> > > > >
> > > > > Thanks
> > > > > Lai