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

["Kirill A. Shutemov" <kirill@shutemov.name>]

On Thu, Aug 12, 2021 at 01:59:01PM -0700, Dave Hansen wrote:
> On 8/12/21 1:49 PM, Kirill A. Shutemov wrote:
> > On Thu, Aug 12, 2021 at 07:14:20AM -0700, Dave Hansen wrote:
> >> On 8/12/21 1:19 AM, Joerg Roedel wrote:
> >> 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.
> 
> The bitmap bouncing around isn't going to really matter when you have a
> global lock protecting it from writes.

The idea with static key would not work if we mark shared memory as
unaccepted there.

> > 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).
> 
> Two easy options:
> 1. Run over the bitmap and counts the bits left.  That can be done
>    outside the lock even.
> 2. Keep a counter of the number of bits set in the bitmap.
> 
> > I like PageOffline() because we only need to consult the cache page
> > allocator already have in hands before looking into bitmap.
> 
> I like it too.  But, it's really nasty if the value is only valid deep
> in the allocator.
> 
> We could keep the PageOffline() thing and then move it to some other
> field in 'struct page' that's only valid between ClearPageOffline() and
> prep_new_page().   Some magic value that says: "This page has not yet
> been accepted, you better check the bitmap."  Something like:
> 
> 	if (TestClearPageOffline(page))
> 		page->private = 0Xdeadbeef;
> 
> and then check page->private in prep_new_page(). There should be plenty
> of 'struct page' space to hijack in that small window.

PageOffline() encoded in mapcount and check_new_page_bad() would complain
is mapcount is not -1.

> BTW, I was going to actually try and hack something up, but I got
> annoyed that your patches don't apply upstream and gave up.  A git tree
> with all of the dependencies would be nice. <hint, hint>

Okay.

-- 
 Kirill A. Shutemov