| Message ID | 20210313160119.1318533-2-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: NUMA | expand |
On 3/13/21 8:01 AM, Jarkko Sakkinen wrote: > Replace the ad-hoc code with a sgx_free_epc_page(), in order to make sure > that all the relevant checks and book keeping is done, while freeing a > borrowed EPC page, and remove redundant code. EREMOVE inside > sgx_free_epc_page() does not change the semantics, as EREMOVE to an > uninitialize pages is a nop. ^ uninitialized I know this is a short patch, but this changelog still falls a bit short for me. Why is this patch a part of _this_ series? What *problem* does it solve, related to this series? It would also be nice to remind me why the EREMOVE is redundant. Why didn't we need one before? What put the page in the uninitialized state? Is EREMOVE guaranteed to do nothing? How expensive is it? > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index 8df81a3ed945..65004fb8a91f 100644 > --- a/arch/x86/kernel/cpu/sgx/main.c > +++ b/arch/x86/kernel/cpu/sgx/main.c > @@ -305,7 +305,6 @@ static void sgx_reclaim_pages(void) > { > struct sgx_epc_page *chunk[SGX_NR_TO_SCAN]; > struct sgx_backing backing[SGX_NR_TO_SCAN]; > - struct sgx_epc_section *section; > struct sgx_encl_page *encl_page; > struct sgx_epc_page *epc_page; > pgoff_t page_index; > @@ -378,11 +377,7 @@ static void sgx_reclaim_pages(void) > kref_put(&encl_page->encl->refcount, sgx_encl_release); > epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED; > > - section = &sgx_epc_sections[epc_page->section]; > - spin_lock(§ion->lock); > - list_add_tail(&epc_page->list, §ion->page_list); > - section->free_cnt++; > - spin_unlock(§ion->lock); > + sgx_free_epc_page(epc_page); > } > } > >
On Mon, Mar 15, 2021 at 08:32:13AM -0700, Dave Hansen wrote: > On 3/13/21 8:01 AM, Jarkko Sakkinen wrote: > > Replace the ad-hoc code with a sgx_free_epc_page(), in order to make sure > > that all the relevant checks and book keeping is done, while freeing a > > borrowed EPC page, and remove redundant code. EREMOVE inside > > sgx_free_epc_page() does not change the semantics, as EREMOVE to an > > uninitialize pages is a nop. > > ^ uninitialized > > I know this is a short patch, but this changelog still falls a bit short > for me. > > Why is this patch a part of _this_ series? What *problem* does it > solve, related to this series? I'm thinking of merging sgx_epc_section and sgx_numa_node. That's why I kept it as part of the series. Also, in any case it's better to clean up duplicate functionality. The code is essentially open coded implementation of sgx_free_epc_page() without EREMOVE. > It would also be nice to remind me why the EREMOVE is redundant. Why > didn't we need one before? What put the page in the uninitialized > state? Is EREMOVE guaranteed to do nothing? How expensive is it? EREMOVE gets removed by KVM series from sgx_free_epc_page() anyway. Maybe should re-send this patch, or series, after KVM series is merged. Then there is no explaining with EREMOVE, as sgx_free_epc_page() won't contain it. /Jarkko
On Mon, Mar 15, 2021 at 09:06:29PM +0200, Jarkko Sakkinen wrote: > On Mon, Mar 15, 2021 at 08:32:13AM -0700, Dave Hansen wrote: > > On 3/13/21 8:01 AM, Jarkko Sakkinen wrote: > > > Replace the ad-hoc code with a sgx_free_epc_page(), in order to make sure > > > that all the relevant checks and book keeping is done, while freeing a > > > borrowed EPC page, and remove redundant code. EREMOVE inside > > > sgx_free_epc_page() does not change the semantics, as EREMOVE to an > > > uninitialize pages is a nop. > > > > ^ uninitialized > > > > I know this is a short patch, but this changelog still falls a bit short > > for me. > > > > Why is this patch a part of _this_ series? What *problem* does it > > solve, related to this series? > > I'm thinking of merging sgx_epc_section and sgx_numa_node. That's why I > kept it as part of the series. > > Also, in any case it's better to clean up duplicate functionality. The > code is essentially open coded implementation of sgx_free_epc_page() > without EREMOVE. > > > It would also be nice to remind me why the EREMOVE is redundant. Why > > didn't we need one before? What put the page in the uninitialized > > state? Is EREMOVE guaranteed to do nothing? How expensive is it? > > EREMOVE gets removed by KVM series from sgx_free_epc_page() anyway. > > Maybe should re-send this patch, or series, after KVM series is merged. > Then there is no explaining with EREMOVE, as sgx_free_epc_page() won't > contain it. Anyway, forgot to put the end statement: I'm cool with dropping this but I'll also send this right after KVM SGX series has landed as separate patch, if I drop this now. /Jarkko
On Mon, Mar 15, 2021 at 09:27:00PM +0200, Jarkko Sakkinen wrote: > On Mon, Mar 15, 2021 at 09:06:29PM +0200, Jarkko Sakkinen wrote: > > On Mon, Mar 15, 2021 at 08:32:13AM -0700, Dave Hansen wrote: > > > On 3/13/21 8:01 AM, Jarkko Sakkinen wrote: > > > > Replace the ad-hoc code with a sgx_free_epc_page(), in order to make sure > > > > that all the relevant checks and book keeping is done, while freeing a > > > > borrowed EPC page, and remove redundant code. EREMOVE inside > > > > sgx_free_epc_page() does not change the semantics, as EREMOVE to an > > > > uninitialize pages is a nop. > > > > > > ^ uninitialized > > > > > > I know this is a short patch, but this changelog still falls a bit short > > > for me. > > > > > > Why is this patch a part of _this_ series? What *problem* does it > > > solve, related to this series? > > > > I'm thinking of merging sgx_epc_section and sgx_numa_node. That's why I > > kept it as part of the series. > > > > Also, in any case it's better to clean up duplicate functionality. The > > code is essentially open coded implementation of sgx_free_epc_page() > > without EREMOVE. > > > > > It would also be nice to remind me why the EREMOVE is redundant. Why > > > didn't we need one before? What put the page in the uninitialized > > > state? Is EREMOVE guaranteed to do nothing? How expensive is it? > > > > EREMOVE gets removed by KVM series from sgx_free_epc_page() anyway. > > > > Maybe should re-send this patch, or series, after KVM series is merged. > > Then there is no explaining with EREMOVE, as sgx_free_epc_page() won't > > contain it. > > Anyway, forgot to put the end statement: I'm cool with dropping this but > I'll also send this right after KVM SGX series has landed as separate > patch, if I drop this now. HOLD ON :-) I recalled why I added this patch to this patch set. I had a reason for it. It's because of the NUMA patch. I have duplicate all the NUMA changes here if I don't refactor this somewhat redundant code out. So, if I add a note about this to the commit message? IMHO, this is good enough reason to carry the patch. /Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index 8df81a3ed945..65004fb8a91f 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -305,7 +305,6 @@ static void sgx_reclaim_pages(void) { struct sgx_epc_page *chunk[SGX_NR_TO_SCAN]; struct sgx_backing backing[SGX_NR_TO_SCAN]; - struct sgx_epc_section *section; struct sgx_encl_page *encl_page; struct sgx_epc_page *epc_page; pgoff_t page_index; @@ -378,11 +377,7 @@ static void sgx_reclaim_pages(void) kref_put(&encl_page->encl->refcount, sgx_encl_release); epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED; - section = &sgx_epc_sections[epc_page->section]; - spin_lock(§ion->lock); - list_add_tail(&epc_page->list, §ion->page_list); - section->free_cnt++; - spin_unlock(§ion->lock); + sgx_free_epc_page(epc_page); } }
Replace the ad-hoc code with a sgx_free_epc_page(), in order to make sure that all the relevant checks and book keeping is done, while freeing a borrowed EPC page, and remove redundant code. EREMOVE inside sgx_free_epc_page() does not change the semantics, as EREMOVE to an uninitialize pages is a nop. Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- v4: * Rewrote the commit message. arch/x86/kernel/cpu/sgx/main.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)