On 31.01.22 20:30, Kirill A. Shutemov wrote:

On Mon, Jan 31, 2022 at 01:13:49PM +0100, David Hildenbrand wrote:

quoted

On 30.01.22 17:45, Kirill A. Shutemov wrote:

quoted

UEFI Specification version 2.9 introduces the concept of memory
acceptance. Some Virtual Machine platforms, such as Intel TDX or AMD
SEV-SNP, requiring memory to be accepted before it can be used by the
guest. Accepting happens via a protocol specific for the Virtual Machine
platform.

Accepting memory is costly and it makes VMM allocate memory for the
accepted guest physical address range. It's better to postpone memory
acceptance until memory is needed. It lowers boot time and reduces
memory overhead.

Support of such memory requires a few changes in core-mm code:

  - memblock has to accept memory on allocation;

  - page allocator has to accept memory on the first allocation of the
    page;

Memblock change is trivial.

The page allocator is modified to accept pages on the first allocation.
PageBuddyUnaccepted() is used to indicate that the page requires acceptance.

Kernel only need to accept memory once after boot, so during the boot
and warm up phase there will be a lot of memory acceptance. After things
are settled down the only price of the feature if couple of checks for
PageBuddyUnaccepted() in alloc and free paths. The check refers a hot
variable (that also encodes PageBuddy()), so it is cheap and not visible
on profiles.

Architecture has to provide three helpers if it wants to support
unaccepted memory:

 - accept_memory() makes a range of physical addresses accepted.

 - maybe_mark_page_unaccepted() marks a page PageBuddyUnaccepted() if it
   requires acceptance. Used during boot to put pages on free lists.

 - accept_page() makes a page accepted and clears PageBuddyUnaccepted().

Signed-off-by: Kirill A. Shutemov <redacted>
Acked-by: Mike Rapoport <redacted>	# memblock

You should somehow document+check+enforce that page poisoning cannot be
enabled concurrently, because it cannot possibly work IIUC.

Looking at code again, I now think that sharing the bit with PageOffline()
is wrong. Previously I convinced myself that there's no conflict on the
bit. In the initial version of the patchset, the page acceptance happened
inside del_page_from_free_list() so any removal from the free list lead to
clearing the bit. It is not the case now when acceptance moved to
post_alloc_hook(). __isolate_free_page() and __offline_isolated_pages()
look problematic now.

Both grab the zone lock. So as long as you'd perform the update of both
bits (PageOffline+PageBuddy) in one go under the zone lock, you could
handle it accordingly. But IIRC we don't want to accept memory while
holding the zone lock ...

Of course, you could clear the flag under the zone lock and forward the
requirement to prep_new_page(). For example, using alloc_flags.

We could have a new ALLOC_UNACCEPTED that will result in
prep_new_page()->post_alloc_hook() calling accept_page().

Relevant functions (e.g., rmqueue()) would consume *alloc_flags instead
of alloc_flags and simply clear+set the bit while updating *alloc_flags.

* __alloc_pages_bulk()->__rmqueue_pcplist() shouldn't need care because
  unaccepted pages shouldn't be on a pcp list (iow, previously
  allocated)
* Not sure if we'd have to touch try_to_compact_pages(), because we can
  only stumble over unnaccepted pages if these pages were never
  allocated, would require some thought.

So maybe it would boil down to rmqueue() only.

I will use brand new bit for the flag and rename BuddyUnaccepted to just
Unaccepted, since it can be set with Buddy cleared.

Sounds okay?

Fine with me, having something restricted to PageBuddy() might be
conceptually nicer, though.

[...]

quoted

You'll be setting the page as unaccepted even before it's actually
PageBuddy(). While that works, I wonder why we call
maybe_mark_page_unaccepted() at these points.

Why are we not moving that deeper into the buddy? __free_pages_core() is
used for any fresh pages that enter the buddy, used outside of
page_alloc.c only for memory hot(un)plug, so I'd suggest moving it at
least into there.

But maybe we'd even move it further down, to the place where we actually
establish PageBuddy().

One idea would be adding a new FPI_UNACCEPTED flag, passing it from
__free_pages_core() only, and calling maybe_mark_page_unaccepted() from
__free_one_page() after set_buddy_order().

If in-lining would do its job properly, we'd be left with the
FPI_UNACCEPTED checks only when called via __free_pages_core(), and we'd
have that call at a single place right where we mess with PageBuddy().

Okay, this approach looks neat. See fixup below.

But there's down side: maybe_mark_page_unaccepted() cannot be __init
anymore, since it is called from __free_one_page().

Good point, do we care?

Any comments?

LGTM

-- 
Thanks,

David / dhildenb