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

[Hugh Dickins <hughd@google.com>]

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