[PATCH] mm: Fix compile error when CONFIG_SHMEM is not set

[PATCH] mm: Fix compile error when CONFIG_SHMEM is not set

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

When CONFIG_SHMEM is not set, the following compiler error occurs:

ld: vmlinux.o: in function `ttm_backup_backup_page':
(.text+0x10363bc): undefined reference to `shmem_writeout'
make[3]: *** [/work/build/trace/nobackup/linux-test.git/scripts/Makefile.vmlinux:91: vmlinux.unstripped] Error 1
make[2]: *** [/work/build/trace/nobackup/linux-test.git/Makefile:1241: vmlinux] Error 2
make[1]: *** [/work/build/trace/nobackup/linux-test.git/Makefile:248: __sub-make] Error 2
make[1]: Leaving directory '/work/build/nobackup/tracetest'
make: *** [Makefile:248: __sub-make] Error 2

This is due to the replacement of writepage and calling swap_writepage()
and shmem_writepage() directly. The issue is that when CONFIG_SHMEM is not
defined, shmem_writepage() is also not defined. Add it as a stub, and it
should also never be called when CONFIG_SHMEM is undefined.

Fixes: 84798514db50 ("mm: Remove swap_writepage() and shmem_writepage()")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 mm/shmem.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/shmem.c b/mm/shmem.c
index 858cee02ca49..dec85388030a 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -5760,6 +5760,11 @@ void shmem_unlock_mapping(struct address_space *mapping)
 {
 }
 
