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 <[email protected]>
---
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
On Tue, Nov 17, 2020 at 3:33 PM Yunfeng Ye <[email protected]> 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 <[email protected]>
> ---
> 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
On 2020/11/18 12:06, Lai Jiangshan wrote:
> On Tue, Nov 17, 2020 at 3:33 PM Yunfeng Ye <[email protected]> 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 <[email protected]>
>> ---
>> 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
> .
>
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 <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
>> .
>>
On Wed, Nov 18, 2020 at 5:05 PM Yunfeng Ye <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>> ---
> >>> 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
> >> .
> >>
On 2020/11/19 9:58, Lai Jiangshan wrote:
> On Wed, Nov 18, 2020 at 5:05 PM Yunfeng Ye <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> 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
>>>> .
>>>>
> .
>