| 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.
On Mon, Mar 24, 2025 at 12:19:37PM +0000, Reshetova, Elena wrote: > > > 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." Yep, whole a lot more sense. > > 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. Have you benchmarked this (memory barrier vs putting the whole thing inside spinlock)? I have doubts that this would even show in margins given how much e.g., ELDU takes. > > Best Regards, > Elena. > > BR, Jarkko
> On Mon, Mar 24, 2025 at 12:19:37PM +0000, Reshetova, Elena wrote: > > > > > 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." > > Yep, whole a lot more sense. > > > > > 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. > > Have you benchmarked this (memory barrier vs putting the whole thing > inside spinlock)? > > I have doubts that this would even show in margins given how much e.g., > ELDU takes. No, I haven’t benchmarked this. The reason to choose this approach (vs. the other one I showed in email now) was purely logical - less locks and better code. Best Regards, Elena.
On Thu, Mar 27, 2025 at 03:31:31PM +0000, Reshetova, Elena wrote: > > On Mon, Mar 24, 2025 at 12:19:37PM +0000, Reshetova, Elena wrote: > > > > > > > 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." > > > > Yep, whole a lot more sense. > > > > > > > > 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. > > > > Have you benchmarked this (memory barrier vs putting the whole thing > > inside spinlock)? > > > > I have doubts that this would even show in margins given how much e.g., > > ELDU takes. > > No, I haven’t benchmarked this. The reason to choose this approach > (vs. the other one I showed in email now) was purely logical - less > locks and better code. You need to have hard data to to turn things around like this. Otherwise, let's not do this. EPC allocation is always combined with heave-weight opcode so this really does not serve any purpose. > > Best Regards, > Elena. BR, Jarkko
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(-)