mm/shmem.c | 5 +++++ 1 file changed, 5 insertions(+)
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.2On 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,
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.
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
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
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 ...
[ 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
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
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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.