Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CAJNi4rOgYmmtOaXVqYB9sAxDmRhGhS_vVXmZbCbMjvFCQsdjCw@mail.gmail.com>
 <CAJhGHyD+oR8SeYeObJ2DSKfudqBtBOiL14jCsST3L5OG8WjT_g@mail.gmail.com>
In-Reply-To: <CAJhGHyD+oR8SeYeObJ2DSKfudqBtBOiL14jCsST3L5OG8WjT_g@mail.gmail.com>
From:   richard clark <richard.xnu.clark@gmail.com>
Date:   Tue, 6 Dec 2022 12:35:41 +0800
Message-ID: <CAJNi4rOs-=xx5qV-hQQYgSLQCz_q3JuFxJEd+wpPaao8Ej26yQ@mail.gmail.com>
Subject: Re: work item still be scheduled to execute after destroy_workqueue?
To:     Lai Jiangshan <jiangshanlai@gmail.com>
Cc:     tj@kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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?