diff mbox series

workqueue: Remove the unbound release work from the per-cpu type

Message ID 20221123095058.669684-1-richard.xnu.clark@gmail.com
State New
Headers
Series workqueue: Remove the unbound release work from the per-cpu type |

Commit Message

richard clark Nov. 23, 2022, 9:50 a.m. UTC 
  Both the per-cpu and unbound workqueue will call init_pwq(...) currently,
the latter will init an unbound release work for the pwq which is unnecessary
for the per-cpu type workqueue.
This commit will remove this work item from the per-cpu workqueue by checking the
wq->flags in init_pwq(...), the work is still reserved for the unbound workqueue.

Signed-off-by: Richard Clark <richard.xnu.clark@gmail.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
---
 kernel/workqueue.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

richard clark Nov. 25, 2022, 3:01 a.m. UTC | #1 
Hi Tejun,

Would you pls help take a look at this? Or point to someone who is
maintaining the workqueue subsystem, I can ping him/her...

Richard

On Wed, Nov 23, 2022 at 5:51 PM Richard Clark
<richard.xnu.clark@gmail.com> wrote:
>
> Both the per-cpu and unbound workqueue will call init_pwq(...) currently,
> the latter will init an unbound release work for the pwq which is unnecessary
> for the per-cpu type workqueue.
> This commit will remove this work item from the per-cpu workqueue by checking the
> wq->flags in init_pwq(...), the work is still reserved for the unbound workqueue.
>
> Signed-off-by: Richard Clark <richard.xnu.clark@gmail.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> ---
>  kernel/workqueue.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 7cd5f5e7e0a1..01bdfb74081e 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -3807,7 +3807,8 @@ static void init_pwq(struct pool_workqueue *pwq, struct workqueue_struct *wq,
>         INIT_LIST_HEAD(&pwq->inactive_works);
>         INIT_LIST_HEAD(&pwq->pwqs_node);
>         INIT_LIST_HEAD(&pwq->mayday_node);
> -       INIT_WORK(&pwq->unbound_release_work, pwq_unbound_release_workfn);
> +       if (wq->flags & WQ_UNBOUND)
> +               INIT_WORK(&pwq->unbound_release_work, pwq_unbound_release_workfn);
>  }
>
>  /* sync @pwq with the current state of its associated wq and link it */
> --
> 2.37.2
>
Lai Jiangshan Nov. 28, 2022, 3:24 a.m. UTC | #2 
On Fri, Nov 25, 2022 at 11:02 AM richard clark
<richard.xnu.clark@gmail.com> wrote:
>
> Hi Tejun,
>
> Would you pls help take a look at this? Or point to someone who is
> maintaining the workqueue subsystem, I can ping him/her...
>
> Richard

Hello, Richard

Thank you for reviewing the code of workqueue and trying to improve it.

But INIT_WORK() has no unwanted effect and it is better to initialize
the field even if it is unused.

The patch would not be accepted.

Thanks.
Lai

>
> On Wed, Nov 23, 2022 at 5:51 PM Richard Clark
> <richard.xnu.clark@gmail.com> wrote:
> >
> > Both the per-cpu and unbound workqueue will call init_pwq(...) currently,
> > the latter will init an unbound release work for the pwq which is unnecessary
> > for the per-cpu type workqueue.
> > This commit will remove this work item from the per-cpu workqueue by checking the
> > wq->flags in init_pwq(...), the work is still reserved for the unbound workqueue.
> >
> > Signed-off-by: Richard Clark <richard.xnu.clark@gmail.com>
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > ---
> >  kernel/workqueue.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index 7cd5f5e7e0a1..01bdfb74081e 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -3807,7 +3807,8 @@ static void init_pwq(struct pool_workqueue *pwq, struct workqueue_struct *wq,
> >         INIT_LIST_HEAD(&pwq->inactive_works);
> >         INIT_LIST_HEAD(&pwq->pwqs_node);
> >         INIT_LIST_HEAD(&pwq->mayday_node);
> > -       INIT_WORK(&pwq->unbound_release_work, pwq_unbound_release_workfn);
> > +       if (wq->flags & WQ_UNBOUND)
> > +               INIT_WORK(&pwq->unbound_release_work, pwq_unbound_release_workfn);
> >  }
> >
> >  /* sync @pwq with the current state of its associated wq and link it */
> > --
> > 2.37.2
> >
richard clark Nov. 28, 2022, 5:25 a.m. UTC | #3 
Hi Lai,

