From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl0-f69.google.com (mail-pl0-f69.google.com [209.85.160.69]) by kanga.kvack.org (Postfix) with ESMTP id 766E86B0006 for ; Thu, 19 Jul 2018 10:13:56 -0400 (EDT) Received: by mail-pl0-f69.google.com with SMTP id cf17-v6so4702201plb.2 for ; Thu, 19 Jul 2018 07:13:56 -0700 (PDT) Received: from mga09.intel.com (mga09.intel.com. [134.134.136.24]) by mx.google.com with ESMTPS id n21-v6si5987661pgk.307.2018.07.19.07.13.55 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 19 Jul 2018 07:13:55 -0700 (PDT) Subject: Re: [PATCHv5 06/19] mm/khugepaged: Handle encrypted pages References: <20180717112029.42378-1-kirill.shutemov@linux.intel.com> <20180717112029.42378-7-kirill.shutemov@linux.intel.com> <20180719085901.ebdciqkjpx6hy4xt@kshutemo-mobl1> From: Dave Hansen Message-ID: Date: Thu, 19 Jul 2018 07:13:39 -0700 MIME-Version: 1.0 In-Reply-To: <20180719085901.ebdciqkjpx6hy4xt@kshutemo-mobl1> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: owner-linux-mm@kvack.org List-ID: To: "Kirill A. Shutemov" Cc: "Kirill A. Shutemov" , Ingo Molnar , x86@kernel.org, Thomas Gleixner , "H. Peter Anvin" , Tom Lendacky , Kai Huang , Jacob Pan , linux-kernel@vger.kernel.org, linux-mm@kvack.org On 07/19/2018 01:59 AM, Kirill A. Shutemov wrote: > On Wed, Jul 18, 2018 at 04:11:57PM -0700, Dave Hansen wrote: >> On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote: >>> khugepaged allocates page in advance, before we found a VMA for >>> collapse. We don't yet know which KeyID to use for the allocation. >> >> That's not really true. We have the VMA and the address in the caller >> (khugepaged_scan_pmd()), but we drop the lock and have to revalidate the >> VMA. > > For !NUMA we allocate the page in khugepaged_do_scan(), well before we > know VMA. Ahh, thanks for clarifying. That's some more very good information about the design and progression of your patch that belongs in the changelog. >>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c >>> index 5ae34097aed1..d116f4ebb622 100644 >>> --- a/mm/khugepaged.c >>> +++ b/mm/khugepaged.c >>> @@ -1056,6 +1056,16 @@ static void collapse_huge_page(struct mm_struct *mm, >>> */ >>> anon_vma_unlock_write(vma->anon_vma); >>> >>> + /* >>> + * At this point new_page is allocated as non-encrypted. >>> + * If VMA's KeyID is non-zero, we need to prepare it to be encrypted >>> + * before coping data. >>> + */ >>> + if (vma_keyid(vma)) { >>> + prep_encrypted_page(new_page, HPAGE_PMD_ORDER, >>> + vma_keyid(vma), false); >>> + } >> >> I guess this isn't horribly problematic now, but if we ever keep pools >> of preassigned-keyids, this won't work any more. > > I don't get this. What pools of preassigned-keyids are you talking about? My point was that if we ever teach the allocator or something _near_ the allocator to keep pools of pre-zeroed and/or pre-cache-cleared pages, this approach will need to get changed otherwise we will double-prep pages. My overall concern with prep_encrypted_page() in this patch set is that it's inserted pretty ad-hoc. It seems easy to miss spots where it should be. I'm also unsure of the failure mode and anything we've done to ensure that if we get this wrong, we scream clearly and loudly about what happened. Do we do something like that?