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

    In file included from rust/helpers/helpers.c:22:
    rust/helpers/spinlock.c: In function ‘rust_helper___spin_lock_init’:
    rust/helpers/spinlock.c:10:30: error: implicit declaration of function ‘spinlock_check’; did you mean ‘spin_lock_bh’? [-Wimplicit-function-declaration]
       10 |         __raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
          |                              ^~~~~~~~~~~~~~
          |                              spin_lock_bh
    rust/helpers/spinlock.c:10:30: error: passing argument 1 of ‘__raw_spin_lock_init’ makes pointer from integer without a cast [-Wint-conversion]
       10 |         __raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
          |                              ^~~~~~~~~~~~~~~~~~~~
          |                              |
          |                              int
    In file included from ./include/linux/wait.h:9,
                     from ./include/linux/wait_bit.h:8,
                     from ./include/linux/fs.h:6,
                     from ./include/linux/highmem.h:5,
                     from ./include/linux/bvec.h:10,
                     from ./include/linux/blk_types.h:10,
                     from ./include/linux/blkdev.h:9,
                     from ./include/linux/blk-mq.h:5,
                     from rust/helpers/blk.c:3,
                     from rust/helpers/helpers.c:10:
    ./include/linux/spinlock.h:101:52: note: expected ‘raw_spinlock_t *’ {aka ‘struct raw_spinlock *’} but argument is of type ‘int’
      101 |   extern void __raw_spin_lock_init(raw_spinlock_t *lock, const char *name,
          |                                    ~~~~~~~~~~~~~~~~^~~~
    make[2]: *** [scripts/Makefile.build:229: rust/helpers/helpers.o] Error 1

Error observed while building a rt-debug kernel for aarch64.

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

Suggested-by: Clark Williams <williams@redhat.com>

Signed-off-by: Eder Zulian <ezulian@redhat.com>
---
 rust/helpers/spinlock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rust/helpers/spinlock.c b/rust/helpers/spinlock.c
index acc1376b833c..924c1a380448 100644
--- a/rust/helpers/spinlock.c
+++ b/rust/helpers/spinlock.c
@@ -6,7 +6,7 @@
 void rust_helper___spin_lock_init(spinlock_t *lock, const char *name,
 				  struct lock_class_key *key)
 {
-#ifdef CONFIG_DEBUG_SPINLOCK
+#if defined(CONFIG_DEBUG_SPINLOCK) && !defined(CONFIG_PREEMPT_RT)
 	__raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
 #else
 	spin_lock_init(lock);
-- 
2.46.2

On Mon, Oct 14, 2024 at 9:54 PM Eder Zulian <ezulian@redhat.com> wrote:
>
> Error observed while building a rt-debug kernel for aarch64.

Thanks for testing with Rust enabled!

> Suggested-by: Clark Williams <williams@redhat.com>

Do you mean `Reported-by`?

Also, I am not sure which `Fixes:` tag would fit best here, since
`PREEMPT_RT` has been around for quite a while, but only enabled very
recently. Thomas: do you have a preference?

In addition (sorry, it was in my backlog):

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202409251238.vetlgXE9-lkp@intel.com/

Finally, I think we should perhaps put a helper in `spinlock{_,rt}.h`
that takes the `key` (instead of having this `#ifdef` here) and then
just use that from the Rust helpers, because we don't want to
duplicate such logic (conditionals) in helpers. And with the RT init
open coding that Boqun mentioned, even more. After all, helpers are
meant to be as straightforward as possible, and if we have this sort
of thing in helpers, it is harder for everyone to keep them in sync.

In other words, I see helpers as following the same "avoid `#ifdef`s"
rule that we prefer in C source files vs. headers.

What do you think, Thomas?

>

Spurious newline.

Cheers,
Miguel

Hi Miguel,
On Mon, Oct 14, 2024 at 10:38:45PM +0200, Miguel Ojeda wrote:
> On Mon, Oct 14, 2024 at 9:54 PM Eder Zulian <ezulian@redhat.com> wrote:
> >
> > Error observed while building a rt-debug kernel for aarch64.
> 
> Thanks for testing with Rust enabled!
Sure, it's been fun!
> 
> > Suggested-by: Clark Williams <williams@redhat.com>
> 
> Do you mean `Reported-by`?
Yes, my mistake.
> 
> Also, I am not sure which `Fixes:` tag would fit best here, since
> `PREEMPT_RT` has been around for quite a while, but only enabled very
> recently. Thomas: do you have a preference?
> 
I can try to find a culprit and add a 'Fixes:' tag. In my opnion, at first
glance, it would be the patch that introduced the Rust helper for spinlocks.
Not sure.
> In addition (sorry, it was in my backlog):
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202409251238.vetlgXE9-lkp@intel.com/
> 
I can fix it and send a v2 if that's ok. Is it valid to add two 'Reported-by'
tags (Clark and kernel test robot)?
> Finally, I think we should perhaps put a helper in `spinlock{_,rt}.h`
> that takes the `key` (instead of having this `#ifdef` here) and then
> just use that from the Rust helpers, because we don't want to
> duplicate such logic (conditionals) in helpers. And with the RT init
Agreed. We don't want code replicated. In my reply to Boqun I added some
notes. If that makes sense, we could avoid even the helper in
'spinlock{_,rt}.h'?
> open coding that Boqun mentioned, even more. After all, helpers are
> meant to be as straightforward as possible, and if we have this sort
> of thing in helpers, it is harder for everyone to keep them in sync.
Please correct me if I misunderstood. It seems that Rust doesn't have a
pre-processor step to replace macros in the code and the Rust compiler works
with 'objects/entities' created for functions and variables, but macros would
be ignored (since they are string substitution.) Do you have pointers for good
docs on this?
> 
> In other words, I see helpers as following the same "avoid `#ifdef`s"
> rule that we prefer in C source files vs. headers.
> 
> What do you think, Thomas?
> 
> >
> 
> Spurious newline.
Thanks, I'll fix the spurious new line.
> 
Thank you.
> Cheers,
> Miguel
> 

On Thu, Oct 17, 2024 at 2:15 AM Eder Zulian <ezulian@redhat.com> wrote:
>
> I can fix it and send a v2 if that's ok. Is it valid to add two 'Reported-by'
> tags (Clark and kernel test robot)?

Yeah, I think so, at least if they were independently reported. Please
add a `Closes` if you have it for Clark's report.

> Agreed. We don't want code replicated. In my reply to Boqun I added some
> notes. If that makes sense, we could avoid even the helper in
> 'spinlock{_,rt}.h'?

Hmm... I am not sure I follow your reply to Boqun. In your version,
under `DEBUG_SPINLOCK && PREEMPT_RT`, you call `spin_lock_init`, but
that means we are not passing the given key but creating a new
static/single one, no? That is why Boqun mentioned that.

> Please correct me if I misunderstood. It seems that Rust doesn't have a
> pre-processor step to replace macros in the code and the Rust compiler works
> with 'objects/entities' created for functions and variables, but macros would
> be ignored (since they are string substitution.) Do you have pointers for good
> docs on this?

I am not sure what exactly you are referring to, so perhaps this quick
summary helps (apologies if you already know all this!).

Rust does not understand C, at all. So we use `bindgen`, which is a
tool that internally uses `libclang` to parse C headers and emit Rust
code to use them from Rust. Clang (of course) knows about macros and
can parse them and expand them etc., but those macros (typically)
expand into C code, not Rust code. So (typically) we can't simply use
the macro because it does not generate valid Rust code. Thus we use a
C source file to declare helpers that call the C macros (which is fine
because it is a C file compiled by a C compiler), and then we can call
the C helper function from Rust.

The problem Boqun pointed out is that, now, since you introduced an
extra condition in the same `#ifdef`, then `spin_lock_init` is also
called in a case when `DEBUG_SPINLOCK=y`, which means the key that was
passed as a parameter is not used but the macro will provide a new
one.

Does that help?

As for docs, I am happy to point to some -- do you mean on the Rust
side of things?

Thanks for the patch!

Cheers,
Miguel

Hi,
Really sorry for the long pause, I was OOO for some days...

On Thu, Oct 17, 2024 at 03:24:06PM +0200, Miguel Ojeda wrote:
> On Thu, Oct 17, 2024 at 2:15 AM Eder Zulian <ezulian@redhat.com> wrote:
> >
> > I can fix it and send a v2 if that's ok. Is it valid to add two 'Reported-by'
> > tags (Clark and kernel test robot)?
> 
> Yeah, I think so, at least if they were independently reported. Please
> add a `Closes` if you have it for Clark's report.
> 
> > Agreed. We don't want code replicated. In my reply to Boqun I added some
> > notes. If that makes sense, we could avoid even the helper in
> > 'spinlock{_,rt}.h'?
> 
> Hmm... I am not sure I follow your reply to Boqun. In your version,
> under `DEBUG_SPINLOCK && PREEMPT_RT`, you call `spin_lock_init`, but
> that means we are not passing the given key but creating a new
> static/single one, no? That is why Boqun mentioned that.
> 

I shall send v2 based on Boqun's suggestion soon.

> > Please correct me if I misunderstood. It seems that Rust doesn't have a
> > pre-processor step to replace macros in the code and the Rust compiler works
> > with 'objects/entities' created for functions and variables, but macros would
> > be ignored (since they are string substitution.) Do you have pointers for good
> > docs on this?
> 
> I am not sure what exactly you are referring to, so perhaps this quick
> summary helps (apologies if you already know all this!).
> 
> Rust does not understand C, at all. So we use `bindgen`, which is a
> tool that internally uses `libclang` to parse C headers and emit Rust
> code to use them from Rust. Clang (of course) knows about macros and
> can parse them and expand them etc., but those macros (typically)
> expand into C code, not Rust code. So (typically) we can't simply use
> the macro because it does not generate valid Rust code. Thus we use a
> C source file to declare helpers that call the C macros (which is fine
> because it is a C file compiled by a C compiler), and then we can call
> the C helper function from Rust.
> 
> The problem Boqun pointed out is that, now, since you introduced an
> extra condition in the same `#ifdef`, then `spin_lock_init` is also
> called in a case when `DEBUG_SPINLOCK=y`, which means the key that was
> passed as a parameter is not used but the macro will provide a new
> one.
> 
> Does that help?

Yes, thank you!

> 
> As for docs, I am happy to point to some -- do you mean on the Rust
> side of things?
> 
> Thanks for the patch!
> 
> Cheers,
> Miguel
> 

Cheeers,
Eder

On Mon, Oct 14, 2024 at 10:38:45PM +0200, Miguel Ojeda wrote:
> On Mon, Oct 14, 2024 at 9:54 PM Eder Zulian <ezulian@redhat.com> wrote:
> >
> > Error observed while building a rt-debug kernel for aarch64.
> 
> Thanks for testing with Rust enabled!
> 
> > Suggested-by: Clark Williams <williams@redhat.com>
> 
> Do you mean `Reported-by`?
> 
> Also, I am not sure which `Fixes:` tag would fit best here, since
> `PREEMPT_RT` has been around for quite a while, but only enabled very
> recently. Thomas: do you have a preference?
> 
> In addition (sorry, it was in my backlog):
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202409251238.vetlgXE9-lkp@intel.com/
> 
> Finally, I think we should perhaps put a helper in `spinlock{_,rt}.h`
> that takes the `key` (instead of having this `#ifdef` here) and then
> just use that from the Rust helpers, because we don't want to
> duplicate such logic (conditionals) in helpers. And with the RT init
> open coding that Boqun mentioned, even more. After all, helpers are
> meant to be as straightforward as possible, and if we have this sort
> of thing in helpers, it is harder for everyone to keep them in sync.
> 

Make sense, and we did have something for !PREEMPT_RT spinlock:

	https://lore.kernel.org/rust-for-linux/20230411054543.21278-4-wedsonaf@gmail.com/

and we can do the same thing for PREEMPT_RT:

(untested code)

diff --git a/include/linux/spinlock_rt.h b/include/linux/spinlock_rt.h
index 61c49b16f69a..6ccdd2231575 100644
--- a/include/linux/spinlock_rt.h
+++ b/include/linux/spinlock_rt.h
@@ -16,20 +16,34 @@ static inline void __rt_spin_lock_init(spinlock_t *lock, const char *name,
 }
 #endif

+static inline void __spin_lock_init_with_key(spinlock_t *lock,
+                                            const char *name,
+                                            struct lock_class_key *key,
+                                            bool percpu)
+{
+       rt_mutex_base_init(&lock->lock);
+       __rt_spin_lock_init(slock, name, key, percpu);
+}
+
+static inline void spin_lock_init_with_key(spinlock_t *lock,
+                                            const char *name,
+                                            struct lock_class_key *key)
+{
+       __spin_lock_init_with_key(lock, name, key, false);
+}
+
 #define spin_lock_init(slock)                                  \
 do {                                                           \
        static struct lock_class_key __key;                     \
                                                                \
-       rt_mutex_base_init(&(slock)->lock);                     \
-       __rt_spin_lock_init(slock, #slock, &__key, false);      \
+       spin_lock_init_with_key(slock, #slock, &__key);         \
 } while (0)

 #define local_spin_lock_init(slock)                            \
 do {                                                           \
        static struct lock_class_key __key;                     \
                                                                \
-       rt_mutex_base_init(&(slock)->lock);                     \
-       __rt_spin_lock_init(slock, #slock, &__key, true);       \
+       __spin_lock_init_with_key(slock, #slock, &__key, true); \
 } while (0)

 extern void rt_spin_lock(spinlock_t *lock);

> In other words, I see helpers as following the same "avoid `#ifdef`s"
> rule that we prefer in C source files vs. headers.
> 
> What do you think, Thomas?
> 

Thanks for copying Thomas, my reply to Eder is here:

	https://lore.kernel.org/rust-for-linux/Zw1_rXUyjTBOh0QH@boqun-archlinux/

Regards,
Boqun

> >
> 
> Spurious newline.
> 
> Cheers,
> Miguel

Hi Eder,

Thanks for the patch!

On Mon, Oct 14, 2024 at 09:52:53PM +0200, Eder Zulian wrote:
> When CONFIG_DEBUG_SPINLOCK=y and CONFIG_PREEMPT_RT=y the following build
> error occurs:
> 
>     In file included from rust/helpers/helpers.c:22:
>     rust/helpers/spinlock.c: In function ‘rust_helper___spin_lock_init’:
>     rust/helpers/spinlock.c:10:30: error: implicit declaration of function ‘spinlock_check’; did you mean ‘spin_lock_bh’? [-Wimplicit-function-declaration]
>        10 |         __raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
>           |                              ^~~~~~~~~~~~~~
>           |                              spin_lock_bh
>     rust/helpers/spinlock.c:10:30: error: passing argument 1 of ‘__raw_spin_lock_init’ makes pointer from integer without a cast [-Wint-conversion]
>        10 |         __raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
>           |                              ^~~~~~~~~~~~~~~~~~~~
>           |                              |
>           |                              int
>     In file included from ./include/linux/wait.h:9,
>                      from ./include/linux/wait_bit.h:8,
>                      from ./include/linux/fs.h:6,
>                      from ./include/linux/highmem.h:5,
>                      from ./include/linux/bvec.h:10,
>                      from ./include/linux/blk_types.h:10,
>                      from ./include/linux/blkdev.h:9,
>                      from ./include/linux/blk-mq.h:5,
>                      from rust/helpers/blk.c:3,
>                      from rust/helpers/helpers.c:10:
>     ./include/linux/spinlock.h:101:52: note: expected ‘raw_spinlock_t *’ {aka ‘struct raw_spinlock *’} but argument is of type ‘int’
>       101 |   extern void __raw_spin_lock_init(raw_spinlock_t *lock, const char *name,
>           |                                    ~~~~~~~~~~~~~~~~^~~~
>     make[2]: *** [scripts/Makefile.build:229: rust/helpers/helpers.o] Error 1
> 
> Error observed while building a rt-debug kernel for aarch64.
> 
> On a PREEMPT_RT build, spin locks have been mapped to rt_mutex types, so
> avoid the raw_spinlock_init call for RT.
> 

This is true, but to keep lockdep working I think we need to open code
the PREEMPT_RT version of spin_lock_init(), please see below

> Suggested-by: Clark Williams <williams@redhat.com>
> 

^ unnecessary emtpy line here.

> Signed-off-by: Eder Zulian <ezulian@redhat.com>
> ---
>  rust/helpers/spinlock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/rust/helpers/spinlock.c b/rust/helpers/spinlock.c
> index acc1376b833c..924c1a380448 100644
> --- a/rust/helpers/spinlock.c
> +++ b/rust/helpers/spinlock.c
> @@ -6,7 +6,7 @@
>  void rust_helper___spin_lock_init(spinlock_t *lock, const char *name,
>  				  struct lock_class_key *key)
>  {
> -#ifdef CONFIG_DEBUG_SPINLOCK
> +#if defined(CONFIG_DEBUG_SPINLOCK) && !defined(CONFIG_PREEMPT_RT)
>  	__raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
>  #else
>  	spin_lock_init(lock);

This should be:

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)
	rt_mutex_base_init(&(lock)->lock);
	__rt_spin_lock_init(lock, name, key, false);
# else /* !CONFIG_PREEMPT_RT */
  	__raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
# endif /* CONFIG_PREEMPT_RT */
#else
	spin_lock_init(lock);
#endif
}

Regards,
Boqun

> -- 
> 2.46.2
> 
> 

Hi Boqun,
On Mon, Oct 14, 2024 at 01:31:41PM -0700, Boqun Feng wrote:
> Hi Eder,
> 
> Thanks for the patch!
Sure, my pleasure.
> 
> On Mon, Oct 14, 2024 at 09:52:53PM +0200, Eder Zulian wrote:
> > When CONFIG_DEBUG_SPINLOCK=y and CONFIG_PREEMPT_RT=y the following build
> > error occurs:
> > 
> >     In file included from rust/helpers/helpers.c:22:
> >     rust/helpers/spinlock.c: In function ‘rust_helper___spin_lock_init’:
> >     rust/helpers/spinlock.c:10:30: error: implicit declaration of function ‘spinlock_check’; did you mean ‘spin_lock_bh’? [-Wimplicit-function-declaration]
> >        10 |         __raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
> >           |                              ^~~~~~~~~~~~~~
> >           |                              spin_lock_bh
> >     rust/helpers/spinlock.c:10:30: error: passing argument 1 of ‘__raw_spin_lock_init’ makes pointer from integer without a cast [-Wint-conversion]
> >        10 |         __raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
> >           |                              ^~~~~~~~~~~~~~~~~~~~
> >           |                              |
> >           |                              int
> >     In file included from ./include/linux/wait.h:9,
> >                      from ./include/linux/wait_bit.h:8,
> >                      from ./include/linux/fs.h:6,
> >                      from ./include/linux/highmem.h:5,
> >                      from ./include/linux/bvec.h:10,
> >                      from ./include/linux/blk_types.h:10,
> >                      from ./include/linux/blkdev.h:9,
> >                      from ./include/linux/blk-mq.h:5,
> >                      from rust/helpers/blk.c:3,
> >                      from rust/helpers/helpers.c:10:
> >     ./include/linux/spinlock.h:101:52: note: expected ‘raw_spinlock_t *’ {aka ‘struct raw_spinlock *’} but argument is of type ‘int’
> >       101 |   extern void __raw_spin_lock_init(raw_spinlock_t *lock, const char *name,
> >           |                                    ~~~~~~~~~~~~~~~~^~~~
> >     make[2]: *** [scripts/Makefile.build:229: rust/helpers/helpers.o] Error 1
> > 
> > Error observed while building a rt-debug kernel for aarch64.
> > 
> > On a PREEMPT_RT build, spin locks have been mapped to rt_mutex types, so
> > avoid the raw_spinlock_init call for RT.
> > 
> 
> This is true, but to keep lockdep working I think we need to open code
> the PREEMPT_RT version of spin_lock_init(), please see below
> 
> > Suggested-by: Clark Williams <williams@redhat.com>
> > 
> 
> ^ unnecessary emtpy line here.
Thanks for pointing it out. Should I fix it and send a v2?
> 
> > Signed-off-by: Eder Zulian <ezulian@redhat.com>
> > ---
> >  rust/helpers/spinlock.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/rust/helpers/spinlock.c b/rust/helpers/spinlock.c
> > index acc1376b833c..924c1a380448 100644
> > --- a/rust/helpers/spinlock.c
> > +++ b/rust/helpers/spinlock.c
> > @@ -6,7 +6,7 @@
> >  void rust_helper___spin_lock_init(spinlock_t *lock, const char *name,
> >  				  struct lock_class_key *key)
> >  {
> > -#ifdef CONFIG_DEBUG_SPINLOCK
> > +#if defined(CONFIG_DEBUG_SPINLOCK) && !defined(CONFIG_PREEMPT_RT)
> >  	__raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
> >  #else
> >  	spin_lock_init(lock);
> 
> This should be:
> 
> 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)
Even though I don't have a strong preference on this, in my opinion, there is no
much difference here just a line break and inverted logic. Perhaps it would be
better to write like this: 'if (IS_ENABLED(CONFIG_PREEMPT_RT && ... ) { this }
else { that }', however, spinlock_check() is not declared for PREEMPT_RT
kernels because its declaration is conditional to #ifndef CONFIG_PREEMPT_RT in
'spinlock.h'. spinlock_check() as is does not make sense for PREEMPT_RT
because spinlock is not mapped to raw_spinlock but to rt_mutex (cf.
'spinlock_types.h')
> 	rt_mutex_base_init(&(lock)->lock);
> 	__rt_spin_lock_init(lock, name, key, false);
Apparently, this ^ matches spin_lock_init() in 'spinlock_rt.h', if true, we could
call spin_lock_init() for PREEMPT_RT kernels. However the debug version of
__rt_spin_lock_init() (instrumented with lockdep in 'spinlock_rt.c') depends
on CONFIG_DEBUG_LOCK_ALLOC. Luckily, in my opnion, the #ifdefs are already in
place.
> # else /* !CONFIG_PREEMPT_RT */
>   	__raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
> # endif /* CONFIG_PREEMPT_RT */
> #else
> 	spin_lock_init(lock);
> #endif
> }
> 
Please let me know what you think, and bear with me if I'm missing something.
Thanks.
> Regards,
> Boqun
> 
> > -- 
> > 2.46.2
> > 
> > 
>