| Message ID | b3a17e1ce7bcd14db3c28903e8a97f094998ae74.1648847675.git.reinette.chatre@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx and selftests/sgx: Support SGX2 | expand |
On Mon, Apr 04, 2022 at 09:49:27AM -0700, Reinette Chatre wrote: > The page reclaimer ensures availability of EPC pages across all > enclaves. In support of this it runs independently from the > individual enclaves in order to take locks from the different > enclaves as it writes pages to swap. > > When needing to load a page from swap an EPC page needs to be > available for its contents to be loaded into. Loading an existing > enclave page from swap does not reclaim EPC pages directly if > none are available, instead the reclaimer is woken when the > available EPC pages are found to be below a watermark. > > When iterating over a large number of pages in an oversubscribed > environment there is a race between the reclaimer woken up and > EPC pages reclaimed fast enough for the page operations to proceed. > > Ensure there are EPC pages available before attempting to load > a page that may potentially be pulled from swap into an available > EPC page. > > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > --- > No changes since V2 > > Changes since v1: > - Reword commit message. > > arch/x86/kernel/cpu/sgx/ioctl.c | 6 ++++++ > arch/x86/kernel/cpu/sgx/main.c | 6 ++++++ > arch/x86/kernel/cpu/sgx/sgx.h | 1 + > 3 files changed, 13 insertions(+) > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > index 515e1961cc02..f88bc1236276 100644 > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > @@ -777,6 +777,8 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl, > for (c = 0 ; c < modp->length; c += PAGE_SIZE) { > addr = encl->base + modp->offset + c; > > + sgx_direct_reclaim(); > + > mutex_lock(&encl->lock); > > entry = sgx_encl_load_page(encl, addr); > @@ -934,6 +936,8 @@ static long sgx_enclave_modify_type(struct sgx_encl *encl, > for (c = 0 ; c < modt->length; c += PAGE_SIZE) { > addr = encl->base + modt->offset + c; > > + sgx_direct_reclaim(); > + > mutex_lock(&encl->lock); > > entry = sgx_encl_load_page(encl, addr); > @@ -1129,6 +1133,8 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl, > for (c = 0 ; c < params->length; c += PAGE_SIZE) { > addr = encl->base + params->offset + c; > > + sgx_direct_reclaim(); > + > mutex_lock(&encl->lock); > > entry = sgx_encl_load_page(encl, addr); > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index 6e2cb7564080..545da16bb3ea 100644 > --- a/arch/x86/kernel/cpu/sgx/main.c > +++ b/arch/x86/kernel/cpu/sgx/main.c > @@ -370,6 +370,12 @@ static bool sgx_should_reclaim(unsigned long watermark) > !list_empty(&sgx_active_page_list); > } > > +void sgx_direct_reclaim(void) > +{ > + if (sgx_should_reclaim(SGX_NR_LOW_PAGES)) > + sgx_reclaim_pages(); > +} Please, instead open code this to both locations - not enough redundancy to be worth of new function. Causes only unnecessary cross-referencing when maintaining. Otherwise, I agree with the idea. BR, Jarkko
Hi Jarkko, On 4/5/2022 12:11 AM, Jarkko Sakkinen wrote: > On Mon, Apr 04, 2022 at 09:49:27AM -0700, Reinette Chatre wrote: >> The page reclaimer ensures availability of EPC pages across all >> enclaves. In support of this it runs independently from the >> individual enclaves in order to take locks from the different >> enclaves as it writes pages to swap. >> >> When needing to load a page from swap an EPC page needs to be >> available for its contents to be loaded into. Loading an existing >> enclave page from swap does not reclaim EPC pages directly if >> none are available, instead the reclaimer is woken when the >> available EPC pages are found to be below a watermark. >> >> When iterating over a large number of pages in an oversubscribed >> environment there is a race between the reclaimer woken up and >> EPC pages reclaimed fast enough for the page operations to proceed. >> >> Ensure there are EPC pages available before attempting to load >> a page that may potentially be pulled from swap into an available >> EPC page. >> >> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> >> --- >> No changes since V2 >> >> Changes since v1: >> - Reword commit message. >> >> arch/x86/kernel/cpu/sgx/ioctl.c | 6 ++++++ >> arch/x86/kernel/cpu/sgx/main.c | 6 ++++++ >> arch/x86/kernel/cpu/sgx/sgx.h | 1 + >> 3 files changed, 13 insertions(+) >> >> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c >> index 515e1961cc02..f88bc1236276 100644 >> --- a/arch/x86/kernel/cpu/sgx/ioctl.c >> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c >> @@ -777,6 +777,8 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl, >> for (c = 0 ; c < modp->length; c += PAGE_SIZE) { >> addr = encl->base + modp->offset + c; >> >> + sgx_direct_reclaim(); >> + >> mutex_lock(&encl->lock); >> >> entry = sgx_encl_load_page(encl, addr); >> @@ -934,6 +936,8 @@ static long sgx_enclave_modify_type(struct sgx_encl *encl, >> for (c = 0 ; c < modt->length; c += PAGE_SIZE) { >> addr = encl->base + modt->offset + c; >> >> + sgx_direct_reclaim(); >> + >> mutex_lock(&encl->lock); >> >> entry = sgx_encl_load_page(encl, addr); >> @@ -1129,6 +1133,8 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl, >> for (c = 0 ; c < params->length; c += PAGE_SIZE) { >> addr = encl->base + params->offset + c; >> >> + sgx_direct_reclaim(); >> + >> mutex_lock(&encl->lock); >> >> entry = sgx_encl_load_page(encl, addr); >> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c >> index 6e2cb7564080..545da16bb3ea 100644 >> --- a/arch/x86/kernel/cpu/sgx/main.c >> +++ b/arch/x86/kernel/cpu/sgx/main.c >> @@ -370,6 +370,12 @@ static bool sgx_should_reclaim(unsigned long watermark) >> !list_empty(&sgx_active_page_list); >> } >> >> +void sgx_direct_reclaim(void) >> +{ >> + if (sgx_should_reclaim(SGX_NR_LOW_PAGES)) >> + sgx_reclaim_pages(); >> +} > > Please, instead open code this to both locations - not enough redundancy > to be worth of new function. Causes only unnecessary cross-referencing > when maintaining. Otherwise, I agree with the idea. > hmmm, that means the heart of the reclaimer (sgx_reclaim_pages()) would be made available for direct use from everywhere in the driver. I will look into this. Reinette
On 4/5/22 10:13, Reinette Chatre wrote: >>> +void sgx_direct_reclaim(void) >>> +{ >>> + if (sgx_should_reclaim(SGX_NR_LOW_PAGES)) >>> + sgx_reclaim_pages(); >>> +} >> Please, instead open code this to both locations - not enough redundancy >> to be worth of new function. Causes only unnecessary cross-referencing >> when maintaining. Otherwise, I agree with the idea. >> > hmmm, that means the heart of the reclaimer (sgx_reclaim_pages()) would be > made available for direct use from everywhere in the driver. I will look into this. I like the change. It's not about reducing code redundancy, it's about *describing* what the code does. Each location could have: /* Enter direct SGX reclaim: */ if (sgx_should_reclaim(SGX_NR_LOW_PAGES)) sgx_reclaim_pages(); Or, it could just be: sgx_direct_reclaim(); Which also provides a logical choke point to add comments, like: /* * sgx_direct_reclaim() should be called in locations where SGX * memory resources might be low and might be needed in order * to make forward progress. */ void sgx_direct_reclaim(void) { ...
On Tue, 2022-04-05 at 10:13 -0700, Reinette Chatre wrote: > Hi Jarkko, > > On 4/5/2022 12:11 AM, Jarkko Sakkinen wrote: > > On Mon, Apr 04, 2022 at 09:49:27AM -0700, Reinette Chatre wrote: > > > The page reclaimer ensures availability of EPC pages across all > > > enclaves. In support of this it runs independently from the > > > individual enclaves in order to take locks from the different > > > enclaves as it writes pages to swap. > > > > > > When needing to load a page from swap an EPC page needs to be > > > available for its contents to be loaded into. Loading an existing > > > enclave page from swap does not reclaim EPC pages directly if > > > none are available, instead the reclaimer is woken when the > > > available EPC pages are found to be below a watermark. > > > > > > When iterating over a large number of pages in an oversubscribed > > > environment there is a race between the reclaimer woken up and > > > EPC pages reclaimed fast enough for the page operations to proceed. > > > > > > Ensure there are EPC pages available before attempting to load > > > a page that may potentially be pulled from swap into an available > > > EPC page. > > > > > > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > > > --- > > > No changes since V2 > > > > > > Changes since v1: > > > - Reword commit message. > > > > > > arch/x86/kernel/cpu/sgx/ioctl.c | 6 ++++++ > > > arch/x86/kernel/cpu/sgx/main.c | 6 ++++++ > > > arch/x86/kernel/cpu/sgx/sgx.h | 1 + > > > 3 files changed, 13 insertions(+) > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > > > index 515e1961cc02..f88bc1236276 100644 > > > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > > > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > > > @@ -777,6 +777,8 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl, > > > for (c = 0 ; c < modp->length; c += PAGE_SIZE) { > > > addr = encl->base + modp->offset + c; > > > > > > + sgx_direct_reclaim(); > > > + > > > mutex_lock(&encl->lock); > > > > > > entry = sgx_encl_load_page(encl, addr); > > > @@ -934,6 +936,8 @@ static long sgx_enclave_modify_type(struct sgx_encl *encl, > > > for (c = 0 ; c < modt->length; c += PAGE_SIZE) { > > > addr = encl->base + modt->offset + c; > > > > > > + sgx_direct_reclaim(); > > > + > > > mutex_lock(&encl->lock); > > > > > > entry = sgx_encl_load_page(encl, addr); > > > @@ -1129,6 +1133,8 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl, > > > for (c = 0 ; c < params->length; c += PAGE_SIZE) { > > > addr = encl->base + params->offset + c; > > > > > > + sgx_direct_reclaim(); > > > + > > > mutex_lock(&encl->lock); > > > > > > entry = sgx_encl_load_page(encl, addr); > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > > index 6e2cb7564080..545da16bb3ea 100644 > > > --- a/arch/x86/kernel/cpu/sgx/main.c > > > +++ b/arch/x86/kernel/cpu/sgx/main.c > > > @@ -370,6 +370,12 @@ static bool sgx_should_reclaim(unsigned long watermark) > > > !list_empty(&sgx_active_page_list); > > > } > > > > > > +void sgx_direct_reclaim(void) > > > +{ > > > + if (sgx_should_reclaim(SGX_NR_LOW_PAGES)) > > > + sgx_reclaim_pages(); > > > +} > > > > Please, instead open code this to both locations - not enough redundancy > > to be worth of new function. Causes only unnecessary cross-referencing > > when maintaining. Otherwise, I agree with the idea. > > > > hmmm, that means the heart of the reclaimer (sgx_reclaim_pages()) would be > made available for direct use from everywhere in the driver. I will look into this. > > Reinette > It's a valid enough point. Let's keep it as it is :-) Acked-by: Jarkko Sakkinen <jarkko@kernel.org> BR, Jarkko
Hi Jarkko, On 4/5/2022 11:42 AM, Jarkko Sakkinen wrote: > On Tue, 2022-04-05 at 10:13 -0700, Reinette Chatre wrote: >> On 4/5/2022 12:11 AM, Jarkko Sakkinen wrote: >>> On Mon, Apr 04, 2022 at 09:49:27AM -0700, Reinette Chatre wrote: ... >>>> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c >>>> index 6e2cb7564080..545da16bb3ea 100644 >>>> --- a/arch/x86/kernel/cpu/sgx/main.c >>>> +++ b/arch/x86/kernel/cpu/sgx/main.c >>>> @@ -370,6 +370,12 @@ static bool sgx_should_reclaim(unsigned long watermark) >>>> !list_empty(&sgx_active_page_list); >>>> } >>>> >>>> +void sgx_direct_reclaim(void) >>>> +{ >>>> + if (sgx_should_reclaim(SGX_NR_LOW_PAGES)) >>>> + sgx_reclaim_pages(); >>>> +} >>> >>> Please, instead open code this to both locations - not enough redundancy >>> to be worth of new function. Causes only unnecessary cross-referencing >>> when maintaining. Otherwise, I agree with the idea. >>> >> >> hmmm, that means the heart of the reclaimer (sgx_reclaim_pages()) would be >> made available for direct use from everywhere in the driver. I will look into this. >> >> Reinette >> > > It's a valid enough point. Let's keep it as it is :-) Will do. I plan to add Dave's suggested comments to sgx_direct_reclaim() that is introduced in this patch. > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org> Thank you very much. Reinette
On Tue, 2022-04-05 at 10:25 -0700, Dave Hansen wrote: > On 4/5/22 10:13, Reinette Chatre wrote: > > > > +void sgx_direct_reclaim(void) > > > > +{ > > > > + if (sgx_should_reclaim(SGX_NR_LOW_PAGES)) > > > > + sgx_reclaim_pages(); > > > > +} > > > Please, instead open code this to both locations - not enough redundancy > > > to be worth of new function. Causes only unnecessary cross-referencing > > > when maintaining. Otherwise, I agree with the idea. > > > > > hmmm, that means the heart of the reclaimer (sgx_reclaim_pages()) would be > > made available for direct use from everywhere in the driver. I will look into this. > > I like the change. It's not about reducing code redundancy, it's about > *describing* what the code does. Each location could have: > > /* Enter direct SGX reclaim: */ > if (sgx_should_reclaim(SGX_NR_LOW_PAGES)) > sgx_reclaim_pages(); > > Or, it could just be: > > sgx_direct_reclaim(); > > Which also provides a logical choke point to add comments, like: > > /* > * sgx_direct_reclaim() should be called in locations where SGX > * memory resources might be low and might be needed in order > * to make forward progress. > */ > void sgx_direct_reclaim(void) > { > ... Maybe cutting hairs but could it be "sgx_reclaim_direct"? Rationale is easier grepping of reclaimer functions, e.g. when tracing. BR, Jarkko
Hi Jarkko, On 4/5/2022 11:35 PM, Jarkko Sakkinen wrote: > On Tue, 2022-04-05 at 10:25 -0700, Dave Hansen wrote: >> On 4/5/22 10:13, Reinette Chatre wrote: >>>>> +void sgx_direct_reclaim(void) >>>>> +{ >>>>> + if (sgx_should_reclaim(SGX_NR_LOW_PAGES)) >>>>> + sgx_reclaim_pages(); >>>>> +} >>>> Please, instead open code this to both locations - not enough redundancy >>>> to be worth of new function. Causes only unnecessary cross-referencing >>>> when maintaining. Otherwise, I agree with the idea. >>>> >>> hmmm, that means the heart of the reclaimer (sgx_reclaim_pages()) would be >>> made available for direct use from everywhere in the driver. I will look into this. >> >> I like the change. It's not about reducing code redundancy, it's about >> *describing* what the code does. Each location could have: >> >> /* Enter direct SGX reclaim: */ >> if (sgx_should_reclaim(SGX_NR_LOW_PAGES)) >> sgx_reclaim_pages(); >> >> Or, it could just be: >> >> sgx_direct_reclaim(); >> >> Which also provides a logical choke point to add comments, like: >> >> /* >> * sgx_direct_reclaim() should be called in locations where SGX >> * memory resources might be low and might be needed in order >> * to make forward progress. >> */ >> void sgx_direct_reclaim(void) >> { >> ... > > Maybe cutting hairs but could it be "sgx_reclaim_direct"? Rationale > is easier grepping of reclaimer functions, e.g. when tracing. Sure, will do. This may not help grepping all reclaimer functions though since the code is not consistent in this regard. Reinette
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 515e1961cc02..f88bc1236276 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -777,6 +777,8 @@ sgx_enclave_restrict_permissions(struct sgx_encl *encl, for (c = 0 ; c < modp->length; c += PAGE_SIZE) { addr = encl->base + modp->offset + c; + sgx_direct_reclaim(); + mutex_lock(&encl->lock); entry = sgx_encl_load_page(encl, addr); @@ -934,6 +936,8 @@ static long sgx_enclave_modify_type(struct sgx_encl *encl, for (c = 0 ; c < modt->length; c += PAGE_SIZE) { addr = encl->base + modt->offset + c; + sgx_direct_reclaim(); + mutex_lock(&encl->lock); entry = sgx_encl_load_page(encl, addr); @@ -1129,6 +1133,8 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl, for (c = 0 ; c < params->length; c += PAGE_SIZE) { addr = encl->base + params->offset + c; + sgx_direct_reclaim(); + mutex_lock(&encl->lock); entry = sgx_encl_load_page(encl, addr); diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index 6e2cb7564080..545da16bb3ea 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -370,6 +370,12 @@ static bool sgx_should_reclaim(unsigned long watermark) !list_empty(&sgx_active_page_list); } +void sgx_direct_reclaim(void) +{ + if (sgx_should_reclaim(SGX_NR_LOW_PAGES)) + sgx_reclaim_pages(); +} + static int ksgxd(void *p) { set_freezable(); diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h index b30cee4de903..85cbf103b0dd 100644 --- a/arch/x86/kernel/cpu/sgx/sgx.h +++ b/arch/x86/kernel/cpu/sgx/sgx.h @@ -86,6 +86,7 @@ static inline void *sgx_get_epc_virt_addr(struct sgx_epc_page *page) struct sgx_epc_page *__sgx_alloc_epc_page(void); void sgx_free_epc_page(struct sgx_epc_page *page); +void sgx_direct_reclaim(void); void sgx_mark_page_reclaimable(struct sgx_epc_page *page); int sgx_unmark_page_reclaimable(struct sgx_epc_page *page); struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim);
The page reclaimer ensures availability of EPC pages across all enclaves. In support of this it runs independently from the individual enclaves in order to take locks from the different enclaves as it writes pages to swap. When needing to load a page from swap an EPC page needs to be available for its contents to be loaded into. Loading an existing enclave page from swap does not reclaim EPC pages directly if none are available, instead the reclaimer is woken when the available EPC pages are found to be below a watermark. When iterating over a large number of pages in an oversubscribed environment there is a race between the reclaimer woken up and EPC pages reclaimed fast enough for the page operations to proceed. Ensure there are EPC pages available before attempting to load a page that may potentially be pulled from swap into an available EPC page. Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> --- No changes since V2 Changes since v1: - Reword commit message. arch/x86/kernel/cpu/sgx/ioctl.c | 6 ++++++ arch/x86/kernel/cpu/sgx/main.c | 6 ++++++ arch/x86/kernel/cpu/sgx/sgx.h | 1 + 3 files changed, 13 insertions(+)