mm: forward declare struct rcuwait together with rcuwait_wake_up()

[PATCH] mm: forward declare struct rcuwait together with rcuwait_wake_up()

Posted by Thomas Weißschuh 2 weeks, 1 day ago

At the point of the forward declaration of rcuwait_wake_up()
in mmap_lock.h 'struct rcuwait' may have not yet been declared,
leading to compiler errors.

Add an explicit forward declaration for the struct.

Fixes: 75404e07663b ("mm: move mmap/vma locking logic into specific files")
Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
This doesn't seem to break any in-tree code right now.
I stumbled upon it while building a series for the next cycle.
Instead of putting this fix into my series and spamming all the mm
maintainers with it, maybe this could be part of the last mm bugfix pull
for this cycle.
---
 include/linux/mmap_lock.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
index 11a078de9150df1beff4f0bfb16e199333767614..9792dd4fff0ff73829833aae8ea3229a31757d61 100644
--- a/include/linux/mmap_lock.h
+++ b/include/linux/mmap_lock.h
@@ -3,6 +3,7 @@
 #define _LINUX_MMAP_LOCK_H
 
 /* Avoid a dependency loop by declaring here. */
+struct rcuwait;
 extern int rcuwait_wake_up(struct rcuwait *w);
 
 #include <linux/lockdep.h>

---
base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
change-id: 20250916-mm-rcuwait-03c5fe95f36d

Best regards,
-- 
Thomas Weißschuh <thomas.weissschuh@linutronix.de>

Re: [PATCH] mm: forward declare struct rcuwait together with rcuwait_wake_up()

Posted by Suren Baghdasaryan 2 weeks, 1 day ago

On Tue, Sep 16, 2025 at 6:59 AM Thomas Weißschuh
<thomas.weissschuh@linutronix.de> wrote:
>
> At the point of the forward declaration of rcuwait_wake_up()
> in mmap_lock.h 'struct rcuwait' may have not yet been declared,
> leading to compiler errors.
>
> Add an explicit forward declaration for the struct.
>
> Fixes: 75404e07663b ("mm: move mmap/vma locking logic into specific files")
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
> This doesn't seem to break any in-tree code right now.
> I stumbled upon it while building a series for the next cycle.
> Instead of putting this fix into my series and spamming all the mm
> maintainers with it, maybe this could be part of the last mm bugfix pull
> for this cycle.

`struct rcuwait` is defined inside include/linux/types.h and
mmap_lock.h includes that file. Could you please explain in more
detail what exactly failed when you were building it?

> ---
>  include/linux/mmap_lock.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
> index 11a078de9150df1beff4f0bfb16e199333767614..9792dd4fff0ff73829833aae8ea3229a31757d61 100644
> --- a/include/linux/mmap_lock.h
> +++ b/include/linux/mmap_lock.h
> @@ -3,6 +3,7 @@
>  #define _LINUX_MMAP_LOCK_H
>
>  /* Avoid a dependency loop by declaring here. */
> +struct rcuwait;
>  extern int rcuwait_wake_up(struct rcuwait *w);
>
>  #include <linux/lockdep.h>
>
> ---
> base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
> change-id: 20250916-mm-rcuwait-03c5fe95f36d
>
> Best regards,
> --
> Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>

Re: [PATCH] mm: forward declare struct rcuwait together with rcuwait_wake_up()

Posted by Thomas Weißschuh 2 weeks, 1 day ago

On Tue, Sep 16, 2025 at 03:04:21PM -0700, Suren Baghdasaryan wrote:
> On Tue, Sep 16, 2025 at 6:59 AM Thomas Weißschuh
> <thomas.weissschuh@linutronix.de> wrote:
> >
> > At the point of the forward declaration of rcuwait_wake_up()
> > in mmap_lock.h 'struct rcuwait' may have not yet been declared,
> > leading to compiler errors.
> >
> > Add an explicit forward declaration for the struct.
> >
> > Fixes: 75404e07663b ("mm: move mmap/vma locking logic into specific files")
> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> > ---
> > This doesn't seem to break any in-tree code right now.
> > I stumbled upon it while building a series for the next cycle.
> > Instead of putting this fix into my series and spamming all the mm
> > maintainers with it, maybe this could be part of the last mm bugfix pull
> > for this cycle.
> 
> `struct rcuwait` is defined inside include/linux/types.h and
> mmap_lock.h includes that file.

