Re: [PATCH v4 08/13] drm/v3d: Fix builds with CONFIG_TRANSPARENT_HUGEPAGE=n

[Boris Brezillon <boris.brezillon@collabora.com>]

On Wed, 15 Oct 2025 22:41:59 +0200
Loïc Molinari <loic.molinari@collabora.com> wrote:

> On 15/10/2025 20:17, Boris Brezillon wrote:
> > On Wed, 15 Oct 2025 17:30:12 +0200
> > Loïc Molinari <loic.molinari@collabora.com> wrote:
> >   
> >> Don't declare "super_pages" on builds with CONFIG_TRANSPARENT_HUGEPAGE
> >> disabled to prevent build error:
> >>
> >> ERROR: modpost: "super_pages" [drivers/gpu/drm/v3d/v3d.ko] undefined!  
> > 
> > I believe this is a bug introduced by the previous commit: the
> > compiler probably drops any code between the
> > IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) check and the err label
> > because IS_ENABLED() evaluates to false at compile time. So I'd squash
> > those changes in the previous commit.  
> 
> Right, it's been introduced in previous commit.
> 
> >  
> >>
> >> Signed-off-by: Loïc Molinari <loic.molinari@collabora.com>
> >> ---
> >>   drivers/gpu/drm/v3d/v3d_drv.h | 2 ++
> >>   drivers/gpu/drm/v3d/v3d_gem.c | 2 ++
> >>   2 files changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
> >> index 99a39329bb85..481502104391 100644
> >> --- a/drivers/gpu/drm/v3d/v3d_drv.h
> >> +++ b/drivers/gpu/drm/v3d/v3d_drv.h
> >> @@ -564,7 +564,9 @@ extern const struct dma_fence_ops v3d_fence_ops;
> >>   struct dma_fence *v3d_fence_create(struct v3d_dev *v3d, enum v3d_queue q);
> >>   
> >>   /* v3d_gem.c */
> >> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >>   extern bool super_pages;
> >> +#endif
> >>   int v3d_gem_init(struct drm_device *dev);
> >>   void v3d_gem_destroy(struct drm_device *dev);
> >>   void v3d_reset_sms(struct v3d_dev *v3d);
> >> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
> >> index 635ff0fabe7e..0039063eb8b2 100644
> >> --- a/drivers/gpu/drm/v3d/v3d_gem.c
> >> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> >> @@ -269,7 +269,9 @@ v3d_huge_mnt_init(struct v3d_dev *v3d)
> >>   	 * match our usecase.
> >>   	 */
> >>   
> >> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >>   	if (super_pages)
> >> +#endif
> >>   		err = drm_gem_huge_mnt_create(&v3d->drm, "within_size");  
> > 
> > Why not
> > 
> > #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >    	if (super_pages)
> >    		err = drm_gem_huge_mnt_create(&v3d->drm, "within_size");
> > #endif
> > 
> > I guess
> > 
> > 	if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && super_pages)
> > 		err = drm_gem_huge_mnt_create(&v3d->drm, "within_size");
> > 
> > would also do, since it's likely to rely on the same optimization the
> > previous v3d_gemfs_init() implementation was relying on, but it's
> > fragile (not sure what happens when compiled with -O0).  
> 
> I'll remove the #ifdef/#endif around the super_pages declaration in 
> v3d_drv.h because it isn't necessary if super_pages is compiled out in 
> v3d_huge_mnt_init().
> 
> In v3d_huge_mnt_init(), I'd add the #ifdef before the ret variable 
> declaration and the #endif right after the last else so that it's clear 
> drm_notice("THP is recommended...") is called unconditionally when 
> CONFIG_TRANSPARENT_HUGEPAGE=n, whatever the optim level. What do you think?

First off, I'm not a huge fan of the following pattern

#if foo
	if (xxxx)
#endif
		do_something

which also applies to

#if foo
	if (xxxx)
		do_xxx
	else if (yyy)
		do_yyy
	else
#endif
		do_something

I'd rather have do_something duplicated in an #else section
like that:

#if foo
	if (xxxx)
		do_xxx
	else if (yyy)
		do_yyy
	else
		do_something
#else
	do_something
#endif

But I'm not even seeing what the problem is here. If you do:

	int err = 0;

#ifdef CONFIG_TRANSPARENT_HUGEPAGE
    	if (super_pages)
    		err = drm_gem_huge_mnt_create(&v3d->drm, "within_size");
#endif

	if (v3d->drm.huge_mnt)
		drm_info(&v3d->drm, "Using Transparent Hugepages\n");
	else if (err)
		drm_warn(&v3d->drm, "Can't use Transparent Hugepages (%d)\n", err);
	else
		drm_notice(&v3d->drm,
			   "Transparent Hugepage support is recommended for optimal performance on this platform!\n");

You're guaranteed that err=0 and v3d->drm.huge_mnt=NULL when
CONFIG_TRANSPARENT_HUGEPAGE=n, so the "THP recommended"
message should be displayed unconditionally. Am I missing
something?