Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: workqueue: Only kick a worker after thawed or for an unbound
 workqueue
To:     Lai Jiangshan <jiangshanlai@gmail.com>
CC:     Tejun Heo <tj@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
        Shiyuan Hu <hushiyuan@huawei.com>,
        Hewenliang <hewenliang4@huawei.com>
References: <aaa87d62-b89d-e241-1a6e-01176694616f@huawei.com>
 <CAJhGHyBCs17Q=iQBfTJ-4Ls7egpa_70aEx0Ym_-oCt2vXKkSOg@mail.gmail.com>
From:   Yunfeng Ye <yeyunfeng@huawei.com>
Message-ID: <6e174c9f-5436-7d1c-0443-3ca21ff8dad7@huawei.com>
Date:   Wed, 18 Nov 2020 14:26:49 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <CAJhGHyBCs17Q=iQBfTJ-4Ls7egpa_70aEx0Ym_-oCt2vXKkSOg@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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
> .
>