Re: [PATCH v2] rust: Fix build error

[Eder Zulian <ezulian@redhat.com>]

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
> > 
> > 
>