[PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[qiang.zhang]

From: Zhang Qiang <qiang.zhang@windriver.com>

The data structure member "wq->rescuer" was reset to a null pointer
in one if branch. It was passed to a call of the function "kfree"
in the callback function "rcu_free_wq" (which was eventually executed).
The function "kfree" does not perform more meaningful data processing
for a passed null pointer (besides immediately returning from such a call).
Thus delete this function call which became unnecessary with the referenced
software update.

Fixes: def98c84b6cd ("workqueue: Fix spurious sanity check failures in destroy_workqueue()")

Suggested-by: Markus Elfring <Markus.Elfring@web.de> 
Signed-off-by: Zhang Qiang <qiang.zhang@windriver.com>
---
 v1->v2->v3->v4->v5:
 Modify weakly submitted information.

 kernel/workqueue.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 891ccad5f271..a2451cdcd503 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3491,7 +3491,6 @@ static void rcu_free_wq(struct rcu_head *rcu)
 	else
 		free_workqueue_attrs(wq->unbound_attrs);
 
-	kfree(wq->rescuer);
 	kfree(wq);
 }
 
-- 
2.24.1

Re: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[Markus Elfring]

> Thus delete this function call which became unnecessary with the referenced
> software update.
…
> Suggested-by: Markus Elfring <Markus.Elfring@web.de>

Would the tag “Co-developed-by” be more appropriate according to the patch review
to achieve a more pleasing commit message?


>  v1->v2->v3->v4->v5:
>  Modify weakly submitted information.

Now I wonder about your wording choice “weakly”.

Regards,
Markus

回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[Zhang, Qiang]

Thanks for your guide.
I will try to change the weakness of weak wording.

________________________________________
发件人: Zhang, Qiang <Qiang.Zhang@windriver.com>
发送时间: 2020年5月28日 9:41
收件人: Markus Elfring; Tejun Heo; Lai Jiangshan
抄送: linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org
主题: 回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

Thanks for your guide. I tried to change the weakness of weak wording


________________________________
发件人: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> 代表 Markus Elfring <Markus.Elfring@web.de>
发送时间: 2020年5月27日 16:20
收件人: Zhang, Qiang <Qiang.Zhang@windriver.com>; Tejun Heo <tj@kernel.org>; Lai Jiangshan <jiangshanlai@gmail.com>
抄送: linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; kernel-janitors@vger.kernel.org <kernel-janitors@vger.kernel.org>
主题: Re: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

> Thus delete this function call which became unnecessary with the referenced
> software update.
…
> Suggested-by: Markus Elfring <Markus.Elfring@web.de>

Would the tag “Co-developed-by” be more appropriate according to the patch review
to achieve a more pleasing commit message?


>  v1->v2->v3->v4->v5:
>  Modify weakly submitted information.

Now I wonder about your wording choice “weakly”.

Regards,
Markus

Re: 回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[Dan Carpenter]

Guys, the patch is wrong.  The kfree is harmless when this is called
from destroy_workqueue() and required when it's called from
pwq_unbound_release_workfn().  Lai Jiangshan already explained this
already.  Why are we still discussing this?

regards,
dan carpenter

Re: 回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[Tejun Heo]

On Thu, May 28, 2020 at 12:57:03PM +0300, Dan Carpenter wrote:
> Guys, the patch is wrong.  The kfree is harmless when this is called
> from destroy_workqueue() and required when it's called from
> pwq_unbound_release_workfn().  Lai Jiangshan already explained this
> already.  Why are we still discussing this?

Oops, missed that. Reverting.

Thanks.

-- 
tejun

