[v2] rust: Fix build error

[PATCH v2] rust: Fix build error

Posted by Eder Zulian 2 weeks, 3 days ago

On a PREEMPT_RT build, spin locks have been mapped to rt_mutex types, so
avoid the raw_spinlock_init call for RT.

When CONFIG_DEBUG_SPINLOCK=y and CONFIG_PREEMPT_RT=y the following build
error occurs:

https://lore.kernel.org/oe-kbuild-all/202409251238.vetlgXE9-lkp@intel.com/

Fixes: 876346536c1b ("rust: kbuild: split up helpers.c")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202409251238.vetlgXE9-lkp@intel.com/
Signed-off-by: Eder Zulian <ezulian@redhat.com>
---
V1 -> V2: Cleaned up style and addressed review comments
 include/linux/spinlock_rt.h | 15 +++++++--------
 rust/helpers/spinlock.c     |  8 ++++++--
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/include/linux/spinlock_rt.h b/include/linux/spinlock_rt.h
index f9f14e135be7..f6499c37157d 100644
--- a/include/linux/spinlock_rt.h
+++ b/include/linux/spinlock_rt.h
@@ -16,22 +16,21 @@ static inline void __rt_spin_lock_init(spinlock_t *lock, const char *name,
 }
 #endif
 
-#define spin_lock_init(slock)					\
+#define __spin_lock_init(slock, name, key, percpu)		\
 do {								\
-	static struct lock_class_key __key;			\
-								\
 	rt_mutex_base_init(&(slock)->lock);			\
-	__rt_spin_lock_init(slock, #slock, &__key, false);	\
+	__rt_spin_lock_init(slock, name, key, percpu);		\
 } while (0)
 
-#define local_spin_lock_init(slock)				\
+#define _spin_lock_init(slock, percpu)				\
 do {								\
 	static struct lock_class_key __key;			\
-								\
-	rt_mutex_base_init(&(slock)->lock);			\
-	__rt_spin_lock_init(slock, #slock, &__key, true);	\
+	__spin_lock_init(slock, #slock, &__key, percpu);	\
 } while (0)
 
+#define spin_lock_init(slock)		_spin_lock_init(slock, false)
+#define local_spin_lock_init(slock)	_spin_lock_init(slock, true)
+
 extern void rt_spin_lock(spinlock_t *lock) __acquires(lock);
 extern void rt_spin_lock_nested(spinlock_t *lock, int subclass)	__acquires(lock);
 extern void rt_spin_lock_nest_lock(spinlock_t *lock, struct lockdep_map *nest_lock) __acquires(lock);
diff --git a/rust/helpers/spinlock.c b/rust/helpers/spinlock.c
index b7b0945e8b3c..5971fdf6f755 100644
--- a/rust/helpers/spinlock.c
+++ b/rust/helpers/spinlock.c
@@ -6,10 +6,14 @@ void rust_helper___spin_lock_init(spinlock_t *lock, const char *name,
 				  struct lock_class_key *key)
 {
 #ifdef CONFIG_DEBUG_SPINLOCK
+# if defined(CONFIG_PREEMPT_RT)
+	__spin_lock_init(lock, name, key, false);
+# else /*!CONFIG_PREEMPT_RT */
 	__raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
-#else
+# endif /* CONFIG_PREEMPT_RT */
+#else /* !CONFIG_DEBUG_SPINLOCK */
 	spin_lock_init(lock);
-#endif
+#endif /* CONFIG_DEBUG_SPINLOCK */
 }
 
 void rust_helper_spin_lock(spinlock_t *lock)
-- 
2.47.0

Re: [PATCH v2] rust: Fix build error

Posted by Boqun Feng 2 weeks, 3 days ago

Hi Eder,

Seems I forgot to reply you on your reply to v1, sorry about that.

For the commit title, I think it better be:

	rust: helpers: Avoid raw_spin_lock initialization for PREEMPT_RT

, because in general, title of the commit should be as specific as
possible (otherwise, half year later there could be 10 commits titled
"rust: Fix build error").

On Wed, Nov 06, 2024 at 10:12:15PM +0100, Eder Zulian wrote:
> On a PREEMPT_RT build, spin locks have been mapped to rt_mutex types, so
> avoid the raw_spinlock_init call for RT.
> 
> When CONFIG_DEBUG_SPINLOCK=y and CONFIG_PREEMPT_RT=y the following build
> error occurs:
> 
> https://lore.kernel.org/oe-kbuild-all/202409251238.vetlgXE9-lkp@intel.com/
> 

Since you already use the "Closes" tag to refer the bug report, let's
avoid links showing twice, how rephrase the above three paragraphs as:

"""
When PREEMPT_RT=y, spin locks are mapped to rt_mutex types, so using
spinlock_check() + __raw_spin_lock_init() to initialize spin locks is
incorrect, and would cause build errors.

Introduce __spin_lock_init() to initialize a spin lock with lockdep
rquired information for PREEMPT_RT builds, and use it in the Rust
helper.
"""

Thoughts?

> Fixes: 876346536c1b ("rust: kbuild: split up helpers.c")

I'm not sure this is the correct "Fixes" tag, that commit is a code
move, so it's unlikely introducing issue itself. Moreover, we really
need PREEMPT_RT being able to trigger the issue, so I think the correct
"Fixes" tag is:

Fixes: d2d6422f8bd1 ("x86: Allow to enable PREEMPT_RT.")

(yes, I know PREEMPT_RT is a long existing project, but it was until
that commit, you can build a kernel with PREEMPT_RT=y IIUC)

This will help stable maintainers for backport decisions.


The rest of patch looks good to me (we could maybe provide a
__spin_lock_init() for !RT build as well, but that's more of a
cleanup)

Regards,
Boqun

> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202409251238.vetlgXE9-lkp@intel.com/
> Signed-off-by: Eder Zulian <ezulian@redhat.com>
> ---
> V1 -> V2: Cleaned up style and addressed review comments
>  include/linux/spinlock_rt.h | 15 +++++++--------
>  rust/helpers/spinlock.c     |  8 ++++++--
>  2 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/spinlock_rt.h b/include/linux/spinlock_rt.h
> index f9f14e135be7..f6499c37157d 100644
> --- a/include/linux/spinlock_rt.h
> +++ b/include/linux/spinlock_rt.h
> @@ -16,22 +16,21 @@ static inline void __rt_spin_lock_init(spinlock_t *lock, const char *name,
>  }
>  #endif
>  
> -#define spin_lock_init(slock)					\
> +#define __spin_lock_init(slock, name, key, percpu)		\
>  do {								\
> -	static struct lock_class_key __key;			\
> -								\
>  	rt_mutex_base_init(&(slock)->lock);			\
> -	__rt_spin_lock_init(slock, #slock, &__key, false);	\
> +	__rt_spin_lock_init(slock, name, key, percpu);		\
>  } while (0)
>  
> -#define local_spin_lock_init(slock)				\
> +#define _spin_lock_init(slock, percpu)				\
>  do {								\
>  	static struct lock_class_key __key;			\
> -								\
> -	rt_mutex_base_init(&(slock)->lock);			\
> -	__rt_spin_lock_init(slock, #slock, &__key, true);	\
> +	__spin_lock_init(slock, #slock, &__key, percpu);	\
>  } while (0)
>  
> +#define spin_lock_init(slock)		_spin_lock_init(slock, false)
> +#define local_spin_lock_init(slock)	_spin_lock_init(slock, true)
> +
>  extern void rt_spin_lock(spinlock_t *lock) __acquires(lock);
>  extern void rt_spin_lock_nested(spinlock_t *lock, int subclass)	__acquires(lock);
>  extern void rt_spin_lock_nest_lock(spinlock_t *lock, struct lockdep_map *nest_lock) __acquires(lock);
> diff --git a/rust/helpers/spinlock.c b/rust/helpers/spinlock.c
> index b7b0945e8b3c..5971fdf6f755 100644
> --- a/rust/helpers/spinlock.c
> +++ b/rust/helpers/spinlock.c
> @@ -6,10 +6,14 @@ void rust_helper___spin_lock_init(spinlock_t *lock, const char *name,
>  				  struct lock_class_key *key)
>  {
>  #ifdef CONFIG_DEBUG_SPINLOCK
> +# if defined(CONFIG_PREEMPT_RT)
> +	__spin_lock_init(lock, name, key, false);
> +# else /*!CONFIG_PREEMPT_RT */
>  	__raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
> -#else
> +# endif /* CONFIG_PREEMPT_RT */
> +#else /* !CONFIG_DEBUG_SPINLOCK */
>  	spin_lock_init(lock);
> -#endif
> +#endif /* CONFIG_DEBUG_SPINLOCK */
>  }
>  
>  void rust_helper_spin_lock(spinlock_t *lock)
> -- 
> 2.47.0
> 
>

Re: [PATCH v2] rust: Fix build error

Posted by Eder Zulian 2 weeks, 2 days ago

Hi Boqun,

On Wed, Nov 06, 2024 at 03:24:42PM -0800, Boqun Feng wrote:
> Hi Eder,
> 
> Seems I forgot to reply you on your reply to v1, sorry about that.
> 
> For the commit title, I think it better be:
> 
> 	rust: helpers: Avoid raw_spin_lock initialization for PREEMPT_RT
> 

Sure, I will fix it. Much better indeed.

> , because in general, title of the commit should be as specific as
> possible (otherwise, half year later there could be 10 commits titled
> "rust: Fix build error").
> 
> On Wed, Nov 06, 2024 at 10:12:15PM +0100, Eder Zulian wrote:
> > On a PREEMPT_RT build, spin locks have been mapped to rt_mutex types, so
> > avoid the raw_spinlock_init call for RT.
> > 
> > When CONFIG_DEBUG_SPINLOCK=y and CONFIG_PREEMPT_RT=y the following build
> > error occurs:
> > 
> > https://lore.kernel.org/oe-kbuild-all/202409251238.vetlgXE9-lkp@intel.com/
> > 
> 
> Since you already use the "Closes" tag to refer the bug report, let's
> avoid links showing twice, how rephrase the above three paragraphs as:
> 
> """
> When PREEMPT_RT=y, spin locks are mapped to rt_mutex types, so using
> spinlock_check() + __raw_spin_lock_init() to initialize spin locks is
> incorrect, and would cause build errors.
> 
> Introduce __spin_lock_init() to initialize a spin lock with lockdep
> rquired information for PREEMPT_RT builds, and use it in the Rust
> helper.
> """
> 
> Thoughts?
> 

Makes sense. Will do.

> > Fixes: 876346536c1b ("rust: kbuild: split up helpers.c")
> 
> I'm not sure this is the correct "Fixes" tag, that commit is a code
> move, so it's unlikely introducing issue itself. Moreover, we really
> need PREEMPT_RT being able to trigger the issue, so I think the correct

One may argue that we need 'RUST=y' in order to trigger the issue.

> "Fixes" tag is:
> 
> Fixes: d2d6422f8bd1 ("x86: Allow to enable PREEMPT_RT.")
> 
> (yes, I know PREEMPT_RT is a long existing project, but it was until
> that commit, you can build a kernel with PREEMPT_RT=y IIUC)
> 
> This will help stable maintainers for backport decisions.
> 

Perhaps omitting the 'Fixes:' tag would be a solution. Is that an option?

> 
> The rest of patch looks good to me (we could maybe provide a
> __spin_lock_init() for !RT build as well, but that's more of a
> cleanup)
> 
> Regards,
> Boqun
> 

Thanks,
Eder

> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202409251238.vetlgXE9-lkp@intel.com/
> > Signed-off-by: Eder Zulian <ezulian@redhat.com>
> > ---
> > V1 -> V2: Cleaned up style and addressed review comments
> >  include/linux/spinlock_rt.h | 15 +++++++--------
> >  rust/helpers/spinlock.c     |  8 ++++++--
> >  2 files changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/linux/spinlock_rt.h b/include/linux/spinlock_rt.h
> > index f9f14e135be7..f6499c37157d 100644
> > --- a/include/linux/spinlock_rt.h
> > +++ b/include/linux/spinlock_rt.h
> > @@ -16,22 +16,21 @@ static inline void __rt_spin_lock_init(spinlock_t *lock, const char *name,
> >  }
> >  #endif
> >  
> > -#define spin_lock_init(slock)					\
> > +#define __spin_lock_init(slock, name, key, percpu)		\
> >  do {								\
> > -	static struct lock_class_key __key;			\
> > -								\
> >  	rt_mutex_base_init(&(slock)->lock);			\
> > -	__rt_spin_lock_init(slock, #slock, &__key, false);	\
> > +	__rt_spin_lock_init(slock, name, key, percpu);		\
> >  } while (0)
> >  
> > -#define local_spin_lock_init(slock)				\
> > +#define _spin_lock_init(slock, percpu)				\
> >  do {								\
> >  	static struct lock_class_key __key;			\
> > -								\
> > -	rt_mutex_base_init(&(slock)->lock);			\
> > -	__rt_spin_lock_init(slock, #slock, &__key, true);	\
> > +	__spin_lock_init(slock, #slock, &__key, percpu);	\
> >  } while (0)
> >  
> > +#define spin_lock_init(slock)		_spin_lock_init(slock, false)
> > +#define local_spin_lock_init(slock)	_spin_lock_init(slock, true)
> > +
> >  extern void rt_spin_lock(spinlock_t *lock) __acquires(lock);
> >  extern void rt_spin_lock_nested(spinlock_t *lock, int subclass)	__acquires(lock);
> >  extern void rt_spin_lock_nest_lock(spinlock_t *lock, struct lockdep_map *nest_lock) __acquires(lock);
> > diff --git a/rust/helpers/spinlock.c b/rust/helpers/spinlock.c
> > index b7b0945e8b3c..5971fdf6f755 100644
> > --- a/rust/helpers/spinlock.c
> > +++ b/rust/helpers/spinlock.c
> > @@ -6,10 +6,14 @@ void rust_helper___spin_lock_init(spinlock_t *lock, const char *name,
> >  				  struct lock_class_key *key)
> >  {
> >  #ifdef CONFIG_DEBUG_SPINLOCK
> > +# if defined(CONFIG_PREEMPT_RT)
> > +	__spin_lock_init(lock, name, key, false);
> > +# else /*!CONFIG_PREEMPT_RT */
> >  	__raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
> > -#else
> > +# endif /* CONFIG_PREEMPT_RT */
> > +#else /* !CONFIG_DEBUG_SPINLOCK */
> >  	spin_lock_init(lock);
> > -#endif
> > +#endif /* CONFIG_DEBUG_SPINLOCK */
> >  }
> >  
> >  void rust_helper_spin_lock(spinlock_t *lock)
> > -- 
> > 2.47.0
> > 
> > 
>

Re: [PATCH v2] rust: Fix build error

Posted by Boqun Feng 2 weeks, 2 days ago

On Thu, Nov 07, 2024 at 08:15:15AM +0100, Eder Zulian wrote:
[...]
> > > Fixes: 876346536c1b ("rust: kbuild: split up helpers.c")
> > 
> > I'm not sure this is the correct "Fixes" tag, that commit is a code
> > move, so it's unlikely introducing issue itself. Moreover, we really
> > need PREEMPT_RT being able to trigger the issue, so I think the correct
> 
> One may argue that we need 'RUST=y' in order to trigger the issue.
> 

But RUST support was in mainline earlier than PREEMPT_RT enablement
(again I know we have RT code quite earlier than Rust support, but we
are talking about mainline and potential stable backporting here), so
when the lock support in Rust was added, although the code was missing
RT support, but it's fine from a mainline PoV, and when we really
enabled PREEMPT_RT, we should have added the missing piece.

In other words, would we want to backport this fix into an early version
(say 6.6 stable) where RT has not been enabled? Would there be users who
want to use RT and Rust in that version?

Regards,
Boqun

> > "Fixes" tag is:
> > 
> > Fixes: d2d6422f8bd1 ("x86: Allow to enable PREEMPT_RT.")
> > 
> > (yes, I know PREEMPT_RT is a long existing project, but it was until
> > that commit, you can build a kernel with PREEMPT_RT=y IIUC)
> > 
> > This will help stable maintainers for backport decisions.
> > 
> 
> Perhaps omitting the 'Fixes:' tag would be a solution. Is that an option?
> 
> > 
> > The rest of patch looks good to me (we could maybe provide a
> > __spin_lock_init() for !RT build as well, but that's more of a
> > cleanup)
> > 
> > Regards,
> > Boqun
> > 
> 
> Thanks,
> Eder
> 
[...]

Re: [PATCH v2] rust: Fix build error

Posted by Eder Zulian 2 weeks, 2 days ago

On Thu, Nov 07, 2024 at 08:18:15AM -0800, Boqun Feng wrote:
> On Thu, Nov 07, 2024 at 08:15:15AM +0100, Eder Zulian wrote:
> [...]
> > > > Fixes: 876346536c1b ("rust: kbuild: split up helpers.c")
> > > 
> > > I'm not sure this is the correct "Fixes" tag, that commit is a code
> > > move, so it's unlikely introducing issue itself. Moreover, we really
> > > need PREEMPT_RT being able to trigger the issue, so I think the correct
> > 
> > One may argue that we need 'RUST=y' in order to trigger the issue.
> > 
> 
> But RUST support was in mainline earlier than PREEMPT_RT enablement
> (again I know we have RT code quite earlier than Rust support, but we
> are talking about mainline and potential stable backporting here), so
> when the lock support in Rust was added, although the code was missing
> RT support, but it's fine from a mainline PoV, and when we really
> enabled PREEMPT_RT, we should have added the missing piece.
> 
> In other words, would we want to backport this fix into an early version
> (say 6.6 stable) where RT has not been enabled? Would there be users who
> want to use RT and Rust in that version?
> 

Got it.
Thank you, Boqun. Appreciate your clarity.

> Regards,
> Boqun
> 

Regards,
Eder

> [...]
>