[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZyxpA2ez-9E4c7G5@f39>
Date: Thu, 7 Nov 2024 08:15:15 +0100
From: Eder Zulian <ezulian@...hat.com>
To: Boqun Feng <boqun.feng@...il.com>
Cc: rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
miguel.ojeda.sandonis@...il.com, tglx@...utronix.de,
williams@...hat.com, ojeda@...nel.org, alex.gaynor@...il.com,
gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
a.hindborg@...nel.org, aliceryhl@...gle.com, tmgross@...ch.edu,
jlelli@...hat.com
Subject: Re: [PATCH v2] rust: Fix build error
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@...el.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202409251238.vetlgXE9-lkp@intel.com/
> > Signed-off-by: Eder Zulian <ezulian@...hat.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
> >
> >
>
Powered by blists - more mailing lists