Re: [PATCH v3 3/5] x86/sgx: Optimize the free_cnt count in sgx_epc_section

From: Tianjia Zhang
Date: Mon Feb 15 2021 - 22:32:40 EST



On 2/12/21 8:19 PM, Jarkko Sakkinen wrote:
On Thu, Feb 11, 2021 at 02:04:12PM +0800, Tianjia Zhang wrote:
Hi,

Sorry for the late reply.

On 1/28/21 1:40 AM, Jarkko Sakkinen wrote:
I could bet some money that this does not bring any significant
performance gain.


Yes, this does not bring performance gains. This is not a change for
performance, mainly to make the value of free_cnt look more accurate.

On Sun, Jan 24, 2021 at 02:29:05PM +0800, Tianjia Zhang wrote:
`section->free_cnt` represents the free page in sgx_epc_section,
which is assigned once after initialization. In fact, just after the
initialization is completed, the pages are in the `init_laundry_list`
list and cannot be allocated. This needs to be recovered by EREMOVE
of function sgx_sanitize_section() before it can be used as a page
that can be allocated. The sgx_sanitize_section() will be called in
the kernel thread ksgxd.

This patch moves the initialization of `section->free_cnt` from the
initialization function `sgx_setup_epc_section()` to the function
`sgx_sanitize_section()`, and then accumulates the count after the

Use single quotes instead of hyphens.
successful execution of EREMOVE. This seems to be more reasonable,
free_cnt will also truly reflect the allocatable free pages in EPC.

Sined-off-by: Tianjia Zhang <tianjia.zhang@xxxxxxxxxxxxxxxxx>
Reviewed-by: Sean Christopherson <seanjc@xxxxxxxxxx>
---
arch/x86/kernel/cpu/sgx/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 4465912174fd..e455ec7b3449 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -48,6 +48,7 @@ static void sgx_sanitize_section(struct sgx_epc_section *section)
if (!ret) {
spin_lock(&section->lock);
list_move(&page->list, &section->page_list);
+ section->free_cnt++;
spin_unlock(&section->lock);

Someone can try to allocate a page while sanitize process is in progress.

I think it is better to keep critical sections in the form that when you
leave from one, the global state is legit.


Do you mean to move the critical section to protect the entire while loop?
Of course, this is also possible, sanitize is a process only needed for
initialization, and the possibility of conflict is very small.

Best regards,
Tianjia

The big picture of this change to me, to be frank is that it's completely
useless.

Please start with the picture.

/Jarkko


I carefully considered your suggestion, and I will delete 2/5 and 3/5 in the next version.

Best regards,
Tianjia