Re: [PATCH 1/5] mm: Add support for unaccepted memory

[Kirill A. Shutemov]

On Thu, Aug 12, 2021 at 07:14:20AM -0700, Dave Hansen wrote:
> On 8/12/21 1:19 AM, Joerg Roedel wrote:
> > On Tue, Aug 10, 2021 at 02:20:08PM -0700, Andi Kleen wrote:
> >> Also I agree with your suggestion that we should get the slow path out of
> >> the zone locks/interrupt disable region. That should be easy enough and is
> >> an obvious improvement.
> > 
> > I also agree that the slow-path needs to be outside of the memory
> > allocator locks. But I think this conflicts with the concept of
> > accepting memory in 2MB chunks even if allocation size is smaller.
> > 
> > Given some kernel code allocated 2 pages and the allocator path starts
> > to validate the whole 2MB page the memory is on, then there are
> > potential races to take into account.
> 
> Yeah, the PageOffline()+PageBuddy() trick breaks down as soon as
> PageBuddy() gets cleared.
> 
> I'm not 100% sure we need a page flag, though.  Imagine if we just did a
> static key check in prep_new_page():
> 
> 	if (static_key_whatever(tdx_accept_ongoing))
> 		maybe_accept_page(page, order);
> 
> maybe_accept_page() could just check the acceptance bitmap and see if
> the 2MB page has been accepted.  If so, just return.  If not, take the
> bitmap lock, accept the 2MB page, then mark the bitmap.
> 
> maybe_accept_page()
> {
> 	unsigned long huge_pfn = page_to_phys(page) / PMD_SIZE;
> 
> 	/* Test the bit before taking any locks: */
> 	if (test_bit(huge_pfn, &accepted_bitmap))
> 		return;
> 
> 	spin_lock_irq();
> 	/* Retest inside the lock: */
> 	if (test_bit(huge_pfn, &accepted_bitmap))
> 		return;
> 	tdx_accept_page(page, PMD_SIZE);
> 	set_bit(huge_pfn, &accepted_bitmap));
> 	spin_unlock_irq();
> }
> 
> That's still not great.  It's still a global lock and the lock is still
> held for quite a while because that accept isn't going to be lightning
> fast.  But, at least it's not holding any *other* locks.  It also
> doesn't take any locks in the fast path where the 2MB page was already
> accepted.

I expect a cache line with bitmap to bounce around during warm up. One
cache line covers a gig of RAM.

It's also not clear at all at what point the static key has to be
switched. We don't have any obvious point where we can say we are done
with accepting (unless you advocate for proactive accepting).

I like PageOffline() because we only need to consult the cache page
allocator already have in hands before looking into bitmap.

> The locking could be more fine-grained, for sure.  The bitmap could, for
> instance, have a lock bit too.  Or we could just have an array of locks
> and hash the huge_pfn to find a lock given a huge_pfn.  But, for now, I
> think it's fine to just keep the global lock.
> 
> > Either some other code path allocates memory from that page and returns
> > it before validation is finished or we end up with double validation.
> > Returning unvalidated memory is a guest-problem and double validation
> > will cause security issues for SNP guests.
> 
> Yeah, I think the *canonical* source of information for accepts is the
> bitmap.  The page flags and any static keys or whatever are
> less-canonical sources that tell you when you _might_ need to consult
> the bitmap.

Right.

-- 
 Kirill A. Shutemov