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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJhGHyBCs17Q=iQBfTJ-4Ls7egpa_70aEx0Ym_-oCt2vXKkSOg@mail.gmail.com>
Date:   Wed, 18 Nov 2020 12:06:50 +0800
From:   Lai Jiangshan <jiangshanlai@...il.com>
To:     Yunfeng Ye <yeyunfeng@...wei.com>
Cc:     Tejun Heo <tj@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
        Shiyuan Hu <hushiyuan@...wei.com>,
        Hewenliang <hewenliang4@...wei.com>
Subject: Re: workqueue: Only kick a worker after thawed or for an unbound workqueue

On Tue, Nov 17, 2020 at 3:33 PM Yunfeng Ye <yeyunfeng@...wei.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@...wei.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