Thanks for your guide.
I will try to change the weakness of weak wording.
________________________________________
??????: Zhang, Qiang <[email protected]>
????ʱ??: 2020??5??28?? 9:41
?ռ???: Markus Elfring; Tejun Heo; Lai Jiangshan
????: [email protected]; [email protected]
????: ?ظ?: [PATCH v5] workqueue: Remove unnecessary kfree() call in rcu_free_wq()
Thanks for your guide. I tried to change the weakness of weak wording
________________________________
??????: [email protected] <[email protected]> ???? Markus Elfring <[email protected]>
????ʱ??: 2020??5??27?? 16:20
?ռ???: Zhang, Qiang <[email protected]>; Tejun Heo <[email protected]>; Lai Jiangshan <[email protected]>
????: [email protected] <[email protected]>; [email protected] <[email protected]>
????: 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 <[email protected]>
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
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
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
>> 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
On Thu, May 28, 2020 at 5:57 PM Dan Carpenter <[email protected]> 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.
On Thu, May 28, 2020 at 08:08:06PM +0800, Lai Jiangshan wrote:
> On Thu, May 28, 2020 at 5:57 PM Dan Carpenter <[email protected]> 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
On Thu, May 28, 2020 at 8:27 PM Dan Carpenter <[email protected]> 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 <[email protected]> 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
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
On Thu, May 28, 2020 at 09:27:03PM +0800, Lai Jiangshan wrote:
> On Thu, May 28, 2020 at 8:27 PM Dan Carpenter <[email protected]> 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 <[email protected]> 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
> 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
> 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
> 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
> 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
>> 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