* [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
@ 2020-05-27 7:57 qiang.zhang
2020-05-27 8:20 ` Markus Elfring
2020-05-27 13:52 ` [PATCH v5] " Tejun Heo
0 siblings, 2 replies; 17+ messages in thread
From: qiang.zhang @ 2020-05-27 7:57 UTC (permalink / raw)
To: tj; +Cc: jiangshanlai, markus.elfring, linux-kernel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
2020-05-27 7:57 [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq() qiang.zhang
@ 2020-05-27 8:20 ` Markus Elfring
[not found] ` <DM6PR11MB32573F3884A864ECD586235EFF8E0@DM6PR11MB3257.namprd11.prod.outlook.com>
2020-05-27 13:52 ` [PATCH v5] " Tejun Heo
1 sibling, 1 reply; 17+ messages in thread
From: Markus Elfring @ 2020-05-27 8:20 UTC (permalink / raw)
To: Zhang Qiang, Tejun Heo, Lai Jiangshan; +Cc: linux-kernel, kernel-janitors
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
2020-05-27 7:57 [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq() qiang.zhang
2020-05-27 8:20 ` Markus Elfring
@ 2020-05-27 13:52 ` Tejun Heo
1 sibling, 0 replies; 17+ messages in thread
From: Tejun Heo @ 2020-05-27 13:52 UTC (permalink / raw)
To: qiang.zhang; +Cc: jiangshanlai, markus.elfring, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* 回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
[not found] ` <DM6PR11MB32573F3884A864ECD586235EFF8E0@DM6PR11MB3257.namprd11.prod.outlook.com>
@ 2020-05-28 1:44 ` Zhang, Qiang
2020-05-28 9:57 ` Dan Carpenter
0 siblings, 1 reply; 17+ messages in thread
From: Zhang, Qiang @ 2020-05-28 1:44 UTC (permalink / raw)
To: Markus Elfring, Tejun Heo, Lai Jiangshan; +Cc: linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
2020-05-28 1:44 ` 回复: " Zhang, Qiang
@ 2020-05-28 9:57 ` Dan Carpenter
2020-05-28 10:38 ` Tejun Heo
2020-05-28 12:08 ` 回复: [PATCH v5] " Lai Jiangshan
0 siblings, 2 replies; 17+ messages in thread
From: Dan Carpenter @ 2020-05-28 9:57 UTC (permalink / raw)
To: Zhang, Qiang
Cc: Markus Elfring, Tejun Heo, Lai Jiangshan, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
2020-05-28 9:57 ` Dan Carpenter
@ 2020-05-28 10:38 ` Tejun Heo
2020-05-28 11:00 ` [v5] " Markus Elfring
2020-05-28 12:08 ` 回复: [PATCH v5] " Lai Jiangshan
1 sibling, 1 reply; 17+ messages in thread
From: Tejun Heo @ 2020-05-28 10:38 UTC (permalink / raw)
To: Dan Carpenter
Cc: Zhang, Qiang, Markus Elfring, Lai Jiangshan, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
2020-05-28 10:38 ` Tejun Heo
@ 2020-05-28 11:00 ` Markus Elfring
2020-05-28 14:40 ` Markus Elfring
0 siblings, 1 reply; 17+ messages in thread
From: Markus Elfring @ 2020-05-28 11:00 UTC (permalink / raw)
To: Tejun Heo, Dan Carpenter, Zhang Qiang, Lai Jiangshan
Cc: linux-kernel, kernel-janitors
>> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
2020-05-28 9:57 ` Dan Carpenter
2020-05-28 10:38 ` Tejun Heo
@ 2020-05-28 12:08 ` Lai Jiangshan
2020-05-28 12:25 ` Dan Carpenter
` (2 more replies)
1 sibling, 3 replies; 17+ messages in thread
From: Lai Jiangshan @ 2020-05-28 12:08 UTC (permalink / raw)
To: Dan Carpenter
Cc: Zhang, Qiang, Markus Elfring, Tejun Heo, linux-kernel, kernel-janitors
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
2020-05-28 12:08 ` 回复: [PATCH v5] " Lai Jiangshan
@ 2020-05-28 12:25 ` Dan Carpenter
2020-05-28 13:27 ` Lai Jiangshan
2020-05-28 15:25 ` [v5] " Markus Elfring
2020-05-28 15:02 ` Markus Elfring
2020-05-28 15:02 ` Markus Elfring
2 siblings, 2 replies; 17+ messages in thread
From: Dan Carpenter @ 2020-05-28 12:25 UTC (permalink / raw)
To: Lai Jiangshan
Cc: Zhang, Qiang, Markus Elfring, Tejun Heo, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
2020-05-28 12:25 ` Dan Carpenter
@ 2020-05-28 13:27 ` Lai Jiangshan
2020-05-28 14:03 ` Tejun Heo
2020-05-28 14:06 ` 回复: [PATCH v5] " Dan Carpenter
2020-05-28 15:25 ` [v5] " Markus Elfring
1 sibling, 2 replies; 17+ messages in thread
From: Lai Jiangshan @ 2020-05-28 13:27 UTC (permalink / raw)
To: Dan Carpenter
Cc: Zhang, Qiang, Markus Elfring, Tejun Heo, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
2020-05-28 13:27 ` Lai Jiangshan
@ 2020-05-28 14:03 ` Tejun Heo
2020-05-28 14:45 ` [v5] " Markus Elfring
2020-05-28 14:06 ` 回复: [PATCH v5] " Dan Carpenter
1 sibling, 1 reply; 17+ messages in thread
From: Tejun Heo @ 2020-05-28 14:03 UTC (permalink / raw)
To: Lai Jiangshan
Cc: Dan Carpenter, Zhang, Qiang, Markus Elfring, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: 回复: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
2020-05-28 13:27 ` Lai Jiangshan
2020-05-28 14:03 ` Tejun Heo
@ 2020-05-28 14:06 ` Dan Carpenter
1 sibling, 0 replies; 17+ messages in thread
From: Dan Carpenter @ 2020-05-28 14:06 UTC (permalink / raw)
To: Lai Jiangshan
Cc: Zhang, Qiang, Markus Elfring, Tejun Heo, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
2020-05-28 11:00 ` [v5] " Markus Elfring
@ 2020-05-28 14:40 ` Markus Elfring
0 siblings, 0 replies; 17+ messages in thread
From: Markus Elfring @ 2020-05-28 14:40 UTC (permalink / raw)
To: Tejun Heo, Dan Carpenter, Zhang Qiang, Lai Jiangshan
Cc: linux-kernel, kernel-janitors
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
2020-05-28 14:03 ` Tejun Heo
@ 2020-05-28 14:45 ` Markus Elfring
0 siblings, 0 replies; 17+ messages in thread
From: Markus Elfring @ 2020-05-28 14:45 UTC (permalink / raw)
To: Tejun Heo, Lai Jiangshan
Cc: Dan Carpenter, Zhang Qiang, linux-kernel, kernel-janitors
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
2020-05-28 12:08 ` 回复: [PATCH v5] " Lai Jiangshan
2020-05-28 12:25 ` Dan Carpenter
@ 2020-05-28 15:02 ` Markus Elfring
2020-05-28 15:02 ` Markus Elfring
2 siblings, 0 replies; 17+ messages in thread
From: Markus Elfring @ 2020-05-28 15:02 UTC (permalink / raw)
To: Lai Jiangshan, Dan Carpenter, Tejun Heo
Cc: Zhang Qiang, linux-kernel, kernel-janitors
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
2020-05-28 12:08 ` 回复: [PATCH v5] " Lai Jiangshan
2020-05-28 12:25 ` Dan Carpenter
2020-05-28 15:02 ` Markus Elfring
@ 2020-05-28 15:02 ` Markus Elfring
2 siblings, 0 replies; 17+ messages in thread
From: Markus Elfring @ 2020-05-28 15:02 UTC (permalink / raw)
To: Lai Jiangshan, Dan Carpenter, Tejun Heo
Cc: Zhang Qiang, linux-kernel, kernel-janitors
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
2020-05-28 12:25 ` Dan Carpenter
2020-05-28 13:27 ` Lai Jiangshan
@ 2020-05-28 15:25 ` Markus Elfring
1 sibling, 0 replies; 17+ messages in thread
From: Markus Elfring @ 2020-05-28 15:25 UTC (permalink / raw)
To: Dan Carpenter, Lai Jiangshan
Cc: Zhang Qiang, Tejun Heo, linux-kernel, kernel-janitors
>> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-05-28 15:25 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-27 7:57 [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq() qiang.zhang
2020-05-27 8:20 ` Markus Elfring
[not found] ` <DM6PR11MB32573F3884A864ECD586235EFF8E0@DM6PR11MB3257.namprd11.prod.outlook.com>
2020-05-28 1:44 ` 回复: " Zhang, Qiang
2020-05-28 9:57 ` Dan Carpenter
2020-05-28 10:38 ` Tejun Heo
2020-05-28 11:00 ` [v5] " Markus Elfring
2020-05-28 14:40 ` Markus Elfring
2020-05-28 12:08 ` 回复: [PATCH v5] " Lai Jiangshan
2020-05-28 12:25 ` Dan Carpenter
2020-05-28 13:27 ` Lai Jiangshan
2020-05-28 14:03 ` Tejun Heo
2020-05-28 14:45 ` [v5] " Markus Elfring
2020-05-28 14:06 ` 回复: [PATCH v5] " Dan Carpenter
2020-05-28 15:25 ` [v5] " Markus Elfring
2020-05-28 15:02 ` Markus Elfring
2020-05-28 15:02 ` Markus Elfring
2020-05-27 13:52 ` [PATCH v5] " Tejun Heo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).