Received-SPF: pass (google.com: domain of linux-kernel+bounces-86047-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240228181850.5895-1-apais@linux.microsoft.com>
In-Reply-To: <20240228181850.5895-1-apais@linux.microsoft.com>
From: Lai Jiangshan <jiangshanlai@gmail.com>
Date: Thu, 29 Feb 2024 10:30:35 +0800
Message-ID: <CAJhGHyD8cruwAkr+hqysv7fTQX1ovpn2cu_wU2C4TXrE5Qy8zg@mail.gmail.com>
Subject: Re: [PATCH] kernel: Introduce enable_and_queue_work() convenience function
To: Allen Pais <apais@linux.microsoft.com>
Cc: tj@kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 29, 2024 at 2:18=E2=80=AFAM Allen Pais <apais@linux.microsoft.c=
om> wrote:
>
> 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(). 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.
>
> 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.
>
> Signed-off-by: Allen Pais <allen.lkml@gmail.com>
> ---
>  include/linux/workqueue.h | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index aedfb81f9c49..31bbd38ef8c8 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -678,6 +678,29 @@ 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 wo=
rkqueue
> + * @wq: The target workqueue
> + * @work: The work item to be enabled and queued
> + *
> + * This function attempts to enable the specified work item using enable=
_work().
> + * If the enable operation is successful, the work item is then queued o=
n the

Could you please specify what "successful" means and also please
document it that it might cause unnecessary spurious wake-ups and
the caller should prepare for it if it is not desired.

Thanks
Lai

PS:

I'm afraid it can cause unnecessary spurious wake-ups in cases
where the work item is expected to be dormant ordinarily but disable/enable=
()
are called often for maintenance.  However, the user can resort to other
synchronization in this case rather than just disable/enable() only to avoi=
d the
wake-ups overheads.


> + * provided workqueue using queue_work(). It returns %true if the work i=
tem was
> + * successfully enabled and queued, and %false otherwise.
> + *
> + * This function combines the operations of enable_work() and queue_work=
(),
> + * providing a convenient way to enable and queue a work item in a singl=
e call.
> + */
> +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 p=
ossible.
>   * Warn attempt to flush system-wide workqueues at runtime.
> --
> 2.17.1
>