Received-SPF: pass (google.com: domain of linux-kernel+bounces-117848-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240325180201.6027-1-apais@linux.microsoft.com> <ZgHTvwIerkcRem2s@slm.duckdns.org>
In-Reply-To: <ZgHTvwIerkcRem2s@slm.duckdns.org>
From: Allen <allen.lkml@gmail.com>
Date: Mon, 25 Mar 2024 12:46:53 -0700
Message-ID: <CAOMdWSLD8McswfEmn_B74F9EejTW8Zi_XAmakA_=19GDgnMRRg@mail.gmail.com>
Subject: Re: [PATCH v2] kernel: Introduce enable_and_queue_work() convenience function
To: Tejun Heo <tj@kernel.org>
Cc: Allen Pais <apais@linux.microsoft.com>, jiangshanlai@gmail.com, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

> The function comment felt a bit too verbose. I applied the following
> compacted version to wq/for-6.10 which hopefully still contains all the
> necessary information. Please let me know if you disagree.
>

 Looks fine to me. Thanks for fixing it.

- Allen

> Thanks.
>
> --- 8< ----
> From 474a549ff4c989427a14fdab851e562c8a63fe24 Mon Sep 17 00:00:00 2001
> From: Allen Pais <apais@linux.microsoft.com>
> Date: Mon, 25 Mar 2024 18:02:01 +0000
> Subject: [PATCH] workqueue: Introduce enable_and_queue_work() convenience
>  function
>
> The enable_and_queue_work() function is introduced to streamline
> the process of enabling and queuing a work item on a specific
> workqueue. This function combines the functionalities of
> enable_work() and queue_work() in a single call, providing a
> concise and convenient API for enabling and queuing work items.
>
> The function accepts a target workqueue and a work item as parameters.
> It first attempts to enable the work item using enable_work(). A successful
> enable operation means that the work item was previously disabled
> and is now marked as eligible for execution. If the enable operation
> is successful, the work item is then queued on the specified workqueue
> using queue_work(). The function returns true if the work item was
> successfully enabled and queued, and false otherwise.
>
> Note: This function may lead to unnecessary spurious wake-ups in cases
> where the work item is expected to be dormant but enable/disable are called
> frequently. Spurious wake-ups refer to the condition where worker threads
> are woken up without actual work to be done. Callers should be aware of
> this behavior and may need to employ additional synchronization mechanisms
> to avoid these overheads if such wake-ups are not desired.
>
> This addition aims to enhance code readability and maintainability by
> providing a unified interface for the common use case of enabling and
> queuing work items on a workqueue.
>
> tj: Made the function comment more compact.
>
> Signed-off-by: Allen Pais <allen.lkml@gmail.com>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  include/linux/workqueue.h | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index bfcf8d38f4b1..2df1188c0f96 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -682,6 +682,32 @@ static inline bool schedule_work(struct work_struct *work)
>         return queue_work(system_wq, work);
>  }
>
> +/**
> + * enable_and_queue_work - Enable and queue a work item on a specific workqueue
> + * @wq: The target workqueue
> + * @work: The work item to be enabled and queued
> + *
> + * This function combines the operations of enable_work() and queue_work(),
> + * providing a convenient way to enable and queue a work item in a single call.
> + * It invokes enable_work() on @work and then queues it if the disable depth
> + * reached 0. Returns %true if the disable depth reached 0 and @work is queued,
> + * and %false otherwise.
> + *
> + * Note that @work is always queued when disable depth reaches zero. If the
> + * desired behavior is queueing only if certain events took place while @work is
> + * disabled, the user should implement the necessary state tracking and perform
> + * explicit conditional queueing after enable_work().
> + */
> +static inline bool enable_and_queue_work(struct workqueue_struct *wq,
> +                                        struct work_struct *work)
> +{
> +       if (enable_work(work)) {
> +               queue_work(wq, work);
> +               return true;
> +       }
> +       return false;
> +}
> +
>  /*
>   * Detect attempt to flush system-wide workqueues at compile time when possible.
>   * Warn attempt to flush system-wide workqueues at runtime.
> --
> 2.44.0
>
>


-- 
       - Allen