Re: [v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[Markus Elfring]

>> Guys, the patch is wrong.  The kfree is harmless when this is called
>> from destroy_workqueue() and required when it's called from
>> pwq_unbound_release_workfn().  Lai Jiangshan already explained this
>> already.  Why are we still discussing this?
>
> Oops, missed that. Reverting.

Can it matter to use separate callback functions for these cases?
https://elixir.bootlin.com/linux/v5.7-rc7/C/ident/rcu_free_pwq

Regards,
Markus

Re: [v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[Markus Elfring]

> Can it matter to use separate callback functions for these cases?
> https://elixir.bootlin.com/linux/v5.7-rc7/C/ident/rcu_free_pwq

See also:
https://elixir.bootlin.com/linux/v5.7-rc7/C/ident/rcu_free_wq

Regards,
Markus

Re: 回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[Lai Jiangshan]

On Thu, May 28, 2020 at 5:57 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Guys, the patch is wrong.  The kfree is harmless when this is called
> from destroy_workqueue() and required when it's called from
> pwq_unbound_release_workfn().  Lai Jiangshan already explained this
> already.  Why are we still discussing this?
>

I'm also confused why they have been debating about the changelog
after the patch was queued. My statement was about "the patch is
a correct cleanup, but the changelog is totally misleading".

destroy_workqueue(percpu_wq) -> rcu_free_wq()
or
destroy_workqueue(unbound_wq) -> put_pwq() ->
pwq_unbound_release_workfn() -> rcu_free_wq()

So the patch is correct to me. Only can destroy_workqueue()
lead to rcu_free_wq().

Still, the kfree(NULL) is harmless. But it is cleaner
to have the patch. But the changelog is wrong, even after
the lengthened debating, and English is not my mother tongue,
so I just looked on.

Re: 回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[Dan Carpenter]

On Thu, May 28, 2020 at 08:08:06PM +0800, Lai Jiangshan wrote:
> On Thu, May 28, 2020 at 5:57 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > Guys, the patch is wrong.  The kfree is harmless when this is called
> > from destroy_workqueue() and required when it's called from
> > pwq_unbound_release_workfn().  Lai Jiangshan already explained this
> > already.  Why are we still discussing this?
> >
> 
> I'm also confused why they have been debating about the changelog
> after the patch was queued. My statement was about "the patch is
> a correct cleanup, but the changelog is totally misleading".
> 
> destroy_workqueue(percpu_wq) -> rcu_free_wq()
> or
> destroy_workqueue(unbound_wq) -> put_pwq() ->
> pwq_unbound_release_workfn() -> rcu_free_wq()
> 
> So the patch is correct to me. Only can destroy_workqueue()
> lead to rcu_free_wq().

It looks like there are lots of paths which call put_pwq() and
put_pwq_unlocked().

  1168  static void pwq_dec_nr_in_flight(struct pool_workqueue *pwq, int color)
  1169  {
  1170          /* uncolored work items don't participate in flushing or nr_active */
  1171          if (color == WORK_NO_COLOR)
  1172                  goto out_put;
  1173  

We don't take an extra reference in this function.

  1200  out_put:
  1201          put_pwq(pwq);
  1202  }

I don't know this code well, so I will defer to your expertise if you
say it is correct.

> 
> Still, the kfree(NULL) is harmless. But it is cleaner
> to have the patch. But the changelog is wrong, even after
> the lengthened debating, and English is not my mother tongue,
> so I just looked on.

We have tried to tell Markus not to advise people about commit messages
but he doesn't listen.  He has discouraged some contributors.  :/

regards,
dan carpenter

Re: 回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[Lai Jiangshan]

On Thu, May 28, 2020 at 8:27 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Thu, May 28, 2020 at 08:08:06PM +0800, Lai Jiangshan wrote:
> > On Thu, May 28, 2020 at 5:57 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > Guys, the patch is wrong.  The kfree is harmless when this is called
> > > from destroy_workqueue() and required when it's called from
> > > pwq_unbound_release_workfn().  Lai Jiangshan already explained this
> > > already.  Why are we still discussing this?
> > >
> >
> > I'm also confused why they have been debating about the changelog
> > after the patch was queued. My statement was about "the patch is
> > a correct cleanup, but the changelog is totally misleading".
> >
> > destroy_workqueue(percpu_wq) -> rcu_free_wq()
> > or
> > destroy_workqueue(unbound_wq) -> put_pwq() ->
> > pwq_unbound_release_workfn() -> rcu_free_wq()
> >
> > So the patch is correct to me. Only can destroy_workqueue()
> > lead to rcu_free_wq().
>
> It looks like there are lots of paths which call put_pwq() and
> put_pwq_unlocked().
>
>   1168  static void pwq_dec_nr_in_flight(struct pool_workqueue *pwq, int color)
>   1169  {
>   1170          /* uncolored work items don't participate in flushing or nr_active */
>   1171          if (color == WORK_NO_COLOR)
>   1172                  goto out_put;
>   1173
>
> We don't take an extra reference in this function.
>
>   1200  out_put:
>   1201          put_pwq(pwq);
>   1202  }
>
> I don't know this code well, so I will defer to your expertise if you
> say it is correct.

wq owns the ultimate or permanent references to itself by
owning references to wq->numa_pwq_tbl[node], wq->dfl_pwq.
The pwq's references keep the pwq in wq->pwqs.

Only destroy_workqueue() can release these ultimate references
and then (after maybe a period of time) deplete the wq->pwqs
finally and then free itself in rcu callback.

Actually, in short, we don't need the care about the above
detail. The responsibility to free rescuer is on
destroy_workqueue(), and since it does the free early,
it doesn't need to do it again later.

e2dca7adff8f moved the free of rescuer into rcu callback,
and I didn't check all the changes between then and now.
But at least now, the rescuer is not accessed in rcu mananer,
so we don't need to free it in rcu mananer.

>
> >
> > Still, the kfree(NULL) is harmless. But it is cleaner
> > to have the patch. But the changelog is wrong, even after
> > the lengthened debating, and English is not my mother tongue,
> > so I just looked on.
>
> We have tried to tell Markus not to advise people about commit messages
> but he doesn't listen.  He has discouraged some contributors.  :/
>
> regards,
> dan carpenter

Re: 回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[Tejun Heo]

Hello,

On Thu, May 28, 2020 at 09:27:03PM +0800, Lai Jiangshan wrote:
> wq owns the ultimate or permanent references to itself by
> owning references to wq->numa_pwq_tbl[node], wq->dfl_pwq.
> The pwq's references keep the pwq in wq->pwqs.

Yeah, regardless of who puts a wq the last time, the base reference is put
by destroy_workqueue() and thus it's guaranteed that a wq can't be rcu freed
without going through destroy_workqueue(). lol I'm undoing the revert.

Thanks.

-- 
tejun

Re: [v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[Markus Elfring]

> Yeah, regardless of who puts a wq the last time, the base reference is put
> by destroy_workqueue() and thus it's guaranteed that a wq can't be rcu freed
> without going through destroy_workqueue(). lol I'm undoing the revert.

* Would you like to add such background information to the change description?

* How do you think about integrate a following patch version after
  the extra clarification?

Regards,
Markus

Re: 回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[Dan Carpenter]

On Thu, May 28, 2020 at 09:27:03PM +0800, Lai Jiangshan wrote:
> On Thu, May 28, 2020 at 8:27 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Thu, May 28, 2020 at 08:08:06PM +0800, Lai Jiangshan wrote:
> > > On Thu, May 28, 2020 at 5:57 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > Guys, the patch is wrong.  The kfree is harmless when this is called
> > > > from destroy_workqueue() and required when it's called from
> > > > pwq_unbound_release_workfn().  Lai Jiangshan already explained this
> > > > already.  Why are we still discussing this?
> > > >
> > >
> > > I'm also confused why they have been debating about the changelog
> > > after the patch was queued. My statement was about "the patch is
> > > a correct cleanup, but the changelog is totally misleading".
> > >
> > > destroy_workqueue(percpu_wq) -> rcu_free_wq()
> > > or
> > > destroy_workqueue(unbound_wq) -> put_pwq() ->
> > > pwq_unbound_release_workfn() -> rcu_free_wq()
> > >
> > > So the patch is correct to me. Only can destroy_workqueue()
> > > lead to rcu_free_wq().
> >
> > It looks like there are lots of paths which call put_pwq() and
> > put_pwq_unlocked().
> >
> >   1168  static void pwq_dec_nr_in_flight(struct pool_workqueue *pwq, int color)
> >   1169  {
> >   1170          /* uncolored work items don't participate in flushing or nr_active */
> >   1171          if (color == WORK_NO_COLOR)
> >   1172                  goto out_put;
> >   1173
> >
> > We don't take an extra reference in this function.
> >
> >   1200  out_put:
> >   1201          put_pwq(pwq);
> >   1202  }
> >
> > I don't know this code well, so I will defer to your expertise if you
> > say it is correct.
> 
> wq owns the ultimate or permanent references to itself by
> owning references to wq->numa_pwq_tbl[node], wq->dfl_pwq.
> The pwq's references keep the pwq in wq->pwqs.
> 
> Only destroy_workqueue() can release these ultimate references
> and then (after maybe a period of time) deplete the wq->pwqs
> finally and then free itself in rcu callback.
> 
> Actually, in short, we don't need the care about the above
> detail. The responsibility to free rescuer is on
> destroy_workqueue(), and since it does the free early,
> it doesn't need to do it again later.
> 
> e2dca7adff8f moved the free of rescuer into rcu callback,
> and I didn't check all the changes between then and now.
> But at least now, the rescuer is not accessed in rcu mananer,
> so we don't need to free it in rcu mananer.
> 

Ah...  Thanks for the explanation!

regards,
dan carpenter

Re: [v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[Markus Elfring]

>> Still, the kfree(NULL) is harmless. But it is cleaner
>> to have the patch. But the changelog is wrong, even after
>> the lengthened debating, and English is not my mother tongue,
>> so I just looked on.
>
> We have tried to tell Markus not to advise people about commit messages

A few concerns were mentioned.


> but he doesn't listen.

I am still listening as usual.

I am offering constructive patch reviews for selected topics.


> He has discouraged some contributors.  :/

There are the usual risks that additional views are occasionally not picked up
in positive ways.

Regards,
Markus

Re: [v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[Markus Elfring]

> I'm also confused why they have been debating about the changelog
> after the patch was queued.

I suggest to take another look at the provided patch review comments.


> My statement was about "the patch is a correct cleanup,
> but the changelog is totally misleading".

The commit message was accordingly adjusted, wasn't it?


> destroy_workqueue(percpu_wq) -> rcu_free_wq()
> or
> destroy_workqueue(unbound_wq) -> put_pwq() ->
> pwq_unbound_release_workfn() -> rcu_free_wq()
>
> So the patch is correct to me. Only can destroy_workqueue()
> lead to rcu_free_wq().
>
> Still, the kfree(NULL) is harmless. But it is cleaner
> to have the patch.

Thanks for such a feedback.


> But the changelog is wrong, even after the lengthened debating,

Do you expect any corresponding improvements?


> and English is not my mother tongue, so I just looked on.

How will the patch review evolve further despite of this information?

Regards,
Markus

Re: [v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[Markus Elfring]

> I'm also confused why they have been debating about the changelog
> after the patch was queued.

I suggest to take another look at the provided patch review comments.


> My statement was about "the patch is a correct cleanup,
> but the changelog is totally misleading".

The commit message was accordingly adjusted, wasn't it?


> destroy_workqueue(percpu_wq) -> rcu_free_wq()
> or
> destroy_workqueue(unbound_wq) -> put_pwq() ->
> pwq_unbound_release_workfn() -> rcu_free_wq()
>
> So the patch is correct to me. Only can destroy_workqueue()
> lead to rcu_free_wq().
>
> Still, the kfree(NULL) is harmless. But it is cleaner
> to have the patch.

Thanks for such a feedback.


> But the changelog is wrong, even after the lengthened debating,

Do you expect any corresponding improvements?


> and English is not my mother tongue, so I just looked on.

How will the patch review evolve further despite of this information?

Regards,
Markus

Re: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()

[Tejun Heo]

On Wed, May 27, 2020 at 03:57:15PM +0800, qiang.zhang@windriver.com wrote:
> From: Zhang Qiang <qiang.zhang@windriver.com>
> 
> The data structure member "wq->rescuer" was reset to a null pointer
> in one if branch. It was passed to a call of the function "kfree"
> in the callback function "rcu_free_wq" (which was eventually executed).
> The function "kfree" does not perform more meaningful data processing
> for a passed null pointer (besides immediately returning from such a call).
> Thus delete this function call which became unnecessary with the referenced
> software update.
> 
> Fixes: def98c84b6cd ("workqueue: Fix spurious sanity check failures in destroy_workqueue()")
> 
> Suggested-by: Markus Elfring <Markus.Elfring@web.de> 
> Signed-off-by: Zhang Qiang <qiang.zhang@windriver.com>

Applied to wq/for-5.8.

Thanks.

-- 
tejun