Re: workqueue: Only kick a worker after thawed or for an unbound workqueue

From: Lai Jiangshan
Date: Wed Nov 18 2020 - 20:58:57 EST

On Wed, Nov 18, 2020 at 5:05 PM Yunfeng Ye <yeyunfeng@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>> ---
> >>> 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
> >> .
> >>