* workqueue: Only kick a worker after thawed or for an unbound workqueue
@ 2020-11-17 7:32 Yunfeng Ye
2020-11-18 4:06 ` Lai Jiangshan
0 siblings, 1 reply; 6+ messages in thread
From: Yunfeng Ye @ 2020-11-17 7:32 UTC (permalink / raw)
To: tj, jiangshanlai, linux-kernel; +Cc: Shiyuan Hu, Hewenliang
In realtime scenario, We do not want to have interference on the
isolated cpu cores. but when invoking alloc_workqueue() for percpu wq
on the housekeeping cpu, it kick a kworker on the isolated cpu.
alloc_workqueue
pwq_adjust_max_active
wake_up_worker
The comment in pwq_adjust_max_active() said:
"Need to kick a worker after thawed or an unbound wq's
max_active is bumped"
So it is unnecessary to kick a kworker for percpu wq's when
alloc_workqueue. this patch only kick a worker after thawed or for an
unbound workqueue.
Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
---
kernel/workqueue.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c41c3c17b86a..80f7bbd4889f 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3696,14 +3696,16 @@ static void pwq_unbound_release_workfn(struct work_struct *work)
}
/**
- * pwq_adjust_max_active - update a pwq's max_active to the current setting
+ * pwq_adjust_max_active_and_kick - update a pwq's max_active to the current setting
* @pwq: target pool_workqueue
+ * @force_kick: force to kick a worker
*
* If @pwq isn't freezing, set @pwq->max_active to the associated
* workqueue's saved_max_active and activate delayed work items
* accordingly. If @pwq is freezing, clear @pwq->max_active to zero.
*/
-static void pwq_adjust_max_active(struct pool_workqueue *pwq)
+static void pwq_adjust_max_active_and_kick(struct pool_workqueue *pwq,
+ bool force_kick)
{
struct workqueue_struct *wq = pwq->wq;
bool freezable = wq->flags & WQ_FREEZABLE;
@@ -3733,9 +3735,10 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
/*
* Need to kick a worker after thawed or an unbound wq's
- * max_active is bumped. It's a slow path. Do it always.
+ * max_active is bumped.
*/
- wake_up_worker(pwq->pool);
+ if (force_kick || (wq->flags & WQ_UNBOUND))
+ wake_up_worker(pwq->pool);
} else {
pwq->max_active = 0;
}
@@ -3743,6 +3746,11 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
raw_spin_unlock_irqrestore(&pwq->pool->lock, flags);
}
+static void pwq_adjust_max_active(struct pool_workqueue *pwq)
+{
+ pwq_adjust_max_active_and_kick(pwq, false);
+}
+
/* initialize newly alloced @pwq which is associated with @wq and @pool */
static void init_pwq(struct pool_workqueue *pwq, struct workqueue_struct *wq,
struct worker_pool *pool)
@@ -5252,7 +5260,7 @@ void thaw_workqueues(void)
list_for_each_entry(wq, &workqueues, list) {
mutex_lock(&wq->mutex);
for_each_pwq(pwq, wq)
- pwq_adjust_max_active(pwq);
+ pwq_adjust_max_active_and_kick(pwq, true);
mutex_unlock(&wq->mutex);
}
--
2.18.4
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: workqueue: Only kick a worker after thawed or for an unbound workqueue
2020-11-17 7:32 workqueue: Only kick a worker after thawed or for an unbound workqueue Yunfeng Ye
@ 2020-11-18 4:06 ` Lai Jiangshan
2020-11-18 6:26 ` Yunfeng Ye
0 siblings, 1 reply; 6+ messages in thread
From: Lai Jiangshan @ 2020-11-18 4:06 UTC (permalink / raw)
To: Yunfeng Ye; +Cc: Tejun Heo, LKML, Shiyuan Hu, Hewenliang
On Tue, Nov 17, 2020 at 3:33 PM Yunfeng Ye <yeyunfeng@huawei.com> wrote:
>
> In realtime scenario, We do not want to have interference on the
> isolated cpu cores. but when invoking alloc_workqueue() for percpu wq
> on the housekeeping cpu, it kick a kworker on the isolated cpu.
>
> alloc_workqueue
> pwq_adjust_max_active
> wake_up_worker
>
> The comment in pwq_adjust_max_active() said:
> "Need to kick a worker after thawed or an unbound wq's
> max_active is bumped"
>
> So it is unnecessary to kick a kworker for percpu wq's when
> alloc_workqueue. this patch only kick a worker after thawed or for an
> unbound workqueue.
>
> Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
> ---
> kernel/workqueue.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index c41c3c17b86a..80f7bbd4889f 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -3696,14 +3696,16 @@ static void pwq_unbound_release_workfn(struct work_struct *work)
> }
>
> /**
> - * pwq_adjust_max_active - update a pwq's max_active to the current setting
> + * pwq_adjust_max_active_and_kick - update a pwq's max_active to the current setting
> * @pwq: target pool_workqueue
> + * @force_kick: force to kick a worker
> *
> * If @pwq isn't freezing, set @pwq->max_active to the associated
> * workqueue's saved_max_active and activate delayed work items
> * accordingly. If @pwq is freezing, clear @pwq->max_active to zero.
> */
> -static void pwq_adjust_max_active(struct pool_workqueue *pwq)
> +static void pwq_adjust_max_active_and_kick(struct pool_workqueue *pwq,
> + bool force_kick)
> {
> struct workqueue_struct *wq = pwq->wq;
> bool freezable = wq->flags & WQ_FREEZABLE;
> @@ -3733,9 +3735,10 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
>
> /*
> * Need to kick a worker after thawed or an unbound wq's
> - * max_active is bumped. It's a slow path. Do it always.
> + * max_active is bumped.
Hello
Thanks for reporting the problem.
But I don't like to add an argument. The waking up is called
always just because it was considered no harm and it is slow
path. But it can still be possible to detect if the waking up
is really needed based on the actual activation of delayed works.
The previous lines are:
while (!list_empty(&pwq->delayed_works) &&
pwq->nr_active < pwq->max_active)
pwq_activate_first_delayed(pwq);
And you can record the old pwq->nr_active before these lines:
int old_nr_active = pwq->nr_active;
while (!list_empty(&pwq->delayed_works) &&
pwq->nr_active < pwq->max_active)
pwq_activate_first_delayed(pwq);
/* please add more comments here, see 951a078a5 */
if (old_nr_active < pwq->nr_active) {
if (!old_nr_active || (wq->flags & WQ_UNBOUND))
wake_up_worker(pwq->pool);
}
Thanks for your work.
Lai.
> */
> - wake_up_worker(pwq->pool);
> + if (force_kick || (wq->flags & WQ_UNBOUND))
> + wake_up_worker(pwq->pool);
> } else {
> pwq->max_active = 0;
> }
> @@ -3743,6 +3746,11 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
> raw_spin_unlock_irqrestore(&pwq->pool->lock, flags);
> }
>
> +static void pwq_adjust_max_active(struct pool_workqueue *pwq)
> +{
> + pwq_adjust_max_active_and_kick(pwq, false);
> +}
> +
> /* initialize newly alloced @pwq which is associated with @wq and @pool */
> static void init_pwq(struct pool_workqueue *pwq, struct workqueue_struct *wq,
> struct worker_pool *pool)
> @@ -5252,7 +5260,7 @@ void thaw_workqueues(void)
> list_for_each_entry(wq, &workqueues, list) {
> mutex_lock(&wq->mutex);
> for_each_pwq(pwq, wq)
> - pwq_adjust_max_active(pwq);
> + pwq_adjust_max_active_and_kick(pwq, true);
> mutex_unlock(&wq->mutex);
> }
>
> --
> 2.18.4
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: workqueue: Only kick a worker after thawed or for an unbound workqueue
2020-11-18 4:06 ` Lai Jiangshan
@ 2020-11-18 6:26 ` Yunfeng Ye
2020-11-18 9:05 ` Yunfeng Ye
0 siblings, 1 reply; 6+ messages in thread
From: Yunfeng Ye @ 2020-11-18 6:26 UTC (permalink / raw)
To: Lai Jiangshan; +Cc: Tejun Heo, LKML, Shiyuan Hu, Hewenliang
On 2020/11/18 12:06, Lai Jiangshan wrote:
> On Tue, Nov 17, 2020 at 3:33 PM Yunfeng Ye <yeyunfeng@huawei.com> wrote:
>>
>> In realtime scenario, We do not want to have interference on the
>> isolated cpu cores. but when invoking alloc_workqueue() for percpu wq
>> on the housekeeping cpu, it kick a kworker on the isolated cpu.
>>
>> alloc_workqueue
>> pwq_adjust_max_active
>> wake_up_worker
>>
>> The comment in pwq_adjust_max_active() said:
>> "Need to kick a worker after thawed or an unbound wq's
>> max_active is bumped"
>>
>> So it is unnecessary to kick a kworker for percpu wq's when
>> alloc_workqueue. this patch only kick a worker after thawed or for an
>> unbound workqueue.
>>
>> Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
>> ---
>> kernel/workqueue.c | 18 +++++++++++++-----
>> 1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index c41c3c17b86a..80f7bbd4889f 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -3696,14 +3696,16 @@ static void pwq_unbound_release_workfn(struct work_struct *work)
>> }
>>
>> /**
>> - * pwq_adjust_max_active - update a pwq's max_active to the current setting
>> + * pwq_adjust_max_active_and_kick - update a pwq's max_active to the current setting
>> * @pwq: target pool_workqueue
>> + * @force_kick: force to kick a worker
>> *
>> * If @pwq isn't freezing, set @pwq->max_active to the associated
>> * workqueue's saved_max_active and activate delayed work items
>> * accordingly. If @pwq is freezing, clear @pwq->max_active to zero.
>> */
>> -static void pwq_adjust_max_active(struct pool_workqueue *pwq)
>> +static void pwq_adjust_max_active_and_kick(struct pool_workqueue *pwq,
>> + bool force_kick)
>> {
>> struct workqueue_struct *wq = pwq->wq;
>> bool freezable = wq->flags & WQ_FREEZABLE;
>> @@ -3733,9 +3735,10 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
>>
>> /*
>> * Need to kick a worker after thawed or an unbound wq's
>> - * max_active is bumped. It's a slow path. Do it always.
>> + * max_active is bumped.
>
>
> Hello
>
> Thanks for reporting the problem.
>
> But I don't like to add an argument. The waking up is called
> always just because it was considered no harm and it is slow
> path. But it can still be possible to detect if the waking up
> is really needed based on the actual activation of delayed works.
>
> The previous lines are:
>
> while (!list_empty(&pwq->delayed_works) &&
> pwq->nr_active < pwq->max_active)
> pwq_activate_first_delayed(pwq);
>
> And you can record the old pwq->nr_active before these lines:
>
> int old_nr_active = pwq->nr_active;
>
> while (!list_empty(&pwq->delayed_works) &&
> pwq->nr_active < pwq->max_active)
> pwq_activate_first_delayed(pwq);
>
> /* please add more comments here, see 951a078a5 */
> if (old_nr_active < pwq->nr_active) {
> if (!old_nr_active || (wq->flags & WQ_UNBOUND))
> wake_up_worker(pwq->pool);
> }
>
Ok, I will send a patch v2.
Thanks.
>
> Thanks for your work.
> Lai.
>
>> */
>> - wake_up_worker(pwq->pool);
>> + if (force_kick || (wq->flags & WQ_UNBOUND))
>> + wake_up_worker(pwq->pool);
>> } else {
>> pwq->max_active = 0;
>> }
>> @@ -3743,6 +3746,11 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
>> raw_spin_unlock_irqrestore(&pwq->pool->lock, flags);
>> }
>>
>> +static void pwq_adjust_max_active(struct pool_workqueue *pwq)
>> +{
>> + pwq_adjust_max_active_and_kick(pwq, false);
>> +}
>> +
>> /* initialize newly alloced @pwq which is associated with @wq and @pool */
>> static void init_pwq(struct pool_workqueue *pwq, struct workqueue_struct *wq,
>> struct worker_pool *pool)
>> @@ -5252,7 +5260,7 @@ void thaw_workqueues(void)
>> list_for_each_entry(wq, &workqueues, list) {
>> mutex_lock(&wq->mutex);
>> for_each_pwq(pwq, wq)
>> - pwq_adjust_max_active(pwq);
>> + pwq_adjust_max_active_and_kick(pwq, true);
>> mutex_unlock(&wq->mutex);
>> }
>>
>> --
>> 2.18.4
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: workqueue: Only kick a worker after thawed or for an unbound workqueue
2020-11-18 6:26 ` Yunfeng Ye
@ 2020-11-18 9:05 ` Yunfeng Ye
2020-11-19 1:58 ` Lai Jiangshan
0 siblings, 1 reply; 6+ messages in thread
From: Yunfeng Ye @ 2020-11-18 9:05 UTC (permalink / raw)
To: Lai Jiangshan; +Cc: Tejun Heo, LKML, Shiyuan Hu, Hewenliang
On 2020/11/18 14:26, Yunfeng Ye wrote:
>
>
> On 2020/11/18 12:06, Lai Jiangshan wrote:
>> On Tue, Nov 17, 2020 at 3:33 PM Yunfeng Ye <yeyunfeng@huawei.com> wrote:
>>>
>>> In realtime scenario, We do not want to have interference on the
>>> isolated cpu cores. but when invoking alloc_workqueue() for percpu wq
>>> on the housekeeping cpu, it kick a kworker on the isolated cpu.
>>>
>>> alloc_workqueue
>>> pwq_adjust_max_active
>>> wake_up_worker
>>>
>>> The comment in pwq_adjust_max_active() said:
>>> "Need to kick a worker after thawed or an unbound wq's
>>> max_active is bumped"
>>>
>>> So it is unnecessary to kick a kworker for percpu wq's when
>>> alloc_workqueue. this patch only kick a worker after thawed or for an
>>> unbound workqueue.
>>>
>>> Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
>>> ---
>>> kernel/workqueue.c | 18 +++++++++++++-----
>>> 1 file changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>>> index c41c3c17b86a..80f7bbd4889f 100644
>>> --- a/kernel/workqueue.c
>>> +++ b/kernel/workqueue.c
>>> @@ -3696,14 +3696,16 @@ static void pwq_unbound_release_workfn(struct work_struct *work)
>>> }
>>>
>>> /**
>>> - * pwq_adjust_max_active - update a pwq's max_active to the current setting
>>> + * pwq_adjust_max_active_and_kick - update a pwq's max_active to the current setting
>>> * @pwq: target pool_workqueue
>>> + * @force_kick: force to kick a worker
>>> *
>>> * If @pwq isn't freezing, set @pwq->max_active to the associated
>>> * workqueue's saved_max_active and activate delayed work items
>>> * accordingly. If @pwq is freezing, clear @pwq->max_active to zero.
>>> */
>>> -static void pwq_adjust_max_active(struct pool_workqueue *pwq)
>>> +static void pwq_adjust_max_active_and_kick(struct pool_workqueue *pwq,
>>> + bool force_kick)
>>> {
>>> struct workqueue_struct *wq = pwq->wq;
>>> bool freezable = wq->flags & WQ_FREEZABLE;
>>> @@ -3733,9 +3735,10 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
>>>
>>> /*
>>> * Need to kick a worker after thawed or an unbound wq's
>>> - * max_active is bumped. It's a slow path. Do it always.
>>> + * max_active is bumped.
>>
>>
>> Hello
>>
>> Thanks for reporting the problem.
>>
>> But I don't like to add an argument. The waking up is called
>> always just because it was considered no harm and it is slow
>> path. But it can still be possible to detect if the waking up
>> is really needed based on the actual activation of delayed works.
>>
>> The previous lines are:
>>
>> while (!list_empty(&pwq->delayed_works) &&
>> pwq->nr_active < pwq->max_active)
>> pwq_activate_first_delayed(pwq);
>>
>> And you can record the old pwq->nr_active before these lines:
>>
>> int old_nr_active = pwq->nr_active;
>>
>> while (!list_empty(&pwq->delayed_works) &&
>> pwq->nr_active < pwq->max_active)
>> pwq_activate_first_delayed(pwq);
>>
>> /* please add more comments here, see 951a078a5 */
>> if (old_nr_active < pwq->nr_active) {
>> if (!old_nr_active || (wq->flags & WQ_UNBOUND))
>> wake_up_worker(pwq->pool);
>> }
>>
> Ok, I will send a patch v2.
> Thanks.
>
I think it is unnecessary to distinguish the percpu or unbound's wq,
kick a worker always based on the actual activation of delayed works.
Look like this:
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c41c3c17b86a..cd551dcb2cc9 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3725,17 +3725,23 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
* is updated and visible.
*/
if (!freezable || !workqueue_freezing) {
+ bool kick = false;
+
pwq->max_active = wq->saved_max_active;
while (!list_empty(&pwq->delayed_works) &&
- pwq->nr_active < pwq->max_active)
+ pwq->nr_active < pwq->max_active) {
pwq_activate_first_delayed(pwq);
+ kick = true;
+ }
/*
* Need to kick a worker after thawed or an unbound wq's
- * max_active is bumped. It's a slow path. Do it always.
+ * max_active is bumped. It's a slow path. Do it always
+ * based on the actual activation of delayed works.
*/
- wake_up_worker(pwq->pool);
+ if (kick)
+ wake_up_worker(pwq->pool);
} else {
pwq->max_active = 0;
}
Is it OK?
Thanks.
>>
>> Thanks for your work.
>> Lai.
>>
>>> */
>>> - wake_up_worker(pwq->pool);
>>> + if (force_kick || (wq->flags & WQ_UNBOUND))
>>> + wake_up_worker(pwq->pool);
>>> } else {
>>> pwq->max_active = 0;
>>> }
>>> @@ -3743,6 +3746,11 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
>>> raw_spin_unlock_irqrestore(&pwq->pool->lock, flags);
>>> }
>>>
>>> +static void pwq_adjust_max_active(struct pool_workqueue *pwq)
>>> +{
>>> + pwq_adjust_max_active_and_kick(pwq, false);
>>> +}
>>> +
>>> /* initialize newly alloced @pwq which is associated with @wq and @pool */
>>> static void init_pwq(struct pool_workqueue *pwq, struct workqueue_struct *wq,
>>> struct worker_pool *pool)
>>> @@ -5252,7 +5260,7 @@ void thaw_workqueues(void)
>>> list_for_each_entry(wq, &workqueues, list) {
>>> mutex_lock(&wq->mutex);
>>> for_each_pwq(pwq, wq)
>>> - pwq_adjust_max_active(pwq);
>>> + pwq_adjust_max_active_and_kick(pwq, true);
>>> mutex_unlock(&wq->mutex);
>>> }
>>>
>>> --
>>> 2.18.4
>> .
>>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: workqueue: Only kick a worker after thawed or for an unbound workqueue
2020-11-18 9:05 ` Yunfeng Ye
@ 2020-11-19 1:58 ` Lai Jiangshan
2020-11-19 2:25 ` Yunfeng Ye
0 siblings, 1 reply; 6+ messages in thread
From: Lai Jiangshan @ 2020-11-19 1:58 UTC (permalink / raw)
To: Yunfeng Ye; +Cc: Tejun Heo, LKML, Shiyuan Hu, Hewenliang
On Wed, Nov 18, 2020 at 5:05 PM Yunfeng Ye <yeyunfeng@huawei.com> wrote:
>
>
>
> On 2020/11/18 14:26, Yunfeng Ye wrote:
> >
> >
> > On 2020/11/18 12:06, Lai Jiangshan wrote:
> >> On Tue, Nov 17, 2020 at 3:33 PM Yunfeng Ye <yeyunfeng@huawei.com> wrote:
> >>>
> >>> In realtime scenario, We do not want to have interference on the
> >>> isolated cpu cores. but when invoking alloc_workqueue() for percpu wq
> >>> on the housekeeping cpu, it kick a kworker on the isolated cpu.
> >>>
> >>> alloc_workqueue
> >>> pwq_adjust_max_active
> >>> wake_up_worker
> >>>
> >>> The comment in pwq_adjust_max_active() said:
> >>> "Need to kick a worker after thawed or an unbound wq's
> >>> max_active is bumped"
> >>>
> >>> So it is unnecessary to kick a kworker for percpu wq's when
> >>> alloc_workqueue. this patch only kick a worker after thawed or for an
> >>> unbound workqueue.
> >>>
> >>> Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
> >>> ---
> >>> kernel/workqueue.c | 18 +++++++++++++-----
> >>> 1 file changed, 13 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> >>> index c41c3c17b86a..80f7bbd4889f 100644
> >>> --- a/kernel/workqueue.c
> >>> +++ b/kernel/workqueue.c
> >>> @@ -3696,14 +3696,16 @@ static void pwq_unbound_release_workfn(struct work_struct *work)
> >>> }
> >>>
> >>> /**
> >>> - * pwq_adjust_max_active - update a pwq's max_active to the current setting
> >>> + * pwq_adjust_max_active_and_kick - update a pwq's max_active to the current setting
> >>> * @pwq: target pool_workqueue
> >>> + * @force_kick: force to kick a worker
> >>> *
> >>> * If @pwq isn't freezing, set @pwq->max_active to the associated
> >>> * workqueue's saved_max_active and activate delayed work items
> >>> * accordingly. If @pwq is freezing, clear @pwq->max_active to zero.
> >>> */
> >>> -static void pwq_adjust_max_active(struct pool_workqueue *pwq)
> >>> +static void pwq_adjust_max_active_and_kick(struct pool_workqueue *pwq,
> >>> + bool force_kick)
> >>> {
> >>> struct workqueue_struct *wq = pwq->wq;
> >>> bool freezable = wq->flags & WQ_FREEZABLE;
> >>> @@ -3733,9 +3735,10 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
> >>>
> >>> /*
> >>> * Need to kick a worker after thawed or an unbound wq's
> >>> - * max_active is bumped. It's a slow path. Do it always.
> >>> + * max_active is bumped.
> >>
> >>
> >> Hello
> >>
> >> Thanks for reporting the problem.
> >>
> >> But I don't like to add an argument. The waking up is called
> >> always just because it was considered no harm and it is slow
> >> path. But it can still be possible to detect if the waking up
> >> is really needed based on the actual activation of delayed works.
> >>
> >> The previous lines are:
> >>
> >> while (!list_empty(&pwq->delayed_works) &&
> >> pwq->nr_active < pwq->max_active)
> >> pwq_activate_first_delayed(pwq);
> >>
> >> And you can record the old pwq->nr_active before these lines:
> >>
> >> int old_nr_active = pwq->nr_active;
> >>
> >> while (!list_empty(&pwq->delayed_works) &&
> >> pwq->nr_active < pwq->max_active)
> >> pwq_activate_first_delayed(pwq);
> >>
> >> /* please add more comments here, see 951a078a5 */
> >> if (old_nr_active < pwq->nr_active) {
> >> if (!old_nr_active || (wq->flags & WQ_UNBOUND))
> >> wake_up_worker(pwq->pool);
> >> }
> >>
> > Ok, I will send a patch v2.
> > Thanks.
> >
> I think it is unnecessary to distinguish the percpu or unbound's wq,
> kick a worker always based on the actual activation of delayed works.
>
> Look like this:
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index c41c3c17b86a..cd551dcb2cc9 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -3725,17 +3725,23 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
> * is updated and visible.
> */
> if (!freezable || !workqueue_freezing) {
> + bool kick = false;
> +
> pwq->max_active = wq->saved_max_active;
>
> while (!list_empty(&pwq->delayed_works) &&
> - pwq->nr_active < pwq->max_active)
> + pwq->nr_active < pwq->max_active) {
> pwq_activate_first_delayed(pwq);
> + kick = true;
> + }
>
> /*
> * Need to kick a worker after thawed or an unbound wq's
> - * max_active is bumped. It's a slow path. Do it always.
> + * max_active is bumped. It's a slow path. Do it always
> + * based on the actual activation of delayed works.
> */
> - wake_up_worker(pwq->pool);
> + if (kick)
> + wake_up_worker(pwq->pool);
> } else {
> pwq->max_active = 0;
> }
>
> Is it OK?
> Thanks.
It is OK, since it is a slow path. Please also add
comments to the code for reasons not to wake up in
some cases as described in your previous comments.
>
> >>
> >> Thanks for your work.
> >> Lai.
> >>
> >>> */
> >>> - wake_up_worker(pwq->pool);
> >>> + if (force_kick || (wq->flags & WQ_UNBOUND))
> >>> + wake_up_worker(pwq->pool);
> >>> } else {
> >>> pwq->max_active = 0;
> >>> }
> >>> @@ -3743,6 +3746,11 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
> >>> raw_spin_unlock_irqrestore(&pwq->pool->lock, flags);
> >>> }
> >>>
> >>> +static void pwq_adjust_max_active(struct pool_workqueue *pwq)
> >>> +{
> >>> + pwq_adjust_max_active_and_kick(pwq, false);
> >>> +}
> >>> +
> >>> /* initialize newly alloced @pwq which is associated with @wq and @pool */
> >>> static void init_pwq(struct pool_workqueue *pwq, struct workqueue_struct *wq,
> >>> struct worker_pool *pool)
> >>> @@ -5252,7 +5260,7 @@ void thaw_workqueues(void)
> >>> list_for_each_entry(wq, &workqueues, list) {
> >>> mutex_lock(&wq->mutex);
> >>> for_each_pwq(pwq, wq)
> >>> - pwq_adjust_max_active(pwq);
> >>> + pwq_adjust_max_active_and_kick(pwq, true);
> >>> mutex_unlock(&wq->mutex);
> >>> }
> >>>
> >>> --
> >>> 2.18.4
> >> .
> >>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: workqueue: Only kick a worker after thawed or for an unbound workqueue
2020-11-19 1:58 ` Lai Jiangshan
@ 2020-11-19 2:25 ` Yunfeng Ye
0 siblings, 0 replies; 6+ messages in thread
From: Yunfeng Ye @ 2020-11-19 2:25 UTC (permalink / raw)
To: Lai Jiangshan; +Cc: Tejun Heo, LKML, Shiyuan Hu, Hewenliang
On 2020/11/19 9:58, Lai Jiangshan wrote:
> On Wed, Nov 18, 2020 at 5:05 PM Yunfeng Ye <yeyunfeng@huawei.com> wrote:
>>
>>
>>
>> On 2020/11/18 14:26, Yunfeng Ye wrote:
>>>
>>>
>>> On 2020/11/18 12:06, Lai Jiangshan wrote:
>>>> On Tue, Nov 17, 2020 at 3:33 PM Yunfeng Ye <yeyunfeng@huawei.com> wrote:
>>>>>
>>>>> In realtime scenario, We do not want to have interference on the
>>>>> isolated cpu cores. but when invoking alloc_workqueue() for percpu wq
>>>>> on the housekeeping cpu, it kick a kworker on the isolated cpu.
>>>>>
>>>>> alloc_workqueue
>>>>> pwq_adjust_max_active
>>>>> wake_up_worker
>>>>>
>>>>> The comment in pwq_adjust_max_active() said:
>>>>> "Need to kick a worker after thawed or an unbound wq's
>>>>> max_active is bumped"
>>>>>
>>>>> So it is unnecessary to kick a kworker for percpu wq's when
>>>>> alloc_workqueue. this patch only kick a worker after thawed or for an
>>>>> unbound workqueue.
>>>>>
>>>>> Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
>>>>> ---
>>>>> kernel/workqueue.c | 18 +++++++++++++-----
>>>>> 1 file changed, 13 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>>>>> index c41c3c17b86a..80f7bbd4889f 100644
>>>>> --- a/kernel/workqueue.c
>>>>> +++ b/kernel/workqueue.c
>>>>> @@ -3696,14 +3696,16 @@ static void pwq_unbound_release_workfn(struct work_struct *work)
>>>>> }
>>>>>
>>>>> /**
>>>>> - * pwq_adjust_max_active - update a pwq's max_active to the current setting
>>>>> + * pwq_adjust_max_active_and_kick - update a pwq's max_active to the current setting
>>>>> * @pwq: target pool_workqueue
>>>>> + * @force_kick: force to kick a worker
>>>>> *
>>>>> * If @pwq isn't freezing, set @pwq->max_active to the associated
>>>>> * workqueue's saved_max_active and activate delayed work items
>>>>> * accordingly. If @pwq is freezing, clear @pwq->max_active to zero.
>>>>> */
>>>>> -static void pwq_adjust_max_active(struct pool_workqueue *pwq)
>>>>> +static void pwq_adjust_max_active_and_kick(struct pool_workqueue *pwq,
>>>>> + bool force_kick)
>>>>> {
>>>>> struct workqueue_struct *wq = pwq->wq;
>>>>> bool freezable = wq->flags & WQ_FREEZABLE;
>>>>> @@ -3733,9 +3735,10 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
>>>>>
>>>>> /*
>>>>> * Need to kick a worker after thawed or an unbound wq's
>>>>> - * max_active is bumped. It's a slow path. Do it always.
>>>>> + * max_active is bumped.
>>>>
>>>>
>>>> Hello
>>>>
>>>> Thanks for reporting the problem.
>>>>
>>>> But I don't like to add an argument. The waking up is called
>>>> always just because it was considered no harm and it is slow
>>>> path. But it can still be possible to detect if the waking up
>>>> is really needed based on the actual activation of delayed works.
>>>>
>>>> The previous lines are:
>>>>
>>>> while (!list_empty(&pwq->delayed_works) &&
>>>> pwq->nr_active < pwq->max_active)
>>>> pwq_activate_first_delayed(pwq);
>>>>
>>>> And you can record the old pwq->nr_active before these lines:
>>>>
>>>> int old_nr_active = pwq->nr_active;
>>>>
>>>> while (!list_empty(&pwq->delayed_works) &&
>>>> pwq->nr_active < pwq->max_active)
>>>> pwq_activate_first_delayed(pwq);
>>>>
>>>> /* please add more comments here, see 951a078a5 */
>>>> if (old_nr_active < pwq->nr_active) {
>>>> if (!old_nr_active || (wq->flags & WQ_UNBOUND))
>>>> wake_up_worker(pwq->pool);
>>>> }
>>>>
>>> Ok, I will send a patch v2.
>>> Thanks.
>>>
>> I think it is unnecessary to distinguish the percpu or unbound's wq,
>> kick a worker always based on the actual activation of delayed works.
>>
>> Look like this:
>>
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index c41c3c17b86a..cd551dcb2cc9 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -3725,17 +3725,23 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
>> * is updated and visible.
>> */
>> if (!freezable || !workqueue_freezing) {
>> + bool kick = false;
>> +
>> pwq->max_active = wq->saved_max_active;
>>
>> while (!list_empty(&pwq->delayed_works) &&
>> - pwq->nr_active < pwq->max_active)
>> + pwq->nr_active < pwq->max_active) {
>> pwq_activate_first_delayed(pwq);
>> + kick = true;
>> + }
>>
>> /*
>> * Need to kick a worker after thawed or an unbound wq's
>> - * max_active is bumped. It's a slow path. Do it always.
>> + * max_active is bumped. It's a slow path. Do it always
>> + * based on the actual activation of delayed works.
>> */
>> - wake_up_worker(pwq->pool);
>> + if (kick)
>> + wake_up_worker(pwq->pool);
>> } else {
>> pwq->max_active = 0;
>> }
>>
>> Is it OK?
>> Thanks.
>
>
> It is OK, since it is a slow path. Please also add
> comments to the code for reasons not to wake up in
> some cases as described in your previous comments.
>
Ok, thanks.
>>
>>>>
>>>> Thanks for your work.
>>>> Lai.
>>>>
>>>>> */
>>>>> - wake_up_worker(pwq->pool);
>>>>> + if (force_kick || (wq->flags & WQ_UNBOUND))
>>>>> + wake_up_worker(pwq->pool);
>>>>> } else {
>>>>> pwq->max_active = 0;
>>>>> }
>>>>> @@ -3743,6 +3746,11 @@ static void pwq_adjust_max_active(struct pool_workqueue *pwq)
>>>>> raw_spin_unlock_irqrestore(&pwq->pool->lock, flags);
>>>>> }
>>>>>
>>>>> +static void pwq_adjust_max_active(struct pool_workqueue *pwq)
>>>>> +{
>>>>> + pwq_adjust_max_active_and_kick(pwq, false);
>>>>> +}
>>>>> +
>>>>> /* initialize newly alloced @pwq which is associated with @wq and @pool */
>>>>> static void init_pwq(struct pool_workqueue *pwq, struct workqueue_struct *wq,
>>>>> struct worker_pool *pool)
>>>>> @@ -5252,7 +5260,7 @@ void thaw_workqueues(void)
>>>>> list_for_each_entry(wq, &workqueues, list) {
>>>>> mutex_lock(&wq->mutex);
>>>>> for_each_pwq(pwq, wq)
>>>>> - pwq_adjust_max_active(pwq);
>>>>> + pwq_adjust_max_active_and_kick(pwq, true);
>>>>> mutex_unlock(&wq->mutex);
>>>>> }
>>>>>
>>>>> --
>>>>> 2.18.4
>>>> .
>>>>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-19 2:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-17 7:32 workqueue: Only kick a worker after thawed or for an unbound workqueue Yunfeng Ye
2020-11-18 4:06 ` Lai Jiangshan
2020-11-18 6:26 ` Yunfeng Ye
2020-11-18 9:05 ` Yunfeng Ye
2020-11-19 1:58 ` Lai Jiangshan
2020-11-19 2:25 ` Yunfeng Ye
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®