Re: [PATCH] rust: Fix build error

[Eder Zulian <ezulian@redhat.com>]

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