| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <02c8447d-25fc-4503-873f-0b2932e218ec@collabora.com>
Date: Thu, 16 Oct 2025 09:09:26 +0200
From: Loïc Molinari <loic.molinari@...labora.com>
To: Boris Brezillon <boris.brezillon@...labora.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Jani Nikula <jani.nikula@...ux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>, Tvrtko Ursulin
<tursulin@...ulin.net>, Rob Herring <robh@...nel.org>,
Steven Price <steven.price@....com>, Liviu Dudau <liviu.dudau@....com>,
Melissa Wen <mwen@...lia.com>, Maíra Canal
<mcanal@...lia.com>, Hugh Dickins <hughd@...gle.com>,
Baolin Wang <baolin.wang@...ux.alibaba.com>,
Andrew Morton <akpm@...ux-foundation.org>, Al Viro
<viro@...iv.linux.org.uk>, Mikołaj Wasiak
<mikolaj.wasiak@...el.com>, Christian Brauner <brauner@...nel.org>,
Nitin Gote <nitin.r.gote@...el.com>, Andi Shyti
<andi.shyti@...ux.intel.com>, Jonathan Corbet <corbet@....net>,
Christopher Healy <healych@...zon.com>, Matthew Wilcox
<willy@...radead.org>, Bagas Sanjaya <bagasdotme@...il.com>,
linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
intel-gfx@...ts.freedesktop.org, linux-mm@...ck.org,
linux-doc@...r.kernel.org, kernel@...labora.com
Subject: Re: [PATCH v4 08/13] drm/v3d: Fix builds with
CONFIG_TRANSPARENT_HUGEPAGE=n
Hi Boris,
On 16/10/2025 07:56, Boris Brezillon wrote:
> On Wed, 15 Oct 2025 22:41:59 +0200
> Loïc Molinari <loic.molinari@...labora.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@...labora.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@...labora.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?
It doesn't really matter here but I just thought it would be cleaner to
explicitly let just the drm_notice() because the compiler doesn't know
v3d->drm.huge_mnt is always NULL here and would emit a branch in
CONFIG_TRANSPARENT_HUGEPAGE=n builds. I know your dislike for this
pattern now, so I will stick to the suggestion :)
Loïc
Powered by blists - more mailing lists