linux-kernel - Re: [PATCH v2 2/2] mm,page_owner: Fix accounting of pages when migrating

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 21 Mar 2024 12:20:24 +0100
From: Vlastimil Babka <vbabka@...e.cz>
To: Oscar Salvador <osalvador@...e.de>
Cc: Matthew Wilcox <willy@...radead.org>,
 Andrew Morton <akpm@...ux-foundation.org>, linux-kernel@...r.kernel.org,
 linux-mm@...ck.org, Michal Hocko <mhocko@...e.com>,
 Marco Elver <elver@...gle.com>, Andrey Konovalov <andreyknvl@...il.com>,
 Alexander Potapenko <glider@...gle.com>,
 Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Subject: Re: [PATCH v2 2/2] mm,page_owner: Fix accounting of pages when
 migrating

On 3/21/24 12:07, Oscar Salvador wrote:
> On Thu, Mar 21, 2024 at 11:50:36AM +0100, Vlastimil Babka wrote:
>> Yeah I think we could keep that logic.
> 
> I am all for keeping it.
> 
>> But we could also simply subtract the refcount of the old handle (the
>> "allocated for migration") in __folio_copy_owner() no? Then we wouldn't need
>> the extra migrate_handle.
> 
> Since new_page will have the old handle pointing to the old stack after
> the call, we
> could uncharge the old_page to the migrate_stack, which new_page->_handle holds
> before it gets changed.
> 
> So we basically swap it.
> 
> It could work, but I kinda have a bittersweet feeling here.
> I am trying to work towards to reduce the number of lookups in the
> hlist, but for the approach described above I would need to lookup
> the stack for new_page->handle in order to substract the page.

Understood, but migration is kinda heavy and non-fast-path operation already
so the extra lookup wouldn't be in a critical fast path.

> OTHO, I understand that adding migrate_handle kinda wasted memory.
> 16MB for 16GB of memory.
> 
>> Also we might have more issues here. Most page owner code takes care to set
>> everything for all pages within a folio, but __folio_copy_owner() and
>> __set_page_owner_migrate_reason() don't.
> 
> I did not check deeply but do not we split the folio upon migration
> in case it is large?
> Which means we should reach split_page_owner() before the copy takes
> place. Do I get it right?

When I mean is we have __set_page_owner_handle() setting up everything for
tail pages and then we have __folio_copy_owner updating only the head page,
so this will create kinda a mixed up information. Which might not be an
issue as __folio_copy_owner() should mean it's a proper folio thus compound
page thus nobody ever will check those mismatched tail pages... Maybe we
could adjust  __set_page_owner_handle() to skip tail pages for compound
pages as well and unify this, and tail pages would be only setup for those
weird high-order non-compound pages so that the odd case in __free_pages()
works?

Or maybe page_owner is considered debugging functionality enough so it might
be worth having the redundant data in tail pages in case something goes
wrong. But either way now it seems we're not doing it consistently.