Yes, linux/types.h is included, but only after the usage of 'struct rcuwait'.
We could also order around the '#include <linux/types.h>' before the
declaration of rcuwait_wake_up(), but to me my current proposal looks cleaner.

> Could you please explain in more
> detail what exactly failed when you were building it?

With the following change for test purposes:

diff --git a/init/main.c b/init/main.c
index 0d4510a7a5c2..7523786e6ad1 100644
--- a/init/main.c
+++ b/init/main.c
@@ -10,6 +10,8 @@
  *  Simplified starting of init:  Michael A. Griffith <grif@acm.org>
  */
 
+#include <linux/mmap_lock.h>
+
 #define DEBUG          /* Enable initcall_debug */
 
 #include <linux/types.h>


This is the error:

In file included from .../init/main.c:13:
.../include/linux/mmap_lock.h:6:35: error: 'struct rcuwait' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
    6 | extern int rcuwait_wake_up(struct rcuwait *w);
      |                                   ^~~~~~~
In file included from .../include/linux/percpu-rwsem.h:7,
                 from .../include/linux/fs.h:34,
                 from .../include/linux/proc_fs.h:10,
                 from .../init/main.c:21:
.../include/linux/rcuwait.h:26:12: error: conflicting types for 'rcuwait_wake_up'; have 'int(struct rcuwait *)'
   26 | extern int rcuwait_wake_up(struct rcuwait *w);
      |            ^~~~~~~~~~~~~~~
.../include/linux/mmap_lock.h:6:12: note: previous declaration of 'rcuwait_wake_up' with type 'int(struct rcuwait *)'
    6 | extern int rcuwait_wake_up(struct rcuwait *w);
      |            ^~~~~~~~~~~~~~~

> > ---
> >  include/linux/mmap_lock.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
> > index 11a078de9150df1beff4f0bfb16e199333767614..9792dd4fff0ff73829833aae8ea3229a31757d61 100644
> > --- a/include/linux/mmap_lock.h
> > +++ b/include/linux/mmap_lock.h
> > @@ -3,6 +3,7 @@
> >  #define _LINUX_MMAP_LOCK_H
> >
> >  /* Avoid a dependency loop by declaring here. */
> > +struct rcuwait;
> >  extern int rcuwait_wake_up(struct rcuwait *w);
> >
> >  #include <linux/lockdep.h>
> >
> > ---
> > base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
> > change-id: 20250916-mm-rcuwait-03c5fe95f36d
> >
> > Best regards,
> > --
> > Thomas Weißschuh <thomas.weissschuh@linutronix.de>

Re: [PATCH] mm: forward declare struct rcuwait together with rcuwait_wake_up()

Posted by Suren Baghdasaryan 2 weeks ago

