Re: [PATCH v2] rust: Fix build error

From: Eder Zulian
Date: Thu Nov 07 2024 - 02:15:42 EST


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@xxxxxxxxx/
> >
>
> 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@xxxxxxxxx>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202409251238.vetlgXE9-lkp@xxxxxxxxx/
> > Signed-off-by: Eder Zulian <ezulian@xxxxxxxxxx>
> > ---
> > 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
> >
> >
>