Re: [PATCHv4 1/8] mm: Add support for unaccepted memory

[Dave Hansen <dave.hansen@intel.com>]

On 4/9/22 08:54, Kirill A. Shutemov wrote:
> On Fri, Apr 08, 2022 at 11:55:43AM -0700, Dave Hansen wrote:
>>> The page allocator is modified to accept pages on the first allocation.
>>> PageUnaccepted() is used to indicate that the page requires acceptance.
>>
>> Does this consume an actual page flag or is it aliased?
> 
> It is encoded as a page type in mapcount of unallocated memory. It is not
> aliased with PageOffline() as I did before.
> 
> I will mention that it is a new page type.

Guess I should have looked at the code. :)

Are we just increasingly using the StudlyNames() for anything to do with
pages?

>>> + /*
>>> +  * PageUnaccepted() indicates that the page has to be "accepted" before it can
>>> +  * be used. Page allocator has to call accept_page() before returning the page
>>> +  * to the caller.
>>> +  */
>>
>> Let's talk about "used" with a bit more detail.  Maybe:
>>
>> /*
>>  * PageUnaccepted() indicates that the page has to be "accepted" before
>>  * it can be read or written.  The page allocator must to call
>>  * accept_page() before touching the page or returning it to the caller.
>>  */
> 
> I guess s/must to call/must call/, right?

Yep.

...
>>> +	/*
>>> +	 * Check if the page needs to be marked as PageUnaccepted().
>>> +	 * Used for the new pages added to the buddy allocator for the first
>>> +	 * time.
>>> +	 */
>>> +	if (!unaccepted && (fpi_flags & FPI_UNACCEPTED))
>>> +		unaccepted = page_is_unaccepted(page, order);
>>
>> 	if (page_needs_acceptance && (fpi_flags & FPI_UNACCEPTED))
>> 		page_needs_acceptance = page_is_unaccepted(page, order);
>>
>>> +	if (unaccepted)
>>> +		__SetPageUnaccepted(page);
>>
>> This is getting hard for me to follow.
>>
>> There are:
>> 1. Pages that come in here with PageUnaccepted()==1
>> 2. Pages that come in here with PageUnaccepted()==0, but a buddy that
>>    was PageUnaccepted()==1
>>
>> In either of those cases, the bitmap will be consulted to see if the
>> page is *truly* unaccepted or not.  But, I'm struggling to figure out
>> how a page could end up in one of those scenarios and *not* be
>> page_is_unaccepted().
>>
>> There are three pieces of information that come in:
>> 1. PageUnaccepted(page)
>> 2. PageUnaccepted(buddies[])
>> 3. the bitmap
> 
> 1 and 2 are the same conceptionally: merged-in pieces of the resulting page.
> 
>>
>> and one piece of information going out:
>>
>> PageUnaccepted(page);
>>
>> I think I need a more coherent description of how those four things fit
>> together.
> 
> The page gets marked as PageUnaccepted() if any of merged-in pages is
> PageUnaccepted().
> 
> For new pages, just being added to buddy allocator, consult
> page_is_unaccepted(). FPI_UNACCEPTED indicates that the page is new and
> page_is_unaccepted() check is required.
> 
> Avoid calling page_is_unaccepted() if it is known that the page needs
> acceptance anyway. It can be costly.
> 
> Is it good enough explanation?

Yeah, elaborating on the slow and fast paths makes a lot of sense.

> FPI_UNACCEPTED is not a good name. Any help with a better one?
> FPI_CHECK_UNACCEPTED?

Maybe even something like FPI_UNACCEPTED_SLOWPATH.  Then you can say
that when this is passed in the pages might not have PageUnaccepted()
set and the slow bitmap needs to be consulted.

>>>  	if (fpi_flags & FPI_TO_TAIL)
>>>  		to_tail = true;
>>>  	else if (is_shuffle_order(order))
>>> @@ -1149,7 +1192,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) &&
>>> +	    !PageUnaccepted(page))
>>>  		return false;
>>
>> That probably deserves a comment, and maybe its own if() statement.
> 
> Own if does not work. PageUnaccepted() is encoded in _mapcount.
> 
> What about this:
> 
> 	/*
> 	 * page->_mapcount is expected to be -1.
> 	 *
> 	 * There is an exception for PageUnaccepted(). The page type can be set
> 	 * for pages on free list. Page types are encoded in _mapcount.
> 	 *
> 	 * PageUnaccepted() will get cleared in post_alloc_hook().
> 	 */
> 	if (unlikely((atomic_read(&page->_mapcount) | PG_unaccepted) != -1))
> 		return false;
> 
> ?

That's better.  But, aren't the PG_* names usually reserved for real
page->flags bits?  That naming might be part of my confusion.

>>>  		add_to_free_list(&page[size], zone, high, migratetype);
>>>  		set_buddy_order(&page[size], high);
>>>  	}
>>> @@ -2396,6 +2446,9 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
>>>  	 */
>>>  	kernel_unpoison_pages(page, 1 << order);
>>>  
>>> +	if (PageUnaccepted(page))
>>> +		accept_page(page, order);
>>> +
>>>  	/*
>>>  	 * As memory initialization might be integrated into KASAN,
>>>  	 * KASAN unpoisoning and memory initializion code must be
>>
>> Is accepted memory guaranteed to be zeroed?  Do we want to skip the
>> __GFP_ZERO behavior later in this function?  Or is that just a silly
>> over-optimization?
> 
> For TDX, it is true that the memory gets cleared on acceptance, but I
> don't we can say the same for any possible implementation.
> 
> I would rather leave __GFP_ZERO for peace of mind. Clearing the cache-hot
> page for the second time shouldn't be a big deal comparing to acceptance
> cost.

Sure, fair enough.