Re: [PATCH 3/9] workqueue: introduce `__INIT_WORK_WITH_KEY`

[Boqun Feng]

On Tue, Jul 11, 2023 at 09:32:57AM +0000, Alice Ryhl wrote:
> From: Wedson Almeida Filho <walmeida@xxxxxxxxxxxxx>
> 
> A Rust helper (introduced in a later patch) needs to call
> `__INIT_WORK` with a passed key, rather than define one in place.
> 
> In order to do that, this moves the initialization code from
> the `__INIT_WORK` macro into a new `__INIT_WORK_WITH_KEY` macro
> which takes the key as an extra parameter.
> 
> Co-developed-by: Alex Gaynor <alex.gaynor@xxxxxxxxx>
> Signed-off-by: Alex Gaynor <alex.gaynor@xxxxxxxxx>
> Signed-off-by: Wedson Almeida Filho <walmeida@xxxxxxxxxxxxx>
> Co-developed-by: Miguel Ojeda <ojeda@xxxxxxxxxx>
> Signed-off-by: Miguel Ojeda <ojeda@xxxxxxxxxx>
> Acked-by: Tejun Heo <tj@xxxxxxxxxx>
> Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> ---
> Taken from [1]. Wedson's email updated at Wedson's request. Tejun's
> Acked-by taken from [2].
> 
> [1]: https://lore.kernel.org/rust-for-linux/20220802015052.10452-7-ojeda@xxxxxxxxxx/
> [2]: https://lore.kernel.org/rust-for-linux/Yvq3IfK4+C94AeE2@xxxxxxxxxxxxxxx/
> 
>  include/linux/workqueue.h | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index 3992c994787f..c91a84a4723d 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -221,24 +221,31 @@ static inline unsigned int work_static(struct work_struct *work) { return 0; }
>   * to generate better code.
>   */
>  #ifdef CONFIG_LOCKDEP
> +#define __INIT_WORK_WITH_KEY(_work, _func, _onstack, _key)		\
> +	do {								\
> +		__init_work((_work), _onstack);				\
> +		(_work)->data = (atomic_long_t) WORK_DATA_INIT();	\
> +		lockdep_init_map(&(_work)->lockdep_map, "(work_completion)"#_work, _key, 0); \

The lock class name is generated as static string via C macro
"(work_completion)"#_work, and since Rust side helper will wrap it as
a function, so all work items in Rust will have the same lock class name
i.e. "(work_completion)work". ;-) 

Those names are for lockdep report readers to locate which lock class
cause the problem, so it make senses to have different names for
different work item types. Maybe change the C side function as what I
suggested[1], and use `core::any::typename::<Self>()` as the name? (Self
is `Work` since it's called in `Work::new`).

[1]: https://lore.kernel.org/rust-for-linux/ZHoZuIz97JN1VSBf@boqun-archlinux/

Regards,
Boqun

> +		INIT_LIST_HEAD(&(_work)->entry);			\
> +		(_work)->func = (_func);				\
> +	} while (0)
> +
>  #define __INIT_WORK(_work, _func, _onstack)				\
>  	do {								\
>  		static struct lock_class_key __key;			\
> -									\
> -		__init_work((_work), _onstack);				\
> -		(_work)->data = (atomic_long_t) WORK_DATA_INIT();	\
> -		lockdep_init_map(&(_work)->lockdep_map, "(work_completion)"#_work, &__key, 0); \
> -		INIT_LIST_HEAD(&(_work)->entry);			\
> -		(_work)->func = (_func);				\
> +		__INIT_WORK_WITH_KEY(_work, _func, _onstack, &__key);	\
>  	} while (0)
>  #else
> +#define __INIT_WORK_WITH_KEY(_work, _func, _onstack, _key)		\
> +	do {								\
> +		__init_work((_work), _onstack);				\
> +		(_work)->data = (atomic_long_t) WORK_DATA_INIT();	\
> +		INIT_LIST_HEAD(&(_work)->entry);			\
> +		(_work)->func = (_func);				\
> +	} while (0)
> +
>  #define __INIT_WORK(_work, _func, _onstack)				\
> -	do {								\
> -		__init_work((_work), _onstack);				\
> -		(_work)->data = (atomic_long_t) WORK_DATA_INIT();	\
> -		INIT_LIST_HEAD(&(_work)->entry);			\
> -		(_work)->func = (_func);				\
> -	} while (0)
> +	__INIT_WORK_WITH_KEY(_work, _func, _onstack, NULL)
>  #endif
>  
>  #define INIT_WORK(_work, _func)						\
> -- 
> 2.41.0.255.g8b1d071c50-goog
>