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

Powered by blists - more mailing lists