| Message ID | 20250415115213.291449-2-elena.reshetova@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable automatic SVN updates for SGX enclaves | expand |
On Tue, 2025-04-15 at 14:51 +0300, 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 The mentioning of ENCLS[EUPDATESVN] is kinda out of blue here. It's better to introduce it first like the next patch does. > EPC and preferably a fast lockless way of checking for this > condition in all code paths where EPC is already used, change the > reclaiming code to track the number of used pages via > sgx_nr_used_pages instead of sgx_nr_free_pages. > For this change to work in the page reclamation code, add a new > variable, sgx_nr_total_pages, that will keep track of total > number of EPC pages. > > It would have been possible to implement ENCLS[EUPDATESVN] using > existing sgx_nr_free_pages counter and a new sgx_nr_total_pages > counter, but it won't be possible to avoid taking a lock *every time* > a new EPC page is being allocated. The conversion of sgx_nr_free_pages > into sgx_nr_used_pages allows avoiding the lock in all cases except > when it is the first EPC page being allocated via a quick > atomic_long_inc_not_zero check. > > Note: The serialization for sgx_nr_total_pages is not needed because > the variable is only updated during the initialization and there's no > concurrent access. > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> Reviewed-by: Kai Huang <kai.huang@intel.com>
> On Tue, 2025-04-15 at 14:51 +0300, 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 > > The mentioning of ENCLS[EUPDATESVN] is kinda out of blue here. It's better > to > introduce it first like the next patch does. Thank you, will expand more. > > > EPC and preferably a fast lockless way of checking for this > > condition in all code paths where EPC is already used, change the > > reclaiming code to track the number of used pages via > > sgx_nr_used_pages instead of sgx_nr_free_pages. > > For this change to work in the page reclamation code, add a new > > variable, sgx_nr_total_pages, that will keep track of total > > number of EPC pages. > > > > It would have been possible to implement ENCLS[EUPDATESVN] using > > existing sgx_nr_free_pages counter and a new sgx_nr_total_pages > > counter, but it won't be possible to avoid taking a lock *every time* > > a new EPC page is being allocated. The conversion of sgx_nr_free_pages > > into sgx_nr_used_pages allows avoiding the lock in all cases except > > when it is the first EPC page being allocated via a quick > > atomic_long_inc_not_zero check. > > > > Note: The serialization for sgx_nr_total_pages is not needed because > > the variable is only updated during the initialization and there's no > > concurrent access. > > > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> > > Reviewed-by: Kai Huang <kai.huang@intel.com> Thank you very much for your review!
On Tue, Apr 15, 2025 at 02:51:21PM +0300, Elena Reshetova wrote: > Note: The serialization for sgx_nr_total_pages is not needed because > the variable is only updated during the initialization and there's no > concurrent access. No. It's - not a side-note but core part of the rationale. - the reasoning here is nonsense, or more like it does not exist at all. sgx_nr_free_pages can be substituted with sgx_nr_used_pages at the sites where sgx_free_pages was previously used *exactly* because sgx_reclaimer_init() is called only after sgx_page_cache_init(). This gives the invariant of it to be constant whenever sgx_alloc_epc_page() is called. These type of changes give a proof of the legitimity of the invariant, which I addressed here. BR, Jarkko
On Wed, Apr 16, 2025 at 09:50:44PM +0300, Jarkko Sakkinen wrote: > On Tue, Apr 15, 2025 at 02:51:21PM +0300, Elena Reshetova wrote: > > Note: The serialization for sgx_nr_total_pages is not needed because > > the variable is only updated during the initialization and there's no > > concurrent access. > > No. It's > > - not a side-note but core part of the rationale. > - the reasoning here is nonsense, or more like it does not exist at all. > > sgx_nr_free_pages can be substituted with sgx_nr_used_pages at the sites > where sgx_free_pages was previously used *exactly* because > sgx_reclaimer_init() is called only after sgx_page_cache_init(). This > gives the invariant of it to be constant whenever sgx_alloc_epc_page() > is called. > > These type of changes give a proof of the legitimity of the invariant, > which I addressed here. Let's assume that this patch set had a bug just as a mind game. If we have a reasoning of the change documented, we will end up having better tools to reason what we did not consider while acking this patch set per se. With convoluted reasoning we have absolutely nothing. This why what I'm saying is important. BR, Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index 8ce352fc72ac..b61d3bad0446 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -32,7 +32,8 @@ 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. */ static nodemask_t sgx_numa_mask; @@ -378,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); } /* @@ -456,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; } @@ -616,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, @@ -648,6 +649,8 @@ 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_total_pages += nr_pages; + return true; } @@ -848,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 preferably a fast lockless way of checking for this condition in all code paths where EPC is already used, change the reclaiming code to track the number of used pages via sgx_nr_used_pages instead of sgx_nr_free_pages. For this change to work in the page reclamation code, add a new variable, sgx_nr_total_pages, that will keep track of total number of EPC pages. It would have been possible to implement ENCLS[EUPDATESVN] using existing sgx_nr_free_pages counter and a new sgx_nr_total_pages counter, but it won't be possible to avoid taking a lock *every time* a new EPC page is being allocated. The conversion of sgx_nr_free_pages into sgx_nr_used_pages allows avoiding the lock in all cases except when it is the first EPC page being allocated via a quick atomic_long_inc_not_zero check. Note: The serialization for sgx_nr_total_pages is not needed because the variable is only updated during the initialization and there's no concurrent access. Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> --- arch/x86/kernel/cpu/sgx/main.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)