+int shmem_writeout(struct folio *folio, struct writeback_control *wbc)
+{
+	return 0;
+}
+
 #ifdef CONFIG_MMU
 unsigned long shmem_get_unmapped_area(struct file *file,
 				      unsigned long addr, unsigned long len,
-- 
2.47.2

Re: [PATCH] mm: Fix compile error when CONFIG_SHMEM is not set

[Steven Rostedt]

On Mon, 2 Jun 2025 17:05:00 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <rostedt@goodmis.org>
> 
> When CONFIG_SHMEM is not set, the following compiler error occurs:
> 
> ld: vmlinux.o: in function `ttm_backup_backup_page':

Actually, I was looking at the wrong location and not at ttm_backup_backup_page().

Which commit fe75adffac33e ("ttm: Call shmem_writeout() from
ttm_backup_backup_page()") updates as:

diff --git a/drivers/gpu/drm/ttm/ttm_backup.c b/drivers/gpu/drm/ttm/ttm_backup.c
index 93c007f18855..0d5718466ffc 100644
--- a/drivers/gpu/drm/ttm/ttm_backup.c
+++ b/drivers/gpu/drm/ttm/ttm_backup.c
@@ -136,13 +136,13 @@ ttm_backup_backup_page(struct ttm_backup *backup, struct page *page,
                        .for_reclaim = 1,
                };
                folio_set_reclaim(to_folio);
-               ret = mapping->a_ops->writepage(folio_file_page(to_folio, idx), &wbc);
+               ret = shmem_writeout(to_folio, &wbc);
                if (!folio_test_writeback(to_folio))
                        folio_clear_reclaim(to_folio);
                /*
-                * If writepage succeeds, it unlocks the folio.
-                * writepage() errors are otherwise dropped, since writepage()
-                * is only best effort here.
+                * If writeout succeeds, it unlocks the folio.  errors
+                * are otherwise dropped, since writeout is only best
+                * effort here.
                 */
                if (ret)
                        folio_unlock(to_folio);

I'm not sure this is the right fix or not.


> (.text+0x10363bc): undefined reference to `shmem_writeout'
> make[3]: *** [/work/build/trace/nobackup/linux-test.git/scripts/Makefile.vmlinux:91: vmlinux.unstripped] Error 1
> make[2]: *** [/work/build/trace/nobackup/linux-test.git/Makefile:1241: vmlinux] Error 2
> make[1]: *** [/work/build/trace/nobackup/linux-test.git/Makefile:248: __sub-make] Error 2
> make[1]: Leaving directory '/work/build/nobackup/tracetest'
> make: *** [Makefile:248: __sub-make] Error 2
> 
> This is due to the replacement of writepage and calling swap_writepage()
> and shmem_writepage() directly. The issue is that when CONFIG_SHMEM is not
> defined, shmem_writepage() is also not defined. Add it as a stub, and it
> should also never be called when CONFIG_SHMEM is undefined.
> 
> Fixes: 84798514db50 ("mm: Remove swap_writepage() and shmem_writepage()")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  mm/shmem.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 858cee02ca49..dec85388030a 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -5760,6 +5760,11 @@ void shmem_unlock_mapping(struct address_space *mapping)
>  {
>  }
>  
> +int shmem_writeout(struct folio *folio, struct writeback_control *wbc)
> +{
> +	return 0;

Perhaps this should return:

	return swap_writeout(folio, wbc);

?

-- Steve

> +}
> +
>  #ifdef CONFIG_MMU
>  unsigned long shmem_get_unmapped_area(struct file *file,
>  				      unsigned long addr, unsigned long len,

Re: [PATCH] mm: Fix compile error when CONFIG_SHMEM is not set

[Matthew Wilcox]

On Mon, Jun 02, 2025 at 05:14:58PM -0400, Steven Rostedt wrote:
> On Mon, 2 Jun 2025 17:05:00 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: Steven Rostedt <rostedt@goodmis.org>
> > 
> > When CONFIG_SHMEM is not set, the following compiler error occurs:
> > 
> > ld: vmlinux.o: in function `ttm_backup_backup_page':
> 
> I'm not sure this is the right fix or not.

> > +int shmem_writeout(struct folio *folio, struct writeback_control *wbc)
> > +{
> > +	return 0;
> 
> Perhaps this should return:
> 
> 	return swap_writeout(folio, wbc);

I don't think so.  ttm_backup_backup_page() gets its page from:

        to_folio = shmem_read_folio_gfp(mapping, idx, alloc_gfp);
...
                ret = shmem_writeout(to_folio, &wbc);

and if you look at the implementation of shmem_read_folio_gfp() does:

#else
        /*
         * The tiny !SHMEM case uses ramfs without swap
         */
        return mapping_read_folio_gfp(mapping, index, gfp);
#endif

so I would say that if anybody is actually using it this way (and 99%
chance they're not), they literally cannot write back the folio.  So
I think your initial patch is fine.

Re: [PATCH] mm: Fix compile error when CONFIG_SHMEM is not set

[Hugh Dickins]

On Mon, 2 Jun 2025, Matthew Wilcox wrote:
> On Mon, Jun 02, 2025 at 05:14:58PM -0400, Steven Rostedt wrote:
> > On Mon, 2 Jun 2025 17:05:00 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > From: Steven Rostedt <rostedt@goodmis.org>
> > > 
> > > When CONFIG_SHMEM is not set, the following compiler error occurs:
> > > 
> > > ld: vmlinux.o: in function `ttm_backup_backup_page':
> > 
> > I'm not sure this is the right fix or not.
> 
> > > +int shmem_writeout(struct folio *folio, struct writeback_control *wbc)
> > > +{
> > > +	return 0;
> > 
> > Perhaps this should return:
> > 
> > 	return swap_writeout(folio, wbc);
> 
> I don't think so.  ttm_backup_backup_page() gets its page from:
> 
>         to_folio = shmem_read_folio_gfp(mapping, idx, alloc_gfp);
> ...
>                 ret = shmem_writeout(to_folio, &wbc);
> 
> and if you look at the implementation of shmem_read_folio_gfp() does:
> 
> #else
>         /*
>          * The tiny !SHMEM case uses ramfs without swap
>          */
>         return mapping_read_folio_gfp(mapping, index, gfp);
> #endif
> 
> so I would say that if anybody is actually using it this way (and 99%
> chance they're not), they literally cannot write back the folio.  So
> I think your initial patch is fine.

Agreed that ramfs does not use swap, so calling swap_writepage() would
be weird.  But, thanks for the build fix Steve, but it cannot be right
because return 0 says shmem_writeout() successfully sent the page to
swap, and that has unlocked the page (or soon will do so).  It should
return an error (-ENXIO?), but I haven't checked what the callers do with
that, nor whether they need the folio to be redirtied.  And wouldn't it
need an EXPORT like the real one?  Sorry, can't keep up, there are many
many things I should have looked at but have not... Tomorrow?

Hugh

Re: [PATCH] mm: Fix compile error when CONFIG_SHMEM is not set

[Steven Rostedt]

On Tue, 3 Jun 2025 01:02:36 -0700 (PDT)
Hugh Dickins <hughd@google.com> wrote:

> Agreed that ramfs does not use swap, so calling swap_writepage() would
> be weird.  But, thanks for the build fix Steve, but it cannot be right
> because return 0 says shmem_writeout() successfully sent the page to
> swap, and that has unlocked the page (or soon will do so).  It should
> return an error (-ENXIO?), but I haven't checked what the callers do with

Yeah, I figured it should return an error, but looking at the code I
couldn't figure out what the proper error would be. Then I also noticed
that the other stub functions just returned zero so I did the same.

Perhaps add a WARN_ON_ONCE() if it is called without CONFIG_SHMEM configured?

> that, nor whether they need the folio to be redirtied.  And wouldn't it
> need an EXPORT like the real one?  Sorry, can't keep up, there are many
> many things I should have looked at but have not... Tomorrow?

Yeah, it probably needs an export as well.

Note, if you come up with a solution, by all means use it and don't use my
patch. I'm happy with a "Reported-by" and don't need to be the author. This
is the "fix" I'm using so that I can test my code because without it, my
tests fail.

I'm also happy to send another update if it's simple.

-- Steve

Re: [PATCH] mm: Fix compile error when CONFIG_SHMEM is not set

[Matthew Wilcox]

On Tue, Jun 03, 2025 at 10:29:59AM -0400, Steven Rostedt wrote:
> On Tue, 3 Jun 2025 01:02:36 -0700 (PDT)
> Hugh Dickins <hughd@google.com> wrote:
> 
> > Agreed that ramfs does not use swap, so calling swap_writepage() would
> > be weird.  But, thanks for the build fix Steve, but it cannot be right
> > because return 0 says shmem_writeout() successfully sent the page to
> > swap, and that has unlocked the page (or soon will do so).  It should
> > return an error (-ENXIO?), but I haven't checked what the callers do with
> 
> Yeah, I figured it should return an error, but looking at the code I
> couldn't figure out what the proper error would be. Then I also noticed
> that the other stub functions just returned zero so I did the same.
> 
> Perhaps add a WARN_ON_ONCE() if it is called without CONFIG_SHMEM configured?

Or just make this module depend on SHMEM?  I don't think it makes much
sense to use it without being able to swap, and shmem can't swap ...

Re: [PATCH] mm: Fix compile error when CONFIG_SHMEM is not set

[Steven Rostedt]

[ Adding DRM folks ]

On Tue, 3 Jun 2025 17:26:23 +0100
Matthew Wilcox <willy@infradead.org> wrote:

> On Tue, Jun 03, 2025 at 10:29:59AM -0400, Steven Rostedt wrote:
> > On Tue, 3 Jun 2025 01:02:36 -0700 (PDT)
> > Hugh Dickins <hughd@google.com> wrote:
> >   
> > > Agreed that ramfs does not use swap, so calling swap_writepage() would
> > > be weird.  But, thanks for the build fix Steve, but it cannot be right
> > > because return 0 says shmem_writeout() successfully sent the page to
> > > swap, and that has unlocked the page (or soon will do so).  It should
> > > return an error (-ENXIO?), but I haven't checked what the callers do with  
> > 
> > Yeah, I figured it should return an error, but looking at the code I
> > couldn't figure out what the proper error would be. Then I also noticed
> > that the other stub functions just returned zero so I did the same.
> > 
> > Perhaps add a WARN_ON_ONCE() if it is called without CONFIG_SHMEM configured?  
> 
> Or just make this module depend on SHMEM?  I don't think it makes much
> sense to use it without being able to swap, and shmem can't swap ...

Heh, not exactly sure what to make depend on CONFIG_SHMEM. The function is:

  ttm_backup_backup_page()

Which is defined when CONFIG_DRM_TTM is set, but just doing:

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index f094797f3b2b..ebb948a0142f 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -187,7 +187,7 @@ source "drivers/gpu/drm/display/Kconfig"
 
 config DRM_TTM
 	tristate
-	depends on DRM && MMU
+	depends on DRM && MMU && SHMEM
 	help
 	  GPU memory management subsystem for devices with multiple
 	  GPU memory types. Will be enabled automatically if a device driver

Isn't good enough because "select" can override depends on :-p and DRM_TTM
gets selected by:

 Symbol: DRM_TTM [=y]
 Type  : tristate
 Defined at drivers/gpu/drm/Kconfig:188
   Depends on: HAS_IOMEM [=y] && DRM [=y] && MMU [=y] && SHMEM [=n]
 Selected by [y]:
   - DRM_TTM_HELPER [=y] && HAS_IOMEM [=y] && DRM [=y]
   - DRM_RADEON [=y] && HAS_IOMEM [=y] && DRM [=y] && PCI [=y] && MMU [=y] && (AGP [=n] || !AGP [=n])
   - DRM_VMWGFX [=y] && HAS_IOMEM [=y] && DRM [=y] && PCI [=y] && MMU [=y] && (X86 [=y] && HYPERVISOR_GUEST [=y] || ARM64)
 Selected by [n]:
   - DRM_TTM_KUNIT_TEST [=n] && HAS_IOMEM [=y] && DRM [=y] && KUNIT [=n] && MMU [=y] && (UML || COMPILE_TEST [=n])
   - DRM_AMDGPU [=n] && HAS_IOMEM [=y] && DRM [=y] && PCI [=y] && MMU [=y] && !UML
   - DRM_NOUVEAU [=n] && HAS_IOMEM [=y] && DRM [=y] && PCI [=y] && MMU [=y]
   - DRM_I915 [=n] && HAS_IOMEM [=y] && DRM [=y] && X86 [=y] && PCI [=y] && !PREEMPT_RT [=n]
   - DRM_XE [=n] && HAS_IOMEM [=y] && DRM [=y] && PCI [=y] && MMU [=y] && (m [=m] && MODULES [=n] || KUNIT [=n]=y [=y]
   - DRM_QXL [=n] && HAS_IOMEM [=y] && DRM [=y] && PCI [=y] && MMU [=y] && HAS_IOPORT [=y]
   - DRM_LOONGSON [=n] && HAS_IOMEM [=y] && DRM [=y] && PCI [=y] && MMU [=y] && (LOONGARCH || MIPS || COMPILE_TEST [=n])
   - DRM_HISI_HIBMC [=n] && HAS_IOMEM [=y] && DRM [=y] && PCI [=y] && MMU [=y]
   - DRM_VBOXVIDEO [=n] && HAS_IOMEM [=y] && DRM [=y] && X86 [=y] && PCI [=y]

Should DRM itself depend on SHMEM?

-- Steve

Re: [PATCH] mm: Fix compile error when CONFIG_SHMEM is not set

[Linus Torvalds]

On Tue, 3 Jun 2025 at 10:26, Steven Rostedt <rostedt@goodmis.org> wrote:
>
>  config DRM_TTM
>         tristate
> -       depends on DRM && MMU
> +       depends on DRM && MMU && SHMEM

Yeah, except I think you should just make it be

          depends on DRM && SHMEM

because SHMEM already depends on MMU.

That said, our docs already say that if you disable SHMEM, it gets
replaced by RAMFS, so maybe just having a ramfs version is the
RightThing(tm).

I don't think such a ramfs version should just return 0 - much less an
error. I think it should always redirty the page.

IOW, I think the "ramfs" version should look something like

        folio_mark_dirty(folio);
        if (wbc->for_reclaim)
                return AOP_WRITEPAGE_ACTIVATE;  /* Return with folio locked */
        folio_unlock(folio);
        return 0;

which is what shmem does for the "page is locked" case.

            Linus

Re: [PATCH] mm: Fix compile error when CONFIG_SHMEM is not set

[Steven Rostedt]

On Tue, 3 Jun 2025 10:54:49 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Tue, 3 Jun 2025 at 10:26, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> >  config DRM_TTM
> >         tristate
> > -       depends on DRM && MMU
> > +       depends on DRM && MMU && SHMEM  
> 
> Yeah, except I think you should just make it be
> 
>           depends on DRM && SHMEM
> 
> because SHMEM already depends on MMU.

Yeah, if I had made this a real patch I would have done that, but this was
only for seeing it it would work.

> 
> That said, our docs already say that if you disable SHMEM, it gets
> replaced by RAMFS, so maybe just having a ramfs version is the
> RightThing(tm).
> 
> I don't think such a ramfs version should just return 0 - much less an
> error. I think it should always redirty the page.
> 
> IOW, I think the "ramfs" version should look something like
> 
>         folio_mark_dirty(folio);
>         if (wbc->for_reclaim)
>                 return AOP_WRITEPAGE_ACTIVATE;  /* Return with folio locked */
>         folio_unlock(folio);
>         return 0;
> 
> which is what shmem does for the "page is locked" case.

I'll let someone that understand the code a bit more than I do to make such
a change. My patch was just a "this makes my system build" thing and let
those that know this code do the RightThing(tm).

-- Steve

Re: [PATCH] mm: Fix compile error when CONFIG_SHMEM is not set

[Hugh Dickins]

[-- Attachment #1: Type: text/plain, Size: 3177 bytes --]

Adding Thomas Hellström, father of ttm_backup_backup_page():
Steve doesn't have CONFIG_SHMEM=y, so now gets a build error because
there's no shmem_writeout(); whereas before 6.16, backup_backup
writeback would have oopsed on calling NULL ram_aops.writepage
when CONFIG_SHMEM is not set.

On Tue, 3 Jun 2025, Steven Rostedt wrote:
> On Tue, 3 Jun 2025 10:54:49 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > On Tue, 3 Jun 2025 at 10:26, Steven Rostedt <rostedt@goodmis.org> wrote:
> > >
> > >  config DRM_TTM
> > >         tristate
> > > -       depends on DRM && MMU
> > > +       depends on DRM && MMU && SHMEM  
> > 
> > Yeah, except I think you should just make it be
> > 
> >           depends on DRM && SHMEM
> > 
> > because SHMEM already depends on MMU.
> 
> Yeah, if I had made this a real patch I would have done that, but this was
> only for seeing it it would work.

Many in drivers/gpu/drm do already "select SHMEM",
so I came to think that

--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -188,6 +188,7 @@ source "drivers/gpu/drm/display/Kconfig"
 config DRM_TTM
 	tristate
 	depends on DRM && MMU
+	select SHMEM
 	help
 	  GPU memory management subsystem for devices with multiple
 	  GPU memory types. Will be enabled automatically if a device driver

would be the right answer.

But perhaps that adds bloat to kernels that don't need backup_backup
writeback, and some #ifdef CONFIG_SHMEMs in or around backup_backup
would be more to the point.  Maybe add that "select SHMEM" line
before rc1, then refine ttm_backup.c with #ifdefs at leisure?

But I just don't appreciate backup_backup and its place in the
drm world: it's a matter for Thomas and dri-devel to work out.

> 
> > 
> > That said, our docs already say that if you disable SHMEM, it gets
> > replaced by RAMFS, so maybe just having a ramfs version is the
> > RightThing(tm).
> > 
> > I don't think such a ramfs version should just return 0 - much less an
> > error. I think it should always redirty the page.
> > 
> > IOW, I think the "ramfs" version should look something like
> > 
> >         folio_mark_dirty(folio);
> >         if (wbc->for_reclaim)
> >                 return AOP_WRITEPAGE_ACTIVATE;  /* Return with folio locked */
> >         folio_unlock(folio);
> >         return 0;
> > 
> > which is what shmem does for the "page is locked" case.
> 
> I'll let someone that understand the code a bit more than I do to make such
> a change. My patch was just a "this makes my system build" thing and let
> those that know this code do the RightThing(tm).

I did start out from the position that mm/shmem.c should provide a good
shmem_writeout() stub for the tiny !CONFIG_SHMEM case.  But seeing as
nobody else has wanted it before, and backup_backup would have been
crashing in such a case, it now seems to me pointless to add such a stub.

Unless all those drm "select SHMEM"s got added long ago to avoid exactly
such crashes, and a shmem stub is preferred throughout drivers/gpu/drm.

I vote for the "select SHMEM", but Thomas and dri-devel and Linus
should decide.

Hugh

Re: [PATCH] mm: Fix compile error when CONFIG_SHMEM is not set

[Steven Rostedt]

On Wed, 4 Jun 2025 00:03:18 -0700 (PDT)
Hugh Dickins <hughd@google.com> wrote:

> I vote for the "select SHMEM", but Thomas and dri-devel and Linus
> should decide.

I only tried "depends on SHMEM" which did not work, but it looks like
"select SHMEM" should.

I prefer this solution too.

Thanks,

-- Steve

Re: [PATCH] mm: Fix compile error when CONFIG_SHMEM is not set

[Thomas Hellström]

On Wed, 2025-06-04 at 08:04 -0400, Steven Rostedt wrote:
> On Wed, 4 Jun 2025 00:03:18 -0700 (PDT)
> Hugh Dickins <hughd@google.com> wrote:
> 
> > I vote for the "select SHMEM", but Thomas and dri-devel and Linus
> > should decide.
> 
> I only tried "depends on SHMEM" which did not work, but it looks like
> "select SHMEM" should.

I agree. The whole ttm_backup implementation is based on backing things
up to shmem objects so IMO it perfectly makes sense to "select SHMEM".

Let me know if you want me to send a patch for that.

In the very unlikely case someone would ever want a config without
SHMEM but with TTM, they'd have to live without the ttm_backup and we'd
create a separate config for that.

/Thomas


> 
> I prefer this solution too.
> 
> Thanks,
> 
> -- Steve

Re: [PATCH] mm: Fix compile error when CONFIG_SHMEM is not set

[Steven Rostedt]

On Wed, 04 Jun 2025 14:18:06 +0200
Thomas Hellström <thomas.hellstrom@linux.intel.com> wrote:

> Let me know if you want me to send a patch for that.

This is a simple fix. I can send the patch and make sure it fixes my builds.

Thanks,

-- Steve