Re: [PATCHv2 1/7] mm: Add support for unaccepted memory
From: Dave Hansen
Date: Wed Jan 12 2022 - 13:47:13 EST
On 1/12/22 10:30, Kirill A. Shutemov wrote:
On Tue, Jan 11, 2022 at 11:46:37AM -0800, Dave Hansen wrote:
diff --git a/mm/memblock.c b/mm/memblock.c
index 1018e50566f3..6dfa594192de 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1400,6 +1400,7 @@ phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size,
*/
kmemleak_alloc_phys(found, size, 0, 0);
+ accept_memory(found, found + size);
return found;
}
This could use a comment.
How about this:
/*
* 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.
*
* Accept the memory of the allocated buffer.
*/
I think a one-liner that might cue the reader to go look at
accept_memory() itself would be fine. Maybe:
/* Make the memblock usable when running in picky VM guests: */
That implies that the memory isn't usable without doing this and also
points out that it's related to running in a guest.
Looking at this, I also have to wonder if accept_memory() is a bit too
generic. Should it perhaps be: cc_accept_memory() or
cc_guest_accept_memory()?
I'll rename accept_memory() to cc_accept_memory() and
accept_and_clear_page_offline() to cc_accept_and_clear_page_offline().
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c5952749ad40..5707b4b5f774 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1064,6 +1064,7 @@ static inline void __free_one_page(struct page *page,
unsigned int max_order;
struct page *buddy;
bool to_tail;
+ bool offline = PageOffline(page);
max_order = min_t(unsigned int, MAX_ORDER - 1, pageblock_order);
@@ -1097,6 +1098,10 @@ static inline void __free_one_page(struct page *page,
clear_page_guard(zone, buddy, order, migratetype);
else
del_page_from_free_list(buddy, zone, order);
+
+ if (PageOffline(buddy))
+ offline = true;
+
combined_pfn = buddy_pfn & pfn;
page = page + (combined_pfn - pfn);
pfn = combined_pfn;
@@ -1130,6 +1135,9 @@ static inline void __free_one_page(struct page *page,
done_merging:
set_buddy_order(page, order);
+ if (offline)
+ __SetPageOffline(page);
+
I'll add
/* Mark page PageOffline() if any merged page was PageOffline() */
above the 'if'.
if (fpi_flags & FPI_TO_TAIL)
to_tail = true;
else if (is_shuffle_order(order))
This is touching some pretty hot code paths. You mention both that
accepting memory is slow and expensive, yet you're doing it in the core
allocator.
That needs at least some discussion in the changelog.
That is page type transfer on page merging. What expensive do you see here?
The cachelines with both struct pages are hot already.
I meant that comment generically rather than at this specific hunk.
Just in general, I think this series needs to acknowledge that it is
touching very core parts of the allocator and might make page allocation
*MASSIVELY* slower, albeit temporarily.
@@ -1155,7 +1163,8 @@ static inline void __free_one_page(struct page *page,
static inline bool page_expected_state(struct page *page,
unsigned long check_flags)
{
- if (unlikely(atomic_read(&page->_mapcount) != -1))
+ if (unlikely(atomic_read(&page->_mapcount) != -1) &&
+ !PageOffline(page))
return false;
Looking at stuff like this, I can't help but think that a:
#define PageOffline PageUnaccepted
and some other renaming would be a fine idea. I get that the Offline bit
can be reused, but I'm not sure that the "Offline" *naming* should be
reused. What you're doing here is logically distinct from existing
offlining.
I find the Offline name fitting. In both cases page is not accessible
without additional preparation.
Why do you want to multiply entities?
The name wouldn't be bad *if* there was no other use of "Offline". But,
logically, your use of "Offline" and the existing use of "Offline" are
different things. They are totally orthogonal areas of the code. They
should have different names.
Again, I'm fine with using the same _bit_ in page->flags. But, the two
logical uses need two different names.