Re: [PATCH v3 3/5] workqueue: Add interface for user-defined workqueue lockdep map

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 10-08-2024 03:58, Matthew Brost wrote:
> Add an interface for a user-defined workqueue lockdep map, which is
> helpful when multiple workqueues are created for the same purpose. This
> also helps avoid leaking lockdep maps on each workqueue creation.
> 
> v2:
>   - Add alloc_workqueue_lockdep_map (Tejun)
> v3:
>   - Drop __WQ_USER_OWNED_LOCKDEP (Tejun)
>   - static inline alloc_ordered_workqueue_lockdep_map (Tejun)
> 
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>   include/linux/workqueue.h | 52 +++++++++++++++++++++++++++++++++++++++
>   kernel/workqueue.c        | 28 +++++++++++++++++++++
>   2 files changed, 80 insertions(+)
> 
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index 4eb8f9563136..8ccbf510880b 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -507,6 +507,58 @@ void workqueue_softirq_dead(unsigned int cpu);
>   __printf(1, 4) struct workqueue_struct *
>   alloc_workqueue(const char *fmt, unsigned int flags, int max_active, ...);
>   
> +#ifdef CONFIG_LOCKDEP
> +/**
> + * alloc_workqueue_lockdep_map - allocate a workqueue with user-defined lockdep_map
> + * @fmt: printf format for the name of the workqueue
> + * @flags: WQ_* flags
> + * @max_active: max in-flight work items, 0 for default
> + * @lockdep_map: user-defined lockdep_map
> + * @...: args for @fmt
> + *
> + * Same as alloc_workqueue but with the a user-define lockdep_map. Useful for
> + * workqueues created with the same purpose and to avoid leaking a lockdep_map
> + * on each workqueue creation.
> + *
> + * RETURNS:
> + * Pointer to the allocated workqueue on success, %NULL on failure.
> + */
> +__printf(1, 5) struct workqueue_struct *
> +alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
> +			    struct lockdep_map *lockdep_map, ...);
> +
> +/**
> + * alloc_ordered_workqueue_lockdep_map - allocate an ordered workqueue with
> + * user-defined lockdep_map
> + *
> + * @fmt: printf format for the name of the workqueue
> + * @flags: WQ_* flags (only WQ_FREEZABLE and WQ_MEM_RECLAIM are meaningful)
> + * @lockdep_map: user-defined lockdep_map
> + * @args: args for @fmt
> + *
> + * Same as alloc_ordered_workqueue but with the a user-define lockdep_map.
> + * Useful for workqueues created with the same purpose and to avoid leaking a
> + * lockdep_map on each workqueue creation.
> + *
> + * RETURNS:
> + * Pointer to the allocated workqueue on success, %NULL on failure.
> + */
> +__printf(1, 4) static inline struct workqueue_struct *
> +alloc_ordered_workqueue_lockdep_map(const char *fmt, unsigned int flags,
> +				    struct lockdep_map *lockdep_map, ...)
> +{
> +	struct workqueue_struct *wq;
> +	va_list args;
> +
> +	va_start(args, lockdep_map);
> +	wq = alloc_workqueue_lockdep_map(fmt, WQ_UNBOUND | __WQ_ORDERED | flags,
> +					 1, lockdep_map, args);
> +	va_end(args);
> +
> +	return wq;
> +}
> +#endif
> +
>   /**
>    * alloc_ordered_workqueue - allocate an ordered workqueue
>    * @fmt: printf format for the name of the workqueue
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 24df85589dc0..f4b50a995e99 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -4775,11 +4775,17 @@ static void wq_init_lockdep(struct workqueue_struct *wq)
>   
>   static void wq_unregister_lockdep(struct workqueue_struct *wq)
>   {
> +	if (wq->lockdep_map != &wq->__lockdep_map)
> +		return;
> +
>   	lockdep_unregister_key(&wq->key);
>   }
>   
>   static void wq_free_lockdep(struct workqueue_struct *wq)
>   {
> +	if (wq->lockdep_map != &wq->__lockdep_map)
> +		return;
> +
>   	if (wq->lock_name != wq->name)
>   		kfree(wq->lock_name);
>   }
> @@ -5756,6 +5762,28 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
>   }
>   EXPORT_SYMBOL_GPL(alloc_workqueue);
>   
> +#ifdef CONFIG_LOCKDEP
> +__printf(1, 5)
> +struct workqueue_struct *
> +alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags,
> +			    int max_active, struct lockdep_map *lockdep_map, ...)
> +{
> +	struct workqueue_struct *wq;
> +	va_list args;
> +
> +	va_start(args, lockdep_map);
> +	wq = __alloc_workqueue(fmt, flags, max_active, args);
> +	va_end(args);
> +	if (!wq)
> +		return NULL;
> +
> +	wq->lockdep_map = lockdep_map;

How about NULL check and fallback to dynamic allocation, just to be safe   ?

> +
> +	return wq;
> +}
> +EXPORT_SYMBOL_GPL(alloc_workqueue_lockdep_map);
> +#endif
> +
>   static bool pwq_busy(struct pool_workqueue *pwq)
>   {
>   	int i;