| Message ID | 20250321123938.802763-3-elena.reshetova@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable automatic SVN updates for SGX enclaves | expand |
On Fri, Mar 21, 2025 at 02:34:41PM +0200, Elena Reshetova wrote: > sgx_nr_free_pages is an atomic that is used to keep track of > free EPC pages and detect whenever page reclaiming should start. > Since successful execution of ENCLS[EUPDATESVN] requires empty > EPC and a fast way of checking for this, change this variable > around to indicate number of used pages instead. The subsequent > patch that introduces ENCLS[EUPDATESVN] will take use of this change. s/subsequent patch// You should rather express how EUPDATESVN trigger will depend on the state of sgx_nr_used_pages and sgx_nr_free_pages. > > No functional changes intended. Not really understanding how I should interpret this sentence. The commit message does not mention sgx_nr_used_pages, and neiher it makes a case why implementing the feature based on sgx_nr_free_pages is not possible. BR, Jarkko
> On Fri, Mar 21, 2025 at 02:34:41PM +0200, Elena Reshetova wrote: > > sgx_nr_free_pages is an atomic that is used to keep track of > > free EPC pages and detect whenever page reclaiming should start. > > Since successful execution of ENCLS[EUPDATESVN] requires empty > > EPC and a fast way of checking for this, change this variable > > around to indicate number of used pages instead. The subsequent > > patch that introduces ENCLS[EUPDATESVN] will take use of this change. > > s/subsequent patch// Ok > > You should rather express how EUPDATESVN trigger will depend on the > state of sgx_nr_used_pages and sgx_nr_free_pages. How about this explanation: "By counting the # of used pages instead of #of free pages, it allows the EPC page allocation path execute without a need to take the lock in all but a single case when the first page is being allocated in EPC. This is achieved via a fast check in atomic_long_inc_not_zero." Also, if you think that it is hard to interpret the patch 2/4 without 4/4 I can also squeeze them together and then it becomes right away clear why the change was done. > > > > > No functional changes intended. > > Not really understanding how I should interpret this sentence. Just as usual: this patch itself doesn’t bring any functional changes to the way as current SGX code works. I only needed this change to implement patch 4/4 in more lockless way. > > The commit message does not mention sgx_nr_used_pages, and neiher it > makes a case why implementing the feature based on sgx_nr_free_pages is > not possible. It is possible to implement it, in fact I did exactly this in the beginning instead, but as mentioned previously this would have resulted in taking a lock for each case the page is being allocated. Best Regards, Elena.
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index d5df67dab247..b61d3bad0446 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -32,7 +32,7 @@ static DEFINE_XARRAY(sgx_epc_address_space); static LIST_HEAD(sgx_active_page_list); static DEFINE_SPINLOCK(sgx_reclaimer_lock); -static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0); +static atomic_long_t sgx_nr_used_pages = ATOMIC_LONG_INIT(0); static unsigned long sgx_nr_total_pages; /* Nodes with one or more EPC sections. */ @@ -379,8 +379,8 @@ static void sgx_reclaim_pages(void) static bool sgx_should_reclaim(unsigned long watermark) { - return atomic_long_read(&sgx_nr_free_pages) < watermark && - !list_empty(&sgx_active_page_list); + return (sgx_nr_total_pages - atomic_long_read(&sgx_nr_used_pages)) + < watermark && !list_empty(&sgx_active_page_list); } /* @@ -457,7 +457,7 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid) page->flags = 0; spin_unlock(&node->lock); - atomic_long_dec(&sgx_nr_free_pages); + atomic_long_inc(&sgx_nr_used_pages); return page; } @@ -617,7 +617,7 @@ void sgx_free_epc_page(struct sgx_epc_page *page) page->flags = SGX_EPC_PAGE_IS_FREE; spin_unlock(&node->lock); - atomic_long_inc(&sgx_nr_free_pages); + atomic_long_dec(&sgx_nr_used_pages); } static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, @@ -851,6 +851,8 @@ static bool __init sgx_page_cache_init(void) return false; } + atomic_long_set(&sgx_nr_used_pages, sgx_nr_total_pages); + for_each_online_node(nid) { if (!node_isset(nid, sgx_numa_mask) && node_state(nid, N_MEMORY) && node_state(nid, N_CPU))
sgx_nr_free_pages is an atomic that is used to keep track of free EPC pages and detect whenever page reclaiming should start. Since successful execution of ENCLS[EUPDATESVN] requires empty EPC and a fast way of checking for this, change this variable around to indicate number of used pages instead. The subsequent patch that introduces ENCLS[EUPDATESVN] will take use of this change. No functional changes intended. Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> --- arch/x86/kernel/cpu/sgx/main.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)