On Tue, Sep 16, 2025 at 11:12 PM Thomas Weißschuh
<thomas.weissschuh@linutronix.de> wrote:
>
> On Tue, Sep 16, 2025 at 03:04:21PM -0700, Suren Baghdasaryan wrote:
> > On Tue, Sep 16, 2025 at 6:59 AM Thomas Weißschuh
> > <thomas.weissschuh@linutronix.de> wrote:
> > >
> > > At the point of the forward declaration of rcuwait_wake_up()
> > > in mmap_lock.h 'struct rcuwait' may have not yet been declared,
> > > leading to compiler errors.
> > >
> > > Add an explicit forward declaration for the struct.
> > >
> > > Fixes: 75404e07663b ("mm: move mmap/vma locking logic into specific files")
> > > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> > > ---
> > > This doesn't seem to break any in-tree code right now.
> > > I stumbled upon it while building a series for the next cycle.
> > > Instead of putting this fix into my series and spamming all the mm
> > > maintainers with it, maybe this could be part of the last mm bugfix pull
> > > for this cycle.
> >
> > `struct rcuwait` is defined inside include/linux/types.h and
> > mmap_lock.h includes that file.
>
> Yes, linux/types.h is included, but only after the usage of 'struct rcuwait'.
> We could also order around the '#include <linux/types.h>' before the
> declaration of rcuwait_wake_up(), but to me my current proposal looks cleaner.
>
> > Could you please explain in more
> > detail what exactly failed when you were building it?
>
> With the following change for test purposes:
>
> diff --git a/init/main.c b/init/main.c
> index 0d4510a7a5c2..7523786e6ad1 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -10,6 +10,8 @@
>   *  Simplified starting of init:  Michael A. Griffith <grif@acm.org>
>   */
>
> +#include <linux/mmap_lock.h>
> +
>  #define DEBUG          /* Enable initcall_debug */
>
>  #include <linux/types.h>
>
>
> This is the error:
>
> In file included from .../init/main.c:13:
> .../include/linux/mmap_lock.h:6:35: error: 'struct rcuwait' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
>     6 | extern int rcuwait_wake_up(struct rcuwait *w);
>       |                                   ^~~~~~~
> In file included from .../include/linux/percpu-rwsem.h:7,
>                  from .../include/linux/fs.h:34,
>                  from .../include/linux/proc_fs.h:10,
>                  from .../init/main.c:21:
> .../include/linux/rcuwait.h:26:12: error: conflicting types for 'rcuwait_wake_up'; have 'int(struct rcuwait *)'
>    26 | extern int rcuwait_wake_up(struct rcuwait *w);
>       |            ^~~~~~~~~~~~~~~
> .../include/linux/mmap_lock.h:6:12: note: previous declaration of 'rcuwait_wake_up' with type 'int(struct rcuwait *)'
>     6 | extern int rcuwait_wake_up(struct rcuwait *w);
>       |            ^~~~~~~~~~~~~~~
>

I see, thanks!
I would prefer simply moving rcuwait_wake_up() forward declaration
after the includes, like this:


 #ifndef _LINUX_MMAP_LOCK_H
 #define _LINUX_MMAP_LOCK_H

-/* Avoid a dependency loop by declaring here. */
-extern int rcuwait_wake_up(struct rcuwait *w);
-
 #include <linux/lockdep.h>
 #include <linux/mm_types.h>
 #include <linux/mmdebug.h>
@@ -14,6 +11,9 @@ extern int rcuwait_wake_up(struct rcuwait *w);
 #include <linux/cleanup.h>
 #include <linux/sched/mm.h>

+extern int rcuwait_wake_up(struct rcuwait *w);
+
 #define MMAP_LOCK_INITIALIZER(name) \


This would avoid extra forward declarations. Not sure why it was
placed before includes in the first place. I tried moving it and
things build just fine. Lorenzo, do you recall specific reasons the
forward declaration should have been placed before the includes?

> > > ---
> > >  include/linux/mmap_lock.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
> > > index 11a078de9150df1beff4f0bfb16e199333767614..9792dd4fff0ff73829833aae8ea3229a31757d61 100644
> > > --- a/include/linux/mmap_lock.h
> > > +++ b/include/linux/mmap_lock.h
> > > @@ -3,6 +3,7 @@
> > >  #define _LINUX_MMAP_LOCK_H
> > >
> > >  /* Avoid a dependency loop by declaring here. */
> > > +struct rcuwait;
> > >  extern int rcuwait_wake_up(struct rcuwait *w);
> > >
> > >  #include <linux/lockdep.h>
> > >
> > > ---
> > > base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
> > > change-id: 20250916-mm-rcuwait-03c5fe95f36d
> > >
> > > Best regards,
> > > --
> > > Thomas Weißschuh <thomas.weissschuh@linutronix.de>