linux-kernel - Re: [PATCH] workqueue: Fix kernel-doc comment of unplug_oldest

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: <ZcZSlBRELjUIVJ_B@slm.duckdns.org>
Date: Fri, 9 Feb 2024 06:28:04 -1000
From: Tejun Heo <tj@...nel.org>
To: Waiman Long <longman@...hat.com>
Cc: Lai Jiangshan <jiangshanlai@...il.com>, linux-kernel@...r.kernel.org,
	Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH] workqueue: Fix kernel-doc comment of unplug_oldest_pwq()

(cc'ing Jonathan and quoting whole body)

I'm not necessarily against the patch but at least from in-code
documentation POV the diagram being in the function comment seems better.
Jonathan, do you happen to know a better way to address this?

Thanks.

On Fri, Feb 09, 2024 at 09:58:50AM -0500, Waiman Long wrote:
> It turns out that it is not a good idea to put an ASCII diagram in the
> kernel-doc comment of unplug_oldest_pwq() as the tool puts out warnings
> about its format and will likely render it illegible anyway. Break the
> ASCII diagram out into its own comment block inside the function to
> avoid this problem.
> 
> Signed-off-by: Waiman Long <longman@...hat.com>
> ---
>  kernel/workqueue.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index cd2c6edc5c66..f622f535bc00 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -1790,25 +1790,29 @@ static bool pwq_activate_first_inactive(struct pool_workqueue *pwq, bool fill)
>   * unplug_oldest_pwq - restart an oldest plugged pool_workqueue
>   * @wq: workqueue_struct to be restarted
>   *
> - * pwq's are linked into wq->pwqs with the oldest first. For ordered
> - * workqueues, only the oldest pwq is unplugged, the others are plugged to
> - * suspend execution until the oldest one is drained. When this happens, the
> - * next oldest one (first plugged pwq in iteration) will be unplugged to
> - * restart work item execution to ensure proper work item ordering.
> - *
> - *    dfl_pwq --------------+     [P] - plugged
> - *                          |
> - *                          v
> - *    pwqs -> A -> B [P] -> C [P] (newest)
> - *            |    |        |
> - *            1    3        5
> - *            |    |        |
> - *            2    4        6
> + * This function should only be called for ordered workqueues where only the
> + * oldest pwq is unplugged, the others are plugged to suspend execution until
> + * the oldest one is drained and removed. When this happens, the next oldest
> + * one will be unplugged to restart work item execution to ensure proper work
> + * item ordering. Note that pwq's are linked into wq->pwqs with the oldest
> + * first, so the first one in the list is the oldest.
>   */
>  static void unplug_oldest_pwq(struct workqueue_struct *wq)
>  {
>  	struct pool_workqueue *pwq;
>  
> +	/*
> +	 * Layout of an ordered workqueue during a wq_unbound_cpumask update:
> +	 *
> +	 *    dfl_pwq --------------+     [P] - plugged
> +	 *                          |
> +	 *                          v
> +	 *    pwqs -> A -> B [P] -> C [P] (newest)
> +	 *            |    |        |
> +	 *            1    3        5
> +	 *            |    |        |
> +	 *            2    4        6
> +	 */
>  	lockdep_assert_held(&wq->mutex);
>  
>  	/* Caller should make sure that pwqs isn't empty before calling */
> -- 
> 2.39.3
> 

-- 
tejun