Received-SPF: pass (google.com: domain of linux-kernel+bounces-59604-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Sender: Tejun Heo <htejun@gmail.com>
Date: Fri, 9 Feb 2024 06:28:04 -1000
From: Tejun Heo <tj@kernel.org>
To: Waiman Long <longman@redhat.com>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>, linux-kernel@vger.kernel.org,
	Jonathan Corbet <corbet@lwn.net>
Subject: Re: [PATCH] workqueue: Fix kernel-doc comment of unplug_oldest_pwq()
Message-ID: <ZcZSlBRELjUIVJ_B@slm.duckdns.org>
References: <20240209145850.1157304-1-longman@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240209145850.1157304-1-longman@redhat.com>

(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@redhat.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