linux-kernel - Re: [PATCH 3/9] workqueue: introduce `__INIT_WORK_WITH

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZK5JRhXsV7RHey9V@Boquns-Mac-mini.home>
Date:   Tue, 11 Jul 2023 23:33:42 -0700
From:   Boqun Feng <boqun.feng@...il.com>
To:     Alice Ryhl <aliceryhl@...gle.com>
Cc:     rust-for-linux@...r.kernel.org, Tejun Heo <tj@...nel.org>,
        Miguel Ojeda <ojeda@...nel.org>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        Benno Lossin <benno.lossin@...ton.me>,
        linux-kernel@...r.kernel.org, patches@...ts.linux.dev,
        Wedson Almeida Filho <walmeida@...rosoft.com>
Subject: Re: [PATCH 3/9] workqueue: introduce `__INIT_WORK_WITH_KEY`

On Tue, Jul 11, 2023 at 09:32:57AM +0000, Alice Ryhl wrote:
> From: Wedson Almeida Filho <walmeida@...rosoft.com>
> 
> 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@...il.com>
> Signed-off-by: Alex Gaynor <alex.gaynor@...il.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@...rosoft.com>
> Co-developed-by: Miguel Ojeda <ojeda@...nel.org>
> Signed-off-by: Miguel Ojeda <ojeda@...nel.org>
> Acked-by: Tejun Heo <tj@...nel.org>
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> ---
> 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@kernel.org/
> [2]: https://lore.kernel.org/rust-for-linux/Yvq3IfK4+C94AeE2@slm.duckdns.org/
> 
>  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
>