| 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: <7ac06a41-73c8-4089-873e-7bb4cc1b3e02@lucifer.local>
Date: Sat, 24 Jan 2026 11:40:21 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Dev Jain <dev.jain@....com>
Cc: "Garg, Shivank" <shivankg@....com>,
Andrew Morton <akpm@...ux-foundation.org>,
David Hildenbrand <david@...nel.org>, Zi Yan <ziy@...dia.com>,
Baolin Wang <baolin.wang@...ux.alibaba.com>,
"Liam R . Howlett" <Liam.Howlett@...cle.com>,
Nico Pache <npache@...hat.com>, Ryan Roberts <ryan.roberts@....com>,
Barry Song <baohua@...nel.org>, Lance Yang <lance.yang@...ux.dev>,
linux-mm@...ck.org, linux-kernel@...r.kernel.org,
Wei Yang <richard.weiyang@...il.com>,
Anshuman Khandual <anshuman.khandual@....com>
Subject: Re: [PATCH V3 5/5] mm/khugepaged: make khugepaged_collapse_control
static
On Sat, Jan 24, 2026 at 04:24:24PM +0530, Dev Jain wrote:
>
> On 24/01/26 2:31 pm, Lorenzo Stoakes wrote:
> > NAK to this change....
> >
> > On Fri, Jan 23, 2026 at 03:03:58PM +0530, Garg, Shivank wrote:
> >>
> >> On 1/23/2026 1:18 PM, Dev Jain wrote:
> >>> On 22/01/26 2:58 pm, Dev Jain wrote:
> >>>> On 19/01/26 12:53 am, Shivank Garg wrote:
> >>>>> The global variable 'khugepaged_collapse_control' is not used outside of
> >>>>> mm/khugepaged.c. Make it static to limit its scope.
> >>>>>
> >>>>> Reviewed-by: Wei Yang <richard.weiyang@...il.com>
> >>>>> Reviewed-by: Zi Yan <ziy@...dia.com>
> >>>>> Acked-by: David Hildenbrand (Red Hat) <david@...nel.org>
> >>>>> Reviewed-by: Anshuman Khandual <anshuman.khandual@....com>
> >>>>> Signed-off-by: Shivank Garg <shivankg@....com>
> >>>>> ---
> >>>>> mm/khugepaged.c | 2 +-
> >>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> >>>>> index 1667abae6d8d..fba6aea5bea6 100644
> >>>>> --- a/mm/khugepaged.c
> >>>>> +++ b/mm/khugepaged.c
> >>>>> @@ -827,7 +827,7 @@ static void khugepaged_alloc_sleep(void)
> >>>>> remove_wait_queue(&khugepaged_wait, &wait);
> >>>>> }
> >>>>>
> >>>>> -struct collapse_control khugepaged_collapse_control = {
> >>>>> +static struct collapse_control khugepaged_collapse_control = {
> >>>>> .is_khugepaged = true,
> >>>>> };
> >>>>>
> >>>> Will it not be better to just remove this variable? In madvise_collapse,
> >>>> we defined cc as a local variable and set .is_khugepaged = false. The
> >>>> same can be done in int khugepaged() - define a local variable and set
> >>>> .is_khugepaged = true.
> >>> Since this patch has been stabilized already by 4 R-bs, it may be a headache
> >>> to now remove this, we can do my suggestion later.
> >>>
> >>> Reviewed-by: Dev Jain <dev.jain@....com>
> >>>
> >>>>
> >> Thank you Dev for the feedback and review.
> >>
> >> I've attached the patch implementing your suggestion and sending this as a separate
> >> follow-up to avoid disrupting the current series.
> >>
> >> I’m happy to queue it for next cycle or if it’s acceptable now, please take it.
> >>
> >> Thanks for the suggestion!
> >>
> >> Regards,
> >> Shivank
> >>
> >> ---
> >> From: Shivank Garg <shivankg@....com>
> >> Date: Thu, 22 Jan 2026 12:36:28 +0000
> >> Subject: [PATCH] mm/khugepaged: convert khugepaged_collapse_control to local
> >> variable in khugepaged()
> >>
> >> Make khugepaged_collapse_control a local variable in khugepaged() instead
> >> of static global, consistent with how madvise_collapse() handles its
> >> collapse_control. Static storage is unnecessary here as node_load and
> >> alloc_nmask are reset per-VMA during scanning.
> >>
> >> No functional change.
> >>
> >> Suggested-by: Dev Jain <dev.jain@....com>
> >> Signed-off-by: Shivank Garg <shivankg@....com>
> >> ---
> >> mm/khugepaged.c | 9 ++++-----
> >> 1 file changed, 4 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> >> index 9f790ec34400..c18d2ce639b1 100644
> >> --- a/mm/khugepaged.c
> >> +++ b/mm/khugepaged.c
> >> @@ -829,10 +829,6 @@ static void khugepaged_alloc_sleep(void)
> >> remove_wait_queue(&khugepaged_wait, &wait);
> >> }
> >>
> >> -static struct collapse_control khugepaged_collapse_control = {
> >> - .is_khugepaged = true,
> >> -};
> >> -
> >> static bool hpage_collapse_scan_abort(int nid, struct collapse_control *cc)
> >> {
> >> int i;
> >> @@ -2629,13 +2625,16 @@ static void khugepaged_wait_work(void)
> >>
> >> static int khugepaged(void *none)
> >> {
> >> + struct collapse_control cc = {
> >> + .is_khugepaged = true,
> >> + };
> >> struct mm_slot *slot;
> >>
> >> set_freezable();
> >> set_user_nice(current, MAX_NICE);
> >>
> >> while (!kthread_should_stop()) {
> >> - khugepaged_do_scan(&khugepaged_collapse_control);
> >> + khugepaged_do_scan(&cc);
> >> khugepaged_wait_work();
> >> }
> >>
> >> --
> >> 2.43.0
> >>
> >>
> >>
> >>
> > Andrew's already commented but this is terribly mistaken.
> >
> > The argument against it (why did nobody check...) is that this struct is HUGE
> > and there's really no benefit to doing this.
> >
> > Nico's series makes this struct even bigger (...!)
> >
> > Dev - PLEASE use pahole or sizeof(...) or something before suggesting moving
> > things like this on to the stack, in future e.g.:
> >
> > $ pahole collapse_control
> > struct collapse_control {
> > bool is_khugepaged; /* 0 1 */
> >
> > /* XXX 3 bytes hole, try to pack */
> >
> > u32 node_load[1024]; /* 4 4096 */
> >
> > /* XXX 4 bytes hole, try to pack */
> >
> > /* --- cacheline 64 boundary (4096 bytes) was 8 bytes ago --- */
> > nodemask_t alloc_nmask; /* 4104 128 */
> >
> > /* size: 4232, cachelines: 67, members: 3 */
> > /* sum members: 4225, holes: 2, sum holes: 7 */
> > /* last cacheline: 8 bytes */
> > };
> >
> > Making this static was fine. Leave it as-is.
>
> I wasn't suggesting that! When I said
>
> "In madvise_collapse, we defined cc as a local variable and set .is_khugepaged = false. The
> same can be done in int khugepaged() - define a local variable and set .is_khugepaged = true."
Yeah I would suggest more precision in your language in future :) 'as a local
variable' and set . not -> is_khugepaged true... I can see why Shivank
interpreted it at as a stack variable.
>
> madvise_collapse does kmalloc() to allocate this large struct. I was suggesting to do the
> same for khugepaged, to enforce consistency.
As I said in reply to Andrew, NAK to the kmalloc idea too.
This consistency argument is nonsense, madvise_collapse() does that because
_there can be multiple instances_ of the cc around for different processes, you
literally _have_ to kmalloc there.
For khugepaged this isn't the case. We're good as we are.
Thanks, Lorenzo
Powered by blists - more mailing lists