| Message ID | 20210405232653.33680-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] x86/sgx: Do not update sgx_nr_free_pages in sgx_setup_epc_section() | expand |
On Tue, Apr 06, 2021 at 02:26:52AM +0300, Jarkko Sakkinen wrote: > Now that the sanitization process will make pages available by calling > sgx_free_epc_page(), sgx_setup_epc_section() should not touch to > sgx_nr_free_pages. This will result sgx_nr_free_pages to contain 2x the > number of actual free pages. Simply, remove the statement. > > Fixes: 51ab30eb2ad4 ("x86/sgx: Replace section->init_laundry_list with sgx_dirty_page_list") > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > --- > arch/x86/kernel/cpu/sgx/main.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index 13a7599ce7d4..7df7048cb1c9 100644 > --- a/arch/x86/kernel/cpu/sgx/main.c > +++ b/arch/x86/kernel/cpu/sgx/main.c > @@ -657,7 +657,6 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, > list_add_tail(§ion->pages[i].list, &sgx_dirty_page_list); > } > > - sgx_nr_free_pages += nr_pages; > return true; > } > First of all, I don't know how I didn't catch this: /* The free page list lock protected variables prepend the lock. */ static unsigned long sgx_nr_free_pages; I need some sort of translator to understand what this comment means. I can guess what is trying to tell me. Which leads to my question: what is sgx_nr_free_pages supposed to denote? Because I understand the callpath sgx_page_cache_init ... for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) { ... sgx_setup_epc_section ... sgx_nr_free_pages += nr_pages; as adding the number of pages of each new EPC section to the total number of the free pages. Unless that variable accounts something else. So what does this variable actually mean?
On Wed, Apr 07, 2021 at 05:49:34PM +0200, Borislav Petkov wrote: > On Tue, Apr 06, 2021 at 02:26:52AM +0300, Jarkko Sakkinen wrote: > > Now that the sanitization process will make pages available by calling > > sgx_free_epc_page(), sgx_setup_epc_section() should not touch to > > sgx_nr_free_pages. This will result sgx_nr_free_pages to contain 2x the > > number of actual free pages. Simply, remove the statement. > > > > Fixes: 51ab30eb2ad4 ("x86/sgx: Replace section->init_laundry_list with sgx_dirty_page_list") > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > --- > > arch/x86/kernel/cpu/sgx/main.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > index 13a7599ce7d4..7df7048cb1c9 100644 > > --- a/arch/x86/kernel/cpu/sgx/main.c > > +++ b/arch/x86/kernel/cpu/sgx/main.c > > @@ -657,7 +657,6 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, > > list_add_tail(§ion->pages[i].list, &sgx_dirty_page_list); > > } > > > > - sgx_nr_free_pages += nr_pages; > > return true; > > } > > > > First of all, I don't know how I didn't catch this: > > /* The free page list lock protected variables prepend the lock. */ > static unsigned long sgx_nr_free_pages; > > I need some sort of translator to understand what this comment means. I > can guess what is trying to tell me. > > Which leads to my question: what is sgx_nr_free_pages supposed to denote? > > Because I understand the callpath > > sgx_page_cache_init > ... > for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) { > ... > sgx_setup_epc_section > ... > sgx_nr_free_pages += nr_pages; > > as adding the number of pages of each new EPC section to the total > number of the free pages. Unless that variable accounts something else. > > So what does this variable actually mean? It's used for only to trigger watermark for reclaiming. I.e. causes ksgxd to trigger. And it gives the number of total free EPC pages in all NUMA nodes. /Jarkko
On Wed, Apr 07, 2021 at 07:03:47PM +0300, Jarkko Sakkinen wrote: > > Which leads to my question: what is sgx_nr_free_pages supposed to denote? > > > > Because I understand the callpath > > > > sgx_page_cache_init > > ... > > for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) { > > ... > > sgx_setup_epc_section > > ... > > sgx_nr_free_pages += nr_pages; > > > > as adding the number of pages of each new EPC section to the total > > number of the free pages. Unless that variable accounts something else. > > > > So what does this variable actually mean? > > It's used for only to trigger watermark for reclaiming. I.e. causes > ksgxd to trigger. And it gives the number of total free EPC pages in > all NUMA nodes. So the callpath I laid out above is adding the number of pages of each section to the total free EPC pages number. Why is that wrong and why is your patch needed?
On Wed, Apr 07, 2021 at 06:18:11PM +0200, Borislav Petkov wrote: > On Wed, Apr 07, 2021 at 07:03:47PM +0300, Jarkko Sakkinen wrote: > > > Which leads to my question: what is sgx_nr_free_pages supposed to denote? > > > > > > Because I understand the callpath > > > > > > sgx_page_cache_init > > > ... > > > for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) { > > > ... > > > sgx_setup_epc_section > > > ... > > > sgx_nr_free_pages += nr_pages; > > > > > > as adding the number of pages of each new EPC section to the total > > > number of the free pages. Unless that variable accounts something else. > > > > > > So what does this variable actually mean? > > > > It's used for only to trigger watermark for reclaiming. I.e. causes > > ksgxd to trigger. And it gives the number of total free EPC pages in > > all NUMA nodes. > > So the callpath I laid out above is adding the number of pages of each > section to the total free EPC pages number. > > Why is that wrong and why is your patch needed? As part of "x86/sgx: Replace section->init_laundry_list with sgx_dirty_page_list" pages are processed from a global list by ksgxd. This in turn introduces change to sanitization: - if (!ret) - list_move(&page->list, §ion->page_list); - else + if (!ret) { + /* + * page is now sanitized. Make it available via the SGX + * page allocator: + */ + list_del(&page->list); + sgx_free_epc_page(page); + } else { + /* The page is not yet clean - move to the dirty list. */ list_move_tail(&page->list, &dirty); - - spin_unlock(§ion->lock); + } This is done for the reason that it is best to keep the logic to assign available-for-use EPC pages to correct NUMA lists in a single location. The regression is that the sgx_nr_free_pages is also incremented by sgx_free_epc_pages(), and thus it ends up having double the number of pages available. /Jarkko
On Thu, Apr 08, 2021 at 11:48:46AM +0300, Jarkko Sakkinen wrote: > The regression is that the sgx_nr_free_pages is also incremented by > sgx_free_epc_pages(), and thus it ends up having double the number of > pages available. So when you add a new EPC section with sgx_setup_epc_section(), those new pages in "nr_pages" are initially not going to be accounted anywhere? Or is that sgx_nr_all_pages? And you do that in your second patch... But those new pages coming in *are* free pages so they should be in the free pages count too, IMHO.
On Thu, Apr 08, 2021 at 10:56:40AM +0200, Borislav Petkov wrote: > On Thu, Apr 08, 2021 at 11:48:46AM +0300, Jarkko Sakkinen wrote: > > The regression is that the sgx_nr_free_pages is also incremented by > > sgx_free_epc_pages(), and thus it ends up having double the number of > > pages available. > > So when you add a new EPC section with sgx_setup_epc_section(), those > new pages in "nr_pages" are initially not going to be accounted > anywhere? Or is that sgx_nr_all_pages? And you do that in your second > patch... > > But those new pages coming in *are* free pages so they should be in the > free pages count too, IMHO. They are not in the "free_page_list" before sanitization process has put them to there. So in that way the count is also better in sync with this fix. It was even before NUMA patches kind of out-of-sync, i.e. free pages count was incremented before putting them to the free list, but it didn't matter that much because sanitization is fairly fast and it only prevented ksgxd to trigger small time after the booth. I think I'll send the fix for this now as a separate patch with a better descriptino so that I can use more time to the attributes and implement that properly at once. > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette /Jarkko
On Thu, Apr 08, 2021 at 12:22:56PM +0300, Jarkko Sakkinen wrote: > They are not in the "free_page_list" before sanitization process has put > them to there. So in that way the count is also better in sync with this > fix. This is the bit of information I was looking for. This needs to be in the commit message.
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index 13a7599ce7d4..7df7048cb1c9 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -657,7 +657,6 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, list_add_tail(§ion->pages[i].list, &sgx_dirty_page_list); } - sgx_nr_free_pages += nr_pages; return true; }
Now that the sanitization process will make pages available by calling sgx_free_epc_page(), sgx_setup_epc_section() should not touch to sgx_nr_free_pages. This will result sgx_nr_free_pages to contain 2x the number of actual free pages. Simply, remove the statement. Fixes: 51ab30eb2ad4 ("x86/sgx: Replace section->init_laundry_list with sgx_dirty_page_list") Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- arch/x86/kernel/cpu/sgx/main.c | 1 - 1 file changed, 1 deletion(-)