Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: iad51458e:Fastmail
Date:   Tue, 11 Jul 2023 23:33:42 -0700
From:   Boqun Feng <boqun.feng@gmail.com>
To:     Alice Ryhl <aliceryhl@google.com>
Cc:     rust-for-linux@vger.kernel.org, Tejun Heo <tj@kernel.org>,
        Miguel Ojeda <ojeda@kernel.org>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Gary Guo <gary@garyguo.net>,
        =?iso-8859-1?Q?Bj=F6rn?= Roy Baron <bjorn3_gh@protonmail.com>,
        Benno Lossin <benno.lossin@proton.me>,
        linux-kernel@vger.kernel.org, patches@lists.linux.dev,
        Wedson Almeida Filho <walmeida@microsoft.com>
Subject: Re: [PATCH 3/9] workqueue: introduce `__INIT_WORK_WITH_KEY`
Message-ID: <ZK5JRhXsV7RHey9V@Boquns-Mac-mini.home>
References: <20230711093303.1433770-1-aliceryhl@google.com>
 <20230711093303.1433770-4-aliceryhl@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230711093303.1433770-4-aliceryhl@google.com>
Precedence: bulk

On Tue, Jul 11, 2023 at 09:32:57AM +0000, Alice Ryhl wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.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@gmail.com>
> Signed-off-by: Alex Gaynor <alex.gaynor@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> Acked-by: Tejun Heo <tj@kernel.org>
> Signed-off-by: Alice Ryhl <aliceryhl@google.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
>