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