On Mon, Nov 28, 2022 at 11:24 AM Lai Jiangshan <jiangshanlai@gmail.com> wrote:
>
> On Fri, Nov 25, 2022 at 11:02 AM richard clark
> <richard.xnu.clark@gmail.com> wrote:
> >
> > Hi Tejun,
> >
> > Would you pls help take a look at this? Or point to someone who is
> > maintaining the workqueue subsystem, I can ping him/her...
> >
> > Richard
>
> Hello, Richard
>
> Thank you for reviewing the code of workqueue and trying to improve it.
>
> But INIT_WORK() has no unwanted effect and it is better to initialize
> the field even if it is unused.
>
IMO, what you said is a perfect 'logical paradox', you think it 'even
if it is unused', then the interesting conclusion is 'it is better to'
use it, would you pls help to justify the underlying logic here?
'unused' but 'better', hmmm...
>
> The patch would not be accepted.
That's definitely fine, forgive me for being a perfectionist to see
the codes... Please note that I've no intention to offend you, trust
me :)

Thanks,
Richard

>
> Thanks.
> Lai
>
> >
> > On Wed, Nov 23, 2022 at 5:51 PM Richard Clark
> > <richard.xnu.clark@gmail.com> wrote:
> > >
> > > Both the per-cpu and unbound workqueue will call init_pwq(...) currently,
> > > the latter will init an unbound release work for the pwq which is unnecessary
> > > for the per-cpu type workqueue.
> > > This commit will remove this work item from the per-cpu workqueue by checking the
> > > wq->flags in init_pwq(...), the work is still reserved for the unbound workqueue.
> > >
> > > Signed-off-by: Richard Clark <richard.xnu.clark@gmail.com>
> > > Cc: Tejun Heo <tj@kernel.org>
> > > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > ---
> > >  kernel/workqueue.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > > index 7cd5f5e7e0a1..01bdfb74081e 100644
> > > --- a/kernel/workqueue.c
> > > +++ b/kernel/workqueue.c
> > > @@ -3807,7 +3807,8 @@ static void init_pwq(struct pool_workqueue *pwq, struct workqueue_struct *wq,
> > >         INIT_LIST_HEAD(&pwq->inactive_works);
> > >         INIT_LIST_HEAD(&pwq->pwqs_node);
> > >         INIT_LIST_HEAD(&pwq->mayday_node);
> > > -       INIT_WORK(&pwq->unbound_release_work, pwq_unbound_release_workfn);
> > > +       if (wq->flags & WQ_UNBOUND)
> > > +               INIT_WORK(&pwq->unbound_release_work, pwq_unbound_release_workfn);
> > >  }
> > >
> > >  /* sync @pwq with the current state of its associated wq and link it */
> > > --
> > > 2.37.2
> > >
diff mbox series

Patch

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 7cd5f5e7e0a1..01bdfb74081e 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3807,7 +3807,8 @@  static void init_pwq(struct pool_workqueue *pwq, struct workqueue_struct *wq,
 	INIT_LIST_HEAD(&pwq->inactive_works);
 	INIT_LIST_HEAD(&pwq->pwqs_node);
 	INIT_LIST_HEAD(&pwq->mayday_node);
-	INIT_WORK(&pwq->unbound_release_work, pwq_unbound_release_workfn);
+	if (wq->flags & WQ_UNBOUND)
+		INIT_WORK(&pwq->unbound_release_work, pwq_unbound_release_workfn);
 }
 
 /* sync @pwq with the current state of its associated wq and link it */