[PATCH] workqueue: Fix kernel-doc comment of unplug_oldest_pwq()

[PATCH] workqueue: Fix kernel-doc comment of unplug_oldest_pwq()

[Waiman Long]

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

Re: [PATCH] workqueue: Fix kernel-doc comment of unplug_oldest_pwq()

[Tejun Heo]

(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

Re: [PATCH] workqueue: Fix kernel-doc comment of unplug_oldest_pwq()

[Jonathan Corbet]

Tejun Heo <tj@kernel.org> writes:

> (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?

So I went to reproduce this problem, but it seems that it's hidden away
in some branch and not in linux-next.  So I'll have to guess without
testing my solution, but...

> 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

The problem here is that you have a literal block without marking it as
such.  If you were to format it as:

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

...it should work.  The changes are the "::" after "ordering" and the
blank line at the end of the block.

Thanks,

jon

Re: [PATCH] workqueue: Fix kernel-doc comment of unplug_oldest_pwq()

[Waiman Long]

On 2/9/24 11:36, Jonathan Corbet wrote:
> Tejun Heo <tj@kernel.org> writes:
>
>> (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?
> So I went to reproduce this problem, but it seems that it's hidden away
> in some branch and not in linux-next.  So I'll have to guess without
> testing my solution, but...
>
>> 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
> The problem here is that you have a literal block without marking it as
> such.  If you were to format it as:
>
>> * 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
> ...it should work.  The changes are the "::" after "ordering" and the
> blank line at the end of the block.
>
Thanks for the tip. I will update my patch to use the proper formatting 
hint.

Cheers,
Longman