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.
