| Message ID | 20210303150323.433207-3-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: NUMA | expand |
On 3/3/21 7:03 AM, Jarkko Sakkinen wrote: > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index 52d070fb4c9a..ed99c60024dc 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); > } > } In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove(). This code does not call __eremove(). That seems to be changing behavior where none was intended. Was this, perhaps, based on top of Kai's series that changes the behavior of sgx_free_epc_page()?
On Wed, Mar 03, 2021 at 08:59:17AM -0800, Dave Hansen wrote: > On 3/3/21 7:03 AM, Jarkko Sakkinen wrote: > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > index 52d070fb4c9a..ed99c60024dc 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); > > } > > } > > In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove(). > This code does not call __eremove(). That seems to be changing > behavior where none was intended. EREMOVE does not matter here, as it doesn't in almost all most of the sites where sgx_free_epc_page() is used in the driver. It does nothing to an uninitialized pages. The two patches that I posted originally for Kai's series took EREMOVE out of sgx_free_epc_page() and put an explicit EREMOVE where it is actually needed, but for reasons unknown to me, that change is gone. Replacing the ad-hoc code with sgx_free_epc_page() is absolutely the right action to take because it follows the pattern how sgx_free_epc_page() is used in the driver. For reference: https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/ > Was this, perhaps, based on top of Kai's series that changes the > behavior of sgx_free_epc_page()? I did not refer to that patch series. /Jarkko
On 3/10/21 7:11 AM, Jarkko Sakkinen wrote: >>> - 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); >>> } >>> } >> In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove(). >> This code does not call __eremove(). That seems to be changing >> behavior where none was intended. > EREMOVE does not matter here, as it doesn't in almost all most of the sites > where sgx_free_epc_page() is used in the driver. It does nothing to an > uninitialized pages. > > The two patches that I posted originally for Kai's series took EREMOVE out > of sgx_free_epc_page() and put an explicit EREMOVE where it is actually > needed, but for reasons unknown to me, that change is gone. > > Replacing the ad-hoc code with sgx_free_epc_page() is absolutely the right > action to take because it follows the pattern how sgx_free_epc_page() is > used in the driver. That sounds generally fine. But, this is a functional change. Where there are functional changes, I always hope to see some mention of the change in the changelog. Could you add some of this to the next changelog, please?
On Wed, 2021-03-10 at 17:11 +0200, Jarkko Sakkinen wrote: > On Wed, Mar 03, 2021 at 08:59:17AM -0800, Dave Hansen wrote: > > On 3/3/21 7:03 AM, Jarkko Sakkinen wrote: > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > > index 52d070fb4c9a..ed99c60024dc 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); > > > } > > > } > > > > In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove(). > > This code does not call __eremove(). That seems to be changing > > behavior where none was intended. > > EREMOVE does not matter here, as it doesn't in almost all most of the sites > where sgx_free_epc_page() is used in the driver. It does nothing to an > uninitialized pages. Right. EREMOVE on uninitialized pages does nothing, so a more reasonable way is to just NOT call EREMOVE (your original code), since it is absolutely unnecessary. I don't see ANY reason we should call EREMOVE here. Actually w/o my patch to split EREMOVE out of sgx_free_epc_page(), it then makes perfect sense to have new sgx_free_epc_page() here. > > The two patches that I posted originally for Kai's series took EREMOVE out > of sgx_free_epc_page() and put an explicit EREMOVE where it is actually > needed, but for reasons unknown to me, that change is gone. > It's not gone. It goes into a new sgx_encl_free_epc_page(), which is exactly the same as current sgx_free_epc_page() which as EREMOVE, instead of putting EREMOVE into a dedicated sgx_reset_epc_page(), as you did in your series: https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/ However, your change has side effort: it always put page back into free pool, even EREMOVE fails. To make your change w/o having any functional change, it has to be: if(!sgx_reset_epc_page()) sgx_free_epc_page(); And for this, Dave raised one concern we should add a WARN() to let user know EPC page is leaked, and reboot is requied to get them back. However with sgx_reset_epc_page(), there's no place to add such WARN(), and implementing original sgx_free_epc_page() as sgx_encl_free_epc_page() looks very reasonable to me: https://www.spinics.net/lists/linux-sgx/msg04631.html > Replacing the ad-hoc code with sgx_free_epc_page() is absolutely the right > action to take because it follows the pattern how sgx_free_epc_page() is > used in the driver. > > For reference: > > https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/ > > > Was this, perhaps, based on top of Kai's series that changes the > > behavior of sgx_free_epc_page()? > > I did not refer to that patch series. > > /Jarkko
On Wed, Mar 10, 2021 at 07:55:35AM -0800, Dave Hansen wrote: > On 3/10/21 7:11 AM, Jarkko Sakkinen wrote: > >>> - 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); > >>> } > >>> } > >> In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove(). > >> This code does not call __eremove(). That seems to be changing > >> behavior where none was intended. > > EREMOVE does not matter here, as it doesn't in almost all most of the sites > > where sgx_free_epc_page() is used in the driver. It does nothing to an > > uninitialized pages. > > > > The two patches that I posted originally for Kai's series took EREMOVE out > > of sgx_free_epc_page() and put an explicit EREMOVE where it is actually > > needed, but for reasons unknown to me, that change is gone. > > > > Replacing the ad-hoc code with sgx_free_epc_page() is absolutely the right > > action to take because it follows the pattern how sgx_free_epc_page() is > > used in the driver. > > That sounds generally fine. But, this is a functional change. Where > there are functional changes, I always hope to see some mention of the > change in the changelog. > > Could you add some of this to the next changelog, please? This appears for the first time in this patch set version, which means that there is no patch changelog for this. Maybe a better idea would be to explain the functional change in the commit message (which of course implies also entry to the patch change log)? /Jarkko
On Thu, Mar 11, 2021 at 09:36:15AM +1300, Kai Huang wrote: > On Wed, 2021-03-10 at 17:11 +0200, Jarkko Sakkinen wrote: > > On Wed, Mar 03, 2021 at 08:59:17AM -0800, Dave Hansen wrote: > > > On 3/3/21 7:03 AM, Jarkko Sakkinen wrote: > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > > > index 52d070fb4c9a..ed99c60024dc 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); > > > > } > > > > } > > > > > > In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove(). > > > This code does not call __eremove(). That seems to be changing > > > behavior where none was intended. > > > > EREMOVE does not matter here, as it doesn't in almost all most of the sites > > where sgx_free_epc_page() is used in the driver. It does nothing to an > > uninitialized pages. > > Right. EREMOVE on uninitialized pages does nothing, so a more reasonable way is to > just NOT call EREMOVE (your original code), since it is absolutely unnecessary. > > I don't see ANY reason we should call EREMOVE here. > > Actually w/o my patch to split EREMOVE out of sgx_free_epc_page(), it then makes > perfect sense to have new sgx_free_epc_page() here. > > > > > The two patches that I posted originally for Kai's series took EREMOVE out > > of sgx_free_epc_page() and put an explicit EREMOVE where it is actually > > needed, but for reasons unknown to me, that change is gone. > > > > It's not gone. It goes into a new sgx_encl_free_epc_page(), which is exactly the same > as current sgx_free_epc_page() which as EREMOVE, instead of putting EREMOVE into a > dedicated sgx_reset_epc_page(), as you did in your series: > > https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/ > > However, your change has side effort: it always put page back into free pool, even > EREMOVE fails. To make your change w/o having any functional change, it has to be: > > if(!sgx_reset_epc_page()) > sgx_free_epc_page(); OK, great, your patch set uses the wrapper only in the necessary call sites. Sorry, I overlooked this part. Anyway, it knowingly does that. I considered either as equally harmful side-ffects when I implemented. Either can only trigger, when there is a bug in the kernel code. It *could* do what that snippet suggest but it's like "out of the frying pan, into the fire" kind of change. Since NUMA patch set anyway requires to have a global dirty list, I think the better way to deal with this, would be to declare a new global in the patch under discussion: static struct list_head sgx_dirty_list; And sgx_encl_free_epc_page() could simply put the pages in this list. In some cases you could possibly even reset the system state using kexec for debugging purposes, so it could potentially bring a tiny bit of value. I can rebase then my NUMA patches on top of SGX specific KVM patches, once Boris have applied them. > And for this, Dave raised one concern we should add a WARN() to let user know EPC > page is leaked, and reboot is requied to get them back. > > However with sgx_reset_epc_page(), there's no place to add such WARN(), and > implementing original sgx_free_epc_page() as sgx_encl_free_epc_page() looks very > reasonable to me: > > https://www.spinics.net/lists/linux-sgx/msg04631.html /Jarkko
On Thu, Mar 11, 2021 at 12:10:56AM +0200, Jarkko Sakkinen wrote: > On Thu, Mar 11, 2021 at 09:36:15AM +1300, Kai Huang wrote: > > On Wed, 2021-03-10 at 17:11 +0200, Jarkko Sakkinen wrote: > > > On Wed, Mar 03, 2021 at 08:59:17AM -0800, Dave Hansen wrote: > > > > On 3/3/21 7:03 AM, Jarkko Sakkinen wrote: > > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > > > > index 52d070fb4c9a..ed99c60024dc 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); > > > > > } > > > > > } > > > > > > > > In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove(). > > > > This code does not call __eremove(). That seems to be changing > > > > behavior where none was intended. > > > > > > EREMOVE does not matter here, as it doesn't in almost all most of the sites > > > where sgx_free_epc_page() is used in the driver. It does nothing to an > > > uninitialized pages. > > > > Right. EREMOVE on uninitialized pages does nothing, so a more reasonable way is to > > just NOT call EREMOVE (your original code), since it is absolutely unnecessary. > > > > I don't see ANY reason we should call EREMOVE here. > > > > Actually w/o my patch to split EREMOVE out of sgx_free_epc_page(), it then makes > > perfect sense to have new sgx_free_epc_page() here. > > > > > > > > The two patches that I posted originally for Kai's series took EREMOVE out > > > of sgx_free_epc_page() and put an explicit EREMOVE where it is actually > > > needed, but for reasons unknown to me, that change is gone. > > > > > > > It's not gone. It goes into a new sgx_encl_free_epc_page(), which is exactly the same > > as current sgx_free_epc_page() which as EREMOVE, instead of putting EREMOVE into a > > dedicated sgx_reset_epc_page(), as you did in your series: > > > > https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/ > > > > However, your change has side effort: it always put page back into free pool, even > > EREMOVE fails. To make your change w/o having any functional change, it has to be: > > > > if(!sgx_reset_epc_page()) > > sgx_free_epc_page(); > > OK, great, your patch set uses the wrapper only in the necessary call > sites. Sorry, I overlooked this part. > > Anyway, it knowingly does that. I considered either as equally harmful > side-ffects when I implemented. Either can only trigger, when there is a > bug in the kernel code. > > It *could* do what that snippet suggest but it's like "out of the frying pan, > into the fire" kind of change. > > Since NUMA patch set anyway requires to have a global dirty list, I think > the better way to deal with this, would be to declare a new global in the > patch under discussion: > > static struct list_head sgx_dirty_list; sgx_dirty_page_list > > And sgx_encl_free_epc_page() could simply put the pages in this list. In > some cases you could possibly even reset the system state using kexec for > debugging purposes, so it could potentially bring a tiny bit of value. > > I can rebase then my NUMA patches on top of SGX specific KVM patches, once > Boris have applied them. > > > And for this, Dave raised one concern we should add a WARN() to let user know EPC > > page is leaked, and reboot is requied to get them back. > > > > However with sgx_reset_epc_page(), there's no place to add such WARN(), and > > implementing original sgx_free_epc_page() as sgx_encl_free_epc_page() looks very > > reasonable to me: > > > > https://www.spinics.net/lists/linux-sgx/msg04631.html > > /Jarkko /Jarkko
On Thu, Mar 11, 2021 at 12:12:17AM +0200, Jarkko Sakkinen wrote: > On Thu, Mar 11, 2021 at 12:10:56AM +0200, Jarkko Sakkinen wrote: > > On Thu, Mar 11, 2021 at 09:36:15AM +1300, Kai Huang wrote: > > > On Wed, 2021-03-10 at 17:11 +0200, Jarkko Sakkinen wrote: > > > > On Wed, Mar 03, 2021 at 08:59:17AM -0800, Dave Hansen wrote: > > > > > On 3/3/21 7:03 AM, Jarkko Sakkinen wrote: > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > > > > > index 52d070fb4c9a..ed99c60024dc 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); > > > > > > } > > > > > > } > > > > > > > > > > In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove(). > > > > > This code does not call __eremove(). That seems to be changing > > > > > behavior where none was intended. > > > > > > > > EREMOVE does not matter here, as it doesn't in almost all most of the sites > > > > where sgx_free_epc_page() is used in the driver. It does nothing to an > > > > uninitialized pages. > > > > > > Right. EREMOVE on uninitialized pages does nothing, so a more reasonable way is to > > > just NOT call EREMOVE (your original code), since it is absolutely unnecessary. > > > > > > I don't see ANY reason we should call EREMOVE here. > > > > > > Actually w/o my patch to split EREMOVE out of sgx_free_epc_page(), it then makes > > > perfect sense to have new sgx_free_epc_page() here. > > > > > > > > > > > The two patches that I posted originally for Kai's series took EREMOVE out > > > > of sgx_free_epc_page() and put an explicit EREMOVE where it is actually > > > > needed, but for reasons unknown to me, that change is gone. > > > > > > > > > > It's not gone. It goes into a new sgx_encl_free_epc_page(), which is exactly the same > > > as current sgx_free_epc_page() which as EREMOVE, instead of putting EREMOVE into a > > > dedicated sgx_reset_epc_page(), as you did in your series: > > > > > > https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/ > > > > > > However, your change has side effort: it always put page back into free pool, even > > > EREMOVE fails. To make your change w/o having any functional change, it has to be: > > > > > > if(!sgx_reset_epc_page()) > > > sgx_free_epc_page(); > > > > OK, great, your patch set uses the wrapper only in the necessary call > > sites. Sorry, I overlooked this part. > > > > Anyway, it knowingly does that. I considered either as equally harmful > > side-ffects when I implemented. Either can only trigger, when there is a > > bug in the kernel code. > > > > It *could* do what that snippet suggest but it's like "out of the frying pan, > > into the fire" kind of change. > > > > Since NUMA patch set anyway requires to have a global dirty list, I think > > the better way to deal with this, would be to declare a new global in the > > patch under discussion: > > > > static struct list_head sgx_dirty_list; > > sgx_dirty_page_list Actually, I think it is good as it is now. Please do nothing :-) Acked-by: Jarkko Sakkinen <jarkko@kernel.org> I can continue from that and improve the fallback further. Not perfect, but good enough. /Jarkko
On Thu, 2021-03-11 at 00:35 +0200, Jarkko Sakkinen wrote: > On Thu, Mar 11, 2021 at 12:12:17AM +0200, Jarkko Sakkinen wrote: > > On Thu, Mar 11, 2021 at 12:10:56AM +0200, Jarkko Sakkinen wrote: > > > On Thu, Mar 11, 2021 at 09:36:15AM +1300, Kai Huang wrote: > > > > On Wed, 2021-03-10 at 17:11 +0200, Jarkko Sakkinen wrote: > > > > > On Wed, Mar 03, 2021 at 08:59:17AM -0800, Dave Hansen wrote: > > > > > > On 3/3/21 7:03 AM, Jarkko Sakkinen wrote: > > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > > > > > > index 52d070fb4c9a..ed99c60024dc 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); > > > > > > > } > > > > > > > } > > > > > > > > > > > > In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove(). > > > > > > This code does not call __eremove(). That seems to be changing > > > > > > behavior where none was intended. > > > > > > > > > > EREMOVE does not matter here, as it doesn't in almost all most of the sites > > > > > where sgx_free_epc_page() is used in the driver. It does nothing to an > > > > > uninitialized pages. > > > > > > > > Right. EREMOVE on uninitialized pages does nothing, so a more reasonable way is to > > > > just NOT call EREMOVE (your original code), since it is absolutely unnecessary. > > > > > > > > I don't see ANY reason we should call EREMOVE here. > > > > > > > > Actually w/o my patch to split EREMOVE out of sgx_free_epc_page(), it then makes > > > > perfect sense to have new sgx_free_epc_page() here. > > > > > > > > > > > > > > The two patches that I posted originally for Kai's series took EREMOVE out > > > > > of sgx_free_epc_page() and put an explicit EREMOVE where it is actually > > > > > needed, but for reasons unknown to me, that change is gone. > > > > > > > > > > > > > It's not gone. It goes into a new sgx_encl_free_epc_page(), which is exactly the same > > > > as current sgx_free_epc_page() which as EREMOVE, instead of putting EREMOVE into a > > > > dedicated sgx_reset_epc_page(), as you did in your series: > > > > > > > > https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/ > > > > > > > > However, your change has side effort: it always put page back into free pool, even > > > > EREMOVE fails. To make your change w/o having any functional change, it has to be: > > > > > > > > if(!sgx_reset_epc_page()) > > > > sgx_free_epc_page(); > > > > > > OK, great, your patch set uses the wrapper only in the necessary call > > > sites. Sorry, I overlooked this part. > > > > > > Anyway, it knowingly does that. I considered either as equally harmful > > > side-ffects when I implemented. Either can only trigger, when there is a > > > bug in the kernel code. > > > > > > It *could* do what that snippet suggest but it's like "out of the frying pan, > > > into the fire" kind of change. > > > > > > Since NUMA patch set anyway requires to have a global dirty list, I think > > > the better way to deal with this, would be to declare a new global in the > > > patch under discussion: > > > > > > static struct list_head sgx_dirty_list; > > > > sgx_dirty_page_list > > Actually, I think it is good as it is now. Please do nothing :-) > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org> > > I can continue from that and improve the fallback further. Not perfect, but > good enough. Great. Thank you Jarkko. I'll add your Acked-by and repost it since I also made a mistake in copy-paste:)
On Thu, 2021-03-11 at 11:43 +1300, Kai Huang wrote: > On Thu, 2021-03-11 at 00:35 +0200, Jarkko Sakkinen wrote: > > On Thu, Mar 11, 2021 at 12:12:17AM +0200, Jarkko Sakkinen wrote: > > > On Thu, Mar 11, 2021 at 12:10:56AM +0200, Jarkko Sakkinen wrote: > > > > On Thu, Mar 11, 2021 at 09:36:15AM +1300, Kai Huang wrote: > > > > > On Wed, 2021-03-10 at 17:11 +0200, Jarkko Sakkinen wrote: > > > > > > On Wed, Mar 03, 2021 at 08:59:17AM -0800, Dave Hansen wrote: > > > > > > > On 3/3/21 7:03 AM, Jarkko Sakkinen wrote: > > > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > > > > > > > index 52d070fb4c9a..ed99c60024dc 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); > > > > > > > > } > > > > > > > > } > > > > > > > > > > > > > > In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove(). > > > > > > > This code does not call __eremove(). That seems to be changing > > > > > > > behavior where none was intended. > > > > > > > > > > > > EREMOVE does not matter here, as it doesn't in almost all most of the sites > > > > > > where sgx_free_epc_page() is used in the driver. It does nothing to an > > > > > > uninitialized pages. > > > > > > > > > > Right. EREMOVE on uninitialized pages does nothing, so a more reasonable way is to > > > > > just NOT call EREMOVE (your original code), since it is absolutely unnecessary. > > > > > > > > > > I don't see ANY reason we should call EREMOVE here. > > > > > > > > > > Actually w/o my patch to split EREMOVE out of sgx_free_epc_page(), it then makes > > > > > perfect sense to have new sgx_free_epc_page() here. > > > > > > > > > > > > > > > > > The two patches that I posted originally for Kai's series took EREMOVE out > > > > > > of sgx_free_epc_page() and put an explicit EREMOVE where it is actually > > > > > > needed, but for reasons unknown to me, that change is gone. > > > > > > > > > > > > > > > > It's not gone. It goes into a new sgx_encl_free_epc_page(), which is exactly the same > > > > > as current sgx_free_epc_page() which as EREMOVE, instead of putting EREMOVE into a > > > > > dedicated sgx_reset_epc_page(), as you did in your series: > > > > > > > > > > https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/ > > > > > > > > > > However, your change has side effort: it always put page back into free pool, even > > > > > EREMOVE fails. To make your change w/o having any functional change, it has to be: > > > > > > > > > > if(!sgx_reset_epc_page()) > > > > > sgx_free_epc_page(); > > > > > > > > OK, great, your patch set uses the wrapper only in the necessary call > > > > sites. Sorry, I overlooked this part. > > > > > > > > Anyway, it knowingly does that. I considered either as equally harmful > > > > side-ffects when I implemented. Either can only trigger, when there is a > > > > bug in the kernel code. > > > > > > > > It *could* do what that snippet suggest but it's like "out of the frying pan, > > > > into the fire" kind of change. > > > > > > > > Since NUMA patch set anyway requires to have a global dirty list, I think > > > > the better way to deal with this, would be to declare a new global in the > > > > patch under discussion: > > > > > > > > static struct list_head sgx_dirty_list; > > > > > > sgx_dirty_page_list > > > > Actually, I think it is good as it is now. Please do nothing :-) > > > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > I can continue from that and improve the fallback further. Not perfect, but > > good enough. > > Great. Thank you Jarkko. > > I'll add your Acked-by and repost it since I also made a mistake in copy-paste:) > Hmm.. This patch was originally from you, so it has From you, and has your SoB. It also has Co-developed-by me, but does it still require Acked-by from you? Anyway I have added it to my local. Let me know if I should remove it.
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index 52d070fb4c9a..ed99c60024dc 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. Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- arch/x86/kernel/cpu/sgx/main.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)