x86/sgx: NUMA

If sgx_page_cache_init() fails in the middle, a trivial return statement causes unused memory and virtual address space reserved for the EPC section, not freed. Fix this by using the same rollback, as when sgx_page_reclaimer_init() fails.

Quoted 4 lines · Jarkko Sakkinen

If sgx_page_cache_init() fails in the middle, a trivial return statement causes unused memory and virtual address space reserved for the EPC section, not freed. Fix this by using the same rollback, as when sgx_page_reclaimer_init() fails.

...

arch/x86/kernel/cpu/sgx/main.c static int __init sgx_init(void) · line 714

Quoted 10 lines

arch/x86/kernel/cpu/sgx/main.c +4 -2

@@ -708,8 +708,10 @@ static int __init sgx_init(void)
 	if (!cpu_feature_enabled(X86_FEATURE_SGX))
 		return -ENODEV;
 
-	if (!sgx_page_cache_init())
-		return -ENOMEM;
+	if (!sgx_page_cache_init()) {
+		ret = -ENOMEM;
+		goto err_page_cache;
+	}

Currently, the only way sgx_page_cache_init() can fail is in the case that there are no sections:

        if (!sgx_nr_epc_sections) {
                pr_err("There are zero EPC sections.\n");
                return false;
        }

That only happened if all sgx_setup_epc_section() calls failed. sgx_setup_epc_section() never both allocates memory with vmalloc for section->pages *and* fails. If sgx_setup_epc_section() has a successful memremap() but a failed vmalloc(), it cleans up with memunmap().

In other words, I see how this _looks_ like a memory leak from sgx_init(), but I don't see an actual leak in practice.

Am I missing something?

Quoted 31 lines · Dave Hansen

Quoted 4 lines · Jarkko Sakkinen

If sgx_page_cache_init() fails in the middle, a trivial return statement causes unused memory and virtual address space reserved for the EPC section, not freed. Fix this by using the same rollback, as when sgx_page_reclaimer_init() fails.

...

arch/x86/kernel/cpu/sgx/main.c static int __init sgx_init(void) · line 714

Quoted 10 lines

arch/x86/kernel/cpu/sgx/main.c +4 -2

@@ -708,8 +708,10 @@ static int __init sgx_init(void)
 	if (!cpu_feature_enabled(X86_FEATURE_SGX))
 		return -ENODEV;
 
-	if (!sgx_page_cache_init())
-		return -ENOMEM;
+	if (!sgx_page_cache_init()) {
+		ret = -ENOMEM;
+		goto err_page_cache;
+	}

Currently, the only way sgx_page_cache_init() can fail is in the case that there are no sections:

        if (!sgx_nr_epc_sections) {
                pr_err("There are zero EPC sections.\n");
                return false;
        }

That only happened if all sgx_setup_epc_section() calls failed. sgx_setup_epc_section() never both allocates memory with vmalloc for section->pages *and* fails. If sgx_setup_epc_section() has a successful memremap() but a failed vmalloc(), it cleans up with memunmap().

Quoted 4 lines

In other words, I see how this _looks_ like a memory leak from sgx_init(), but I don't see an actual leak in practice.

Am I missing something?

In sgx_setup_epc_section():

	section->pages = vmalloc(nr_pages * sizeof(struct sgx_epc_page));
	if (!section->pages) {
		memunmap(section->virt_addr);
		return false;
	}

I.e. this rollback does not happen without this fix applied:

	for (i = 0; i < sgx_nr_epc_sections; i++) {
		vfree(sgx_epc_sections[i].pages);
		memunmap(sgx_epc_sections[i].virt_addr);
	}

Quoted 46 lines · Jarkko Sakkinen

Quoted 31 lines · Dave Hansen

Quoted 4 lines · Jarkko Sakkinen

If sgx_page_cache_init() fails in the middle, a trivial return statement causes unused memory and virtual address space reserved for the EPC section, not freed. Fix this by using the same rollback, as when sgx_page_reclaimer_init() fails.

...

arch/x86/kernel/cpu/sgx/main.c static int __init sgx_init(void) · line 714

Quoted 10 lines

arch/x86/kernel/cpu/sgx/main.c +4 -2

@@ -708,8 +708,10 @@ static int __init sgx_init(void)
 	if (!cpu_feature_enabled(X86_FEATURE_SGX))
 		return -ENODEV;
 
-	if (!sgx_page_cache_init())
-		return -ENOMEM;
+	if (!sgx_page_cache_init()) {
+		ret = -ENOMEM;
+		goto err_page_cache;
+	}

Currently, the only way sgx_page_cache_init() can fail is in the case that there are no sections:

        if (!sgx_nr_epc_sections) {
                pr_err("There are zero EPC sections.\n");
                return false;
        }

That only happened if all sgx_setup_epc_section() calls failed. sgx_setup_epc_section() never both allocates memory with vmalloc for section->pages *and* fails. If sgx_setup_epc_section() has a successful memremap() but a failed vmalloc(), it cleans up with memunmap().

Quoted 4 lines

In other words, I see how this _looks_ like a memory leak from sgx_init(), but I don't see an actual leak in practice.

Am I missing something?

In sgx_setup_epc_section():

	section->pages = vmalloc(nr_pages * sizeof(struct sgx_epc_page));
	if (!section->pages) {
		memunmap(section->virt_addr);
		return false;
	}

Quoted 6 lines

I.e. this rollback does not happen without this fix applied:

	for (i = 0; i < sgx_nr_epc_sections; i++) {
		vfree(sgx_epc_sections[i].pages);
		memunmap(sgx_epc_sections[i].virt_addr);
	}

Dave is pointing out that sgx_page_cache_init() fails if and only if _all_ sections fail sgx_setup_epc_section(), and if all sections fail then sgx_nr_epc_sections is '0' and the above is a nop.

That behavior is by design, as we didn't want to kill SGX if a single section failed to initialize for whatever reason.

arch/x86/kernel/cpu/sgx/main.c +0 -1

index 52d070fb4c9a..ed99c60024dc 100644
@@ -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);

Quoted 16 lines · Jarkko Sakkinen

arch/x86/kernel/cpu/sgx/main.c +0 -1

index 52d070fb4c9a..ed99c60024dc 100644
@@ -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;
 

arch/x86/kernel/cpu/sgx/main.c static void sgx_reclaim_pages(void) · line 382

Quoted 8 lines

arch/x86/kernel/cpu/sgx/main.c +1 -5

-		section = &sgx_epc_sections[epc_page->section];
-		spin_lock(&section->lock);
-		list_add_tail(&epc_page->list, &section->page_list);
-		section->free_cnt++;
-		spin_unlock(&section->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()?

Quoted 11 lines

arch/x86/kernel/cpu/sgx/main.c static void sgx_reclaim_pages(void) · line 382

Quoted 8 lines

arch/x86/kernel/cpu/sgx/main.c +1 -5

-		section = &sgx_epc_sections[epc_page->section];
-		spin_lock(&section->lock);
-		list_add_tail(&epc_page->list, &section->page_list);
-		section->free_cnt++;
-		spin_unlock(&section->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.

Quoted 15 lines · Jarkko Sakkinen

Quoted 11 lines

arch/x86/kernel/cpu/sgx/main.c static void sgx_reclaim_pages(void) · line 382

Quoted 8 lines

arch/x86/kernel/cpu/sgx/main.c +1 -5

-		section = &sgx_epc_sections[epc_page->section];
-		spin_lock(&section->lock);
-		list_add_tail(&epc_page->list, &section->page_list);
-		section->free_cnt++;
-		spin_unlock(&section->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.

Quoted 7 lines

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?

Quoted 24 lines · Dave Hansen

Quoted 23 lines · Jarkko Sakkinen

arch/x86/kernel/cpu/sgx/main.c +0 -1

index 52d070fb4c9a..ed99c60024dc 100644
@@ -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;

Quoted 12 lines

Quoted 8 lines

arch/x86/kernel/cpu/sgx/main.c +1 -5

-		section = &sgx_epc_sections[epc_page->section];
-		spin_lock(&section->lock);
-		list_add_tail(&epc_page->list, &section->page_list);
-		section->free_cnt++;
-		spin_unlock(&section->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/[email protected]/

Quoted 2 lines

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

Quoted 38 lines · Jarkko Sakkinen

Quoted 24 lines · Dave Hansen

Quoted 23 lines · Jarkko Sakkinen

arch/x86/kernel/cpu/sgx/main.c +0 -1

index 52d070fb4c9a..ed99c60024dc 100644
@@ -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;

Quoted 12 lines

Quoted 8 lines

arch/x86/kernel/cpu/sgx/main.c +1 -5

-		section = &sgx_epc_sections[epc_page->section];
-		spin_lock(&section->lock);
-		list_add_tail(&epc_page->list, &section->page_list);
-		section->free_cnt++;
-		spin_unlock(&section->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.

Quoted 3 lines

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.

Quoted 5 lines

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/[email protected]/

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

Quoted 61 lines · Kai Huang

Quoted 38 lines · Jarkko Sakkinen

Quoted 24 lines · Dave Hansen

Quoted 23 lines · Jarkko Sakkinen

arch/x86/kernel/cpu/sgx/main.c +0 -1

index 52d070fb4c9a..ed99c60024dc 100644
@@ -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;

Quoted 12 lines

Quoted 8 lines

arch/x86/kernel/cpu/sgx/main.c +1 -5

-		section = &sgx_epc_sections[epc_page->section];
-		spin_lock(&section->lock);
-		list_add_tail(&epc_page->list, &section->page_list);
-		section->free_cnt++;
-		spin_unlock(&section->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.

Quoted 3 lines

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.

Quoted 5 lines

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:

Quoted 7 lines

https://lore.kernel.org/linux-sgx/[email protected]/

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.

Quoted 3 lines

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.

Quoted 5 lines

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

Quoted 80 lines · Jarkko Sakkinen

Quoted 61 lines · Kai Huang

Quoted 38 lines · Jarkko Sakkinen

Quoted 24 lines · Dave Hansen

Quoted 23 lines · Jarkko Sakkinen

arch/x86/kernel/cpu/sgx/main.c +0 -1

index 52d070fb4c9a..ed99c60024dc 100644
@@ -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;

Quoted 12 lines

Quoted 8 lines

arch/x86/kernel/cpu/sgx/main.c +1 -5

-		section = &sgx_epc_sections[epc_page->section];
-		spin_lock(&section->lock);
-		list_add_tail(&epc_page->list, &section->page_list);
-		section->free_cnt++;
-		spin_unlock(&section->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.

Quoted 3 lines

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.

Quoted 5 lines

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:

Quoted 7 lines

https://lore.kernel.org/linux-sgx/[email protected]/

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.

Quoted 5 lines

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

Quoted 87 lines · Jarkko Sakkinen

Quoted 85 lines · Jarkko Sakkinen

Quoted 61 lines · Kai Huang

Quoted 38 lines · Jarkko Sakkinen

Quoted 24 lines · Dave Hansen

Quoted 23 lines · Jarkko Sakkinen

arch/x86/kernel/cpu/sgx/main.c +0 -1

index 52d070fb4c9a..ed99c60024dc 100644
@@ -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;

Quoted 12 lines

Quoted 8 lines

arch/x86/kernel/cpu/sgx/main.c +1 -5

-		section = &sgx_epc_sections[epc_page->section];
-		spin_lock(&section->lock);
-		list_add_tail(&epc_page->list, &section->page_list);
-		section->free_cnt++;
-		spin_unlock(&section->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.

Quoted 3 lines

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.

Quoted 5 lines

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:

Quoted 7 lines

https://lore.kernel.org/linux-sgx/[email protected]/

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;

Quoted 1 line

sgx_dirty_page_list

Actually, I think it is good as it is now. Please do nothing :-)

Acked-by Jarkko Sakkinen <[email protected]>

I can continue from that and improve the fallback further. Not perfect, but good enough.

Quoted 92 lines · Jarkko Sakkinen

Quoted 81 lines · Jarkko Sakkinen

Quoted 80 lines · Jarkko Sakkinen

Quoted 61 lines · Kai Huang

Quoted 38 lines · Jarkko Sakkinen

Quoted 24 lines · Dave Hansen

Quoted 23 lines · Jarkko Sakkinen

arch/x86/kernel/cpu/sgx/main.c +0 -1

index 52d070fb4c9a..ed99c60024dc 100644
@@ -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;

Quoted 12 lines

Quoted 8 lines

arch/x86/kernel/cpu/sgx/main.c +1 -5

-		section = &sgx_epc_sections[epc_page->section];
-		spin_lock(&section->lock);
-		list_add_tail(&epc_page->list, &section->page_list);
-		section->free_cnt++;
-		spin_unlock(&section->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.

Quoted 3 lines

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.

Quoted 5 lines

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:

Quoted 7 lines

https://lore.kernel.org/linux-sgx/[email protected]/

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.

Quoted 7 lines

Quoted 5 lines

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 :-)

Quoted 4 lines

Acked-by Jarkko Sakkinen <[email protected]>

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:)

arch/x86/kernel/cpu/sgx/main.c +11 -12

 	struct sgx_epc_page *page;
 	LIST_HEAD(dirty);
 	int ret;
 
 	/* init_laundry_list is thread-local, no need for a lock: */
-	while (!list_empty(&section->init_laundry_list)) {
+	while (!list_empty(laundry)) {
 		if (kthread_should_stop())
 			return;
 
-		/* needed for access to ->page_list: */
-		spin_lock(&section->lock);
-
-		page = list_first_entry(&section->init_laundry_list,
-					struct sgx_epc_page, list);
+		page = list_first_entry(laundry, struct sgx_epc_page, list);
 
 		ret = __eremove(sgx_get_epc_virt_addr(page));
-		if (!ret)
-			list_move(&page->list, &section->page_list);
-		else
+		if (!ret) {
+			/* The page is clean - move to the free list. */
+			list_del(&page->list);
+			sgx_free_epc_page(page);
+		} else {
+			/* The page is not yet clean - move to the dirty list. */
 			list_move_tail(&page->list, &dirty);
-
-		spin_unlock(&section->lock);
+		}
 
 		cond_resched();
 	}
 
-	list_splice(&dirty, &section->init_laundry_list);
+	list_splice(&dirty, laundry);
 }
 
 static bool sgx_reclaimer_age(struct sgx_epc_page *epc_page)

@@ -400,6 +398,7 @@ static bool sgx_should_reclaim(unsigned long watermark)
 
 static int ksgxd(void *p)
 {
+	struct list_head *laundry = p;
 	int i;
 
 	set_freezable();

arch/x86/kernel/cpu/sgx/main.c +2 -2

 	while (!kthread_should_stop()) {
 		if (try_to_freeze())

@@ -436,11 +432,11 @@ static int ksgxd(void *p)
 	return 0;
 }
 
-static bool __init sgx_page_reclaimer_init(void)
+static bool __init sgx_page_reclaimer_init(struct list_head *laundry)
 {
 	struct task_struct *tsk;
 
-	tsk = kthread_run(ksgxd, NULL, "ksgxd");
+	tsk = kthread_run(ksgxd, laundry, "ksgxd");
 	if (IS_ERR(tsk))
 		return false;
 

arch/x86/kernel/cpu/sgx/main.c +2 -3

 	unsigned long nr_pages = size >> PAGE_SHIFT;
 	unsigned long i;

@@ -632,13 +629,12 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
 	section->phys_addr = phys_addr;
 	spin_lock_init(&section->lock);
 	INIT_LIST_HEAD(&section->page_list);
-	INIT_LIST_HEAD(&section->init_laundry_list);
 
 	for (i = 0; i < nr_pages; i++) {
 		section->pages[i].section = index;
 		section->pages[i].flags = 0;
 		section->pages[i].owner = NULL;
-		list_add_tail(&section->pages[i].list, &section->init_laundry_list);
+		list_add_tail(&section->pages[i].list, laundry);
 	}
 
 	section->free_cnt = nr_pages;

@@ -656,7 +652,7 @@ static inline u64 __init sgx_calc_section_metric(u64 low, u64 high)
 	       ((high & GENMASK_ULL(19, 0)) << 32);
 }
 
-static bool __init sgx_page_cache_init(void)
+static bool __init sgx_page_cache_init(struct list_head *laundry)
 {
 	u32 eax, ebx, ecx, edx, type;
 	u64 pa, size;

arch/x86/kernel/cpu/sgx/main.c +7 -1

@@ -697,18 +693,25 @@ static bool __init sgx_page_cache_init(void)
 
 static int __init sgx_init(void)
 {
+	struct list_head *laundry;
 	int ret;
 	int i;
 
 	if (!cpu_feature_enabled(X86_FEATURE_SGX))
 		return -ENODEV;
 
-	if (!sgx_page_cache_init()) {
+	laundry = kzalloc(sizeof(*laundry), GFP_KERNEL);
+	if (!laundry)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(laundry);
+

...

arch/x86/kernel/cpu/sgx/main.c static DEFINE_SPINLOCK(sgx_reclaimer_lock); · line 34

Quoted 3 lines

arch/x86/kernel/cpu/sgx/main.c +1 -1

-static void sgx_sanitize_section(struct sgx_epc_section *section)
+static void sgx_sanitize_section(struct list_head *laundry)
 {

Does this need a better function name now that it's not literally dealing with sections at *all*?

sgx_sanitize_pages()

perhaps.

Quoted 47 lines

arch/x86/kernel/cpu/sgx/main.c +11 -12

 	struct sgx_epc_page *page;
 	LIST_HEAD(dirty);
 	int ret;
 
 	/* init_laundry_list is thread-local, no need for a lock: */
-	while (!list_empty(&section->init_laundry_list)) {
+	while (!list_empty(laundry)) {
 		if (kthread_should_stop())
 			return;
 
-		/* needed for access to ->page_list: */
-		spin_lock(&section->lock);
-
-		page = list_first_entry(&section->init_laundry_list,
-					struct sgx_epc_page, list);
+		page = list_first_entry(laundry, struct sgx_epc_page, list);
 
 		ret = __eremove(sgx_get_epc_virt_addr(page));
-		if (!ret)
-			list_move(&page->list, &section->page_list);
-		else
+		if (!ret) {
+			/* The page is clean - move to the free list. */
+			list_del(&page->list);
+			sgx_free_epc_page(page);
+		} else {
+			/* The page is not yet clean - move to the dirty list. */
 			list_move_tail(&page->list, &dirty);
-
-		spin_unlock(&section->lock);
+		}
 
 		cond_resched();
 	}
 
-	list_splice(&dirty, &section->init_laundry_list);
+	list_splice(&dirty, laundry);
 }
 
 static bool sgx_reclaimer_age(struct sgx_epc_page *epc_page)

@@ -400,6 +398,7 @@ static bool sgx_should_reclaim(unsigned long watermark)
 
 static int ksgxd(void *p)
 {
+	struct list_head *laundry = p;
 	int i;
 

arch/x86/kernel/cpu/sgx/main.c static int ksgxd(void *p) · line 411

Quoted 9 lines

arch/x86/kernel/cpu/sgx/main.c +2 -2

 	set_freezable();

@@ -408,16 +407,13 @@ static int ksgxd(void *p)
 	 * Sanitize pages in order to recover from kexec(). The 2nd pass is
 	 * required for SECS pages, whose child pages blocked EREMOVE.
 	 */
-	for (i = 0; i < sgx_nr_epc_sections; i++)
-		sgx_sanitize_section(&sgx_epc_sections[i]);
+	sgx_sanitize_section(laundry);
+	sgx_sanitize_section(laundry);

Did you intend to call this twice?

arch/x86/kernel/cpu/sgx/main.c static int ksgxd(void *p) · line 416

Quoted 10 lines

arch/x86/kernel/cpu/sgx/main.c +3 -6

-	for (i = 0; i < sgx_nr_epc_sections; i++) {
-		sgx_sanitize_section(&sgx_epc_sections[i]);
+	if (!list_empty(laundry))
+		WARN(1, "EPC section %d has unsanitized pages.\n", i);
 
-		/* Should never happen. */
-		if (!list_empty(&sgx_epc_sections[i].init_laundry_list))
-			WARN(1, "EPC section %d has unsanitized pages.\n", i);
-	}
+	kfree(laundry);

This is a bit unfortunate. 'laundry' is allocated up in another thread and the lifetime isn't obvious. It's just 32 bytes, but this is just asking to be leaked.

Quoted 18 lines

arch/x86/kernel/cpu/sgx/main.c +2 -2

 	while (!kthread_should_stop()) {
 		if (try_to_freeze())

@@ -436,11 +432,11 @@ static int ksgxd(void *p)
 	return 0;
 }
 
-static bool __init sgx_page_reclaimer_init(void)
+static bool __init sgx_page_reclaimer_init(struct list_head *laundry)
 {
 	struct task_struct *tsk;
 
-	tsk = kthread_run(ksgxd, NULL, "ksgxd");
+	tsk = kthread_run(ksgxd, laundry, "ksgxd");
 	if (IS_ERR(tsk))
 		return false;
 

@@ -614,7 +610,8 @@ void sgx_free_epc_page(struct sgx_epc_page *page)
 

arch/x86/kernel/cpu/sgx/main.c void sgx_free_epc_page(struct sgx_epc_page *page) · line 615

Quoted 6 lines

arch/x86/kernel/cpu/sgx/main.c +2 -1

 static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
 					 unsigned long index,
-					 struct sgx_epc_section *section)
+					 struct sgx_epc_section *section,
+					 struct list_head *laundry)
 {

I think this at least need a comment somewhere about what this function is doing with 'laundry'.

Quoted 27 lines

arch/x86/kernel/cpu/sgx/main.c +2 -3

 	unsigned long nr_pages = size >> PAGE_SHIFT;
 	unsigned long i;

@@ -632,13 +629,12 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
 	section->phys_addr = phys_addr;
 	spin_lock_init(&section->lock);
 	INIT_LIST_HEAD(&section->page_list);
-	INIT_LIST_HEAD(&section->init_laundry_list);
 
 	for (i = 0; i < nr_pages; i++) {
 		section->pages[i].section = index;
 		section->pages[i].flags = 0;
 		section->pages[i].owner = NULL;
-		list_add_tail(&section->pages[i].list, &section->init_laundry_list);
+		list_add_tail(&section->pages[i].list, laundry);
 	}
 
 	section->free_cnt = nr_pages;

@@ -656,7 +652,7 @@ static inline u64 __init sgx_calc_section_metric(u64 low, u64 high)
 	       ((high & GENMASK_ULL(19, 0)) << 32);
 }
 
-static bool __init sgx_page_cache_init(void)
+static bool __init sgx_page_cache_init(struct list_head *laundry)
 {
 	u32 eax, ebx, ecx, edx, type;
 	u64 pa, size;

arch/x86/kernel/cpu/sgx/main.c static bool __init sgx_page_cache_init(void) · line 681

Quoted 8 lines

arch/x86/kernel/cpu/sgx/main.c +1 -1

@@ -679,7 +675,7 @@ static bool __init sgx_page_cache_init(void)
 
 		pr_info("EPC section 0x%llx-0x%llx\n", pa, pa + size - 1);
 
-		if (!sgx_setup_epc_section(pa, size, i, &sgx_epc_sections[i])) {
+		if (!sgx_setup_epc_section(pa, size, i, &sgx_epc_sections[i], laundry)) {
 			pr_err("No free memory for an EPC section\n");
 			break;
 		}

This is a great place for a comment about what is coming back on 'laundry'.

Quoted 18 lines

arch/x86/kernel/cpu/sgx/main.c +7 -1

@@ -697,18 +693,25 @@ static bool __init sgx_page_cache_init(void)
 
 static int __init sgx_init(void)
 {
+	struct list_head *laundry;
 	int ret;
 	int i;
 
 	if (!cpu_feature_enabled(X86_FEATURE_SGX))
 		return -ENODEV;
 
-	if (!sgx_page_cache_init()) {
+	laundry = kzalloc(sizeof(*laundry), GFP_KERNEL);
+	if (!laundry)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(laundry);
+

arch/x86/kernel/cpu/sgx/main.c static bool __init sgx_page_cache_init(void) · line 717

Quoted 10 lines

arch/x86/kernel/cpu/sgx/main.c +2 -1

+	if (!sgx_page_cache_init(laundry)) {
 		ret = -ENOMEM;
 		goto err_page_cache;
 	}
 
-	if (!sgx_page_reclaimer_init()) {
+	if (!sgx_page_reclaimer_init(laundry)) {
 		ret = -ENOMEM;
 		goto err_page_cache;
 	}

I really don't like this being dynamically allocated, especially since it's freed in another task in a non-obvious place.

Wouldn't this all just be a lot simpler if we had a global list_head? That will eat a whopping 16 bytes of space.

Background
==========

EPC section is covered by one or more SRAT entries that are associated with one and only one PXM (NUMA node). The current implementation overheats a

single NUMA node, because sgx_alloc_epc_page() always starts looking for pages from the same EPC section everytime.

Only within a section it does pick pages in FIFO fashion, i.e. the oldest freed in that section is the EPC page given back to the caller. That does not do any good, as the pages in the same node are performance-wise equal.

Replace local lists with a single global free page list, from which pages are borrowed in FIFO fashion.

Quoted 5 lines · Jarkko Sakkinen

Background
==========

EPC section is covered by one or more SRAT entries that are associated with one and only one PXM (NUMA node). The current implementation overheats a

Overheats?

Quoted 2 lines

single NUMA node, because sgx_alloc_epc_page() always starts looking for pages from the same EPC section everytime.

"every time"

Quoted 3 lines

Only within a section it does pick pages in FIFO fashion, i.e. the oldest freed in that section is the EPC page given back to the caller. That does not do any good, as the pages in the same node are performance-wise equal.

I'm not sure why all of this is relevant and it doesn't really tell me anything about this patch's place in the *series*.

Why are we destroying all of the per-node structures just before adding NUMA support?

Background
==========

EPC section is covered by one or more SRAT entries that are associated with one and only one PXM (NUMA node). The motivation behind this patch is to provide basic elements of building allocation scheme based on this premise.

Use phys_to_target_node() to associate each NUMA node with the EPC sections contained within its range. In sgx_alloc_epc_page(), first try to allocate from the NUMA node, where the CPU is executing. If that fails, fallback to the legacy allocation.

Link https://lore.kernel.org/lkml/158188326978.894464.217282995221175417.stgit@dwillia2-desk3.amr.corp.intel.com/ open here

Signed-off-by Jarkko Sakkinen <[email protected]>

---
 arch/x86/Kconfig               |  1 +
 arch/x86/kernel/cpu/sgx/main.c | 84 ++++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/sgx.h  |  9 ++++
 3 files changed, 94 insertions(+)

arch/x86/Kconfig +0 -0

index a5f6a3013138..7eb1e96cfe8a 100644

arch/x86/Kconfig +21 -0

+	if (list_empty(&sgx_node->free_page_list)) {
+		spin_unlock(&sgx_free_page_list_lock);
+		return NULL;
+	}
+
+	page = list_first_entry(&sgx_node->free_page_list, struct sgx_epc_page, numa_list);
+	list_del_init(&page->numa_list);
+	list_del_init(&page->list);
+	sgx_nr_free_pages--;
+
+	spin_unlock(&sgx_free_page_list_lock);
+
+	return page;
+}
+
 /**
  * __sgx_alloc_epc_page() - Allocate an EPC page
  *

@@ -446,8 +493,14 @@ static bool __init sgx_page_reclaimer_init(struct list_head *laundry)
  */
 struct sgx_epc_page *__sgx_alloc_epc_page(void)
 {
+	int current_nid = numa_node_id();
 	struct sgx_epc_page *page;
 
+	/* Try to allocate EPC from the current node, first: */
+	page = __sgx_alloc_epc_page_from_node(current_nid);
+	if (page)
+		return page;
+
 	spin_lock(&sgx_free_page_list_lock);
 
 	if (list_empty(&sgx_free_page_list)) {

arch/x86/Kconfig +0 -0

 void sgx_free_epc_page(struct sgx_epc_page *page)

{

arch/x86/Kconfig +10 -0

+	int nid = sgx_section_to_numa_node_id[page->section];
+	struct sgx_numa_node *sgx_node = &sgx_numa_nodes[nid];
 	int ret;
 
 	WARN_ON_ONCE(page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);

@@ -575,7 +631,15 @@ void sgx_free_epc_page(struct sgx_epc_page *page)
 		return;
 
 	spin_lock(&sgx_free_page_list_lock);
+
+	/*   Enable NUMA local allocation in sgx_alloc_epc_page(). */
+	if (!node_isset(nid, sgx_numa_mask)) {
+		INIT_LIST_HEAD(&sgx_node->free_page_list);
+		node_set(nid, sgx_numa_mask);
+	}
+
 	list_add_tail(&page->list, &sgx_free_page_list);
+	list_add_tail(&page->numa_list, &sgx_node->free_page_list);
 	sgx_nr_free_pages++;
 	spin_unlock(&sgx_free_page_list_lock);
 }

arch/x86/Kconfig +16 -0

+	/*
+	 * Create NUMA node lookup table for sgx_free_epc_page() as the very
+	 * first step, as it is used to populate the free list's during the
+	 * initialization.
+	 */
+	for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) {
+		nid = numa_map_to_online_node(phys_to_target_node(pa));
+		if (nid == NUMA_NO_NODE) {
+			/* The physical address is already printed above. */
+			pr_warn(FW_BUG "Unable to map EPC section to online node. Fallback to the NUMA node 0.\n");
+			nid = 0;
+		}
+
+		sgx_section_to_numa_node_id[i] = nid;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) {
 		cpuid_count(SGX_CPUID, i + SGX_CPUID_EPC, &eax, &ebx, &ecx, &edx);
 

arch/x86/kernel/cpu/sgx/sgx.h +0 -0

index 41ca045a574a..3a3c07fc0c8e 100644

What changed from the last patch?

Quoted 6 lines · Jarkko Sakkinen

Background
==========

EPC section is covered by one or more SRAT entries that are associated with one and only one PXM (NUMA node). The motivation behind this patch is to provide basic elements of building allocation scheme based on this premise.

Just like normal RAM, enclave memory (EPC) should be covered by entries in the ACPI SRAT table. These entries allow each EPC section to be associated with a NUMA node.

Use this information to implement a simple NUMA-aware allocator for enclave memory.

Quoted 4 lines

Use phys_to_target_node() to associate each NUMA node with the EPC sections contained within its range. In sgx_alloc_epc_page(), first try to allocate from the NUMA node, where the CPU is executing. If that fails, fallback to the legacy allocation.

By "legacy", you mean the one from the last patch? :)

Quoted 13 lines

Link https://lore.kernel.org/lkml/158188326978.894464.217282995221175417.stgit@dwillia2-desk3.amr.corp.intel.com/ open here

Signed-off-by Jarkko Sakkinen <[email protected]>

---
 arch/x86/Kconfig               |  1 +
 arch/x86/kernel/cpu/sgx/main.c | 84 ++++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/sgx.h  |  9 ++++
 3 files changed, 94 insertions(+)

arch/x86/Kconfig +0 -0

index a5f6a3013138..7eb1e96cfe8a 100644

arch/x86/Kconfig config X86_SGX · line 1943

Quoted 4 lines

arch/x86/Kconfig +1 -0

@@ -1940,6 +1940,7 @@ config X86_SGX
 	depends on CRYPTO_SHA256=y
 	select SRCU
 	select MMU_NOTIFIER
+	select NUMA_KEEP_MEMINFO if NUMA

This dependency is worth mentioning somewhere. Why do we suddenly need NUMA_KEEP_MEMINFO?

arch/x86/Kconfig config X86_SGX

Quoted 6 lines

arch/x86/Kconfig +6 -0

+/* Nodes with one or more EPC sections. */
+static nodemask_t sgx_numa_mask;
+
+/*
+ * Array with one list_head for each possible NUMA node.  Each
+ * list contains all the sgx_epc_section's which are on that

					   ^ no "'", please

arch/x86/Kconfig

Quoted 9 lines

+ * node.
+ */

arch/x86/Kconfig +7 -0

+static struct sgx_numa_node *sgx_numa_nodes;
+
+/*
+ * sgx_free_epc_page() uses this to find out the correct struct sgx_numa_node,
+ * to put the page in.
+ */
+static int sgx_section_to_numa_node_id[SGX_MAX_EPC_SECTIONS];

If this is per-section, why not put it in struct sgx_epc_section?

Quoted 6 lines

/*

arch/x86/Kconfig +0 -0

@@ -434,6 +451,36 @@ static bool __init sgx_page_reclaimer_init(struct list_head *laundry)
 	return true;
 }
 

arch/x86/Kconfig static bool __init sgx_page_reclaimer_init(struct list_head *laundry) · line 460

Quoted 7 lines

arch/x86/Kconfig +7 -0

+static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
+{
+	struct sgx_epc_page *page = NULL;
+	struct sgx_numa_node *sgx_node;
+
+	if (WARN_ON_ONCE(nid < 0 || nid >= num_possible_nodes()))
+		return NULL;

This has exactly one call-site which plumbs numa_node_id() in here pretty directly. Is this check worthwhile?

arch/x86/Kconfig static bool __init sgx_page_reclaimer_init(struct list_head *laundry) · line 466

Quoted 6 lines

arch/x86/Kconfig +6 -0

+	if (!node_isset(nid, sgx_numa_mask))
+		return NULL;
+
+	sgx_node = &sgx_numa_nodes[nid];
+
+	spin_lock(&sgx_free_page_list_lock);

The glocal lock protecting a per-node structure is a bit unsightly.

Quoted 34 lines

arch/x86/Kconfig +21 -0

+	if (list_empty(&sgx_node->free_page_list)) {
+		spin_unlock(&sgx_free_page_list_lock);
+		return NULL;
+	}
+
+	page = list_first_entry(&sgx_node->free_page_list, struct sgx_epc_page, numa_list);
+	list_del_init(&page->numa_list);
+	list_del_init(&page->list);
+	sgx_nr_free_pages--;
+
+	spin_unlock(&sgx_free_page_list_lock);
+
+	return page;
+}
+
 /**
  * __sgx_alloc_epc_page() - Allocate an EPC page
  *

@@ -446,8 +493,14 @@ static bool __init sgx_page_reclaimer_init(struct list_head *laundry)
  */
 struct sgx_epc_page *__sgx_alloc_epc_page(void)
 {
+	int current_nid = numa_node_id();
 	struct sgx_epc_page *page;
 
+	/* Try to allocate EPC from the current node, first: */
+	page = __sgx_alloc_epc_page_from_node(current_nid);
+	if (page)
+		return page;
+
 	spin_lock(&sgx_free_page_list_lock);
 
 	if (list_empty(&sgx_free_page_list)) {

arch/x86/Kconfig struct sgx_epc_page *__sgx_alloc_epc_page(void) · line 514

Quoted 6 lines

arch/x86/Kconfig +1 -0

@@ -456,6 +509,7 @@ struct sgx_epc_page *__sgx_alloc_epc_page(void)
 	}
 
 	page = list_first_entry(&sgx_free_page_list, struct sgx_epc_page, list);
+	list_del_init(&page->numa_list);
 	list_del_init(&page->list);
 	sgx_nr_free_pages--;

I would much rather prefer that this does what the real page allocator
does: kep the page on a single list. That list is maintained per-NUMA-node. Allocations try local NUMA node structures, then fall back to other structures (hopefully in a locality-aware fashion).

I wrote you the loop that I want to see this implement in an earlier review. This, basically:

	page = NULL;
	nid = numa_node_id();
	while (true) {
		page = __sgx_alloc_epc_page_from_node(nid);	
		if (page)
			break;

		nid = // ... some search here, next_node_in()...
		// check if we wrapped around:
		if (nid == numa_node_id())
			break;
	}

There's no global list. You just walk around nodes trying to find one with space. If you wrap around, you stop.

Please implement this. If you think it's a bad idea, or can't, let's talk about it in advance. Right now, it appears that my review comments aren't being incorporated into newer versions.

Quoted 30 lines

arch/x86/Kconfig +0 -0

 void sgx_free_epc_page(struct sgx_epc_page *page)

{

arch/x86/Kconfig +11 -0

+	int nid = sgx_section_to_numa_node_id[page->section];
+	struct sgx_numa_node *sgx_node = &sgx_numa_nodes[nid];
 	int ret;
 
 	WARN_ON_ONCE(page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);

@@ -575,7 +631,15 @@ void sgx_free_epc_page(struct sgx_epc_page *page)
 		return;
 
 	spin_lock(&sgx_free_page_list_lock);
+
+	/*   Enable NUMA local allocation in sgx_alloc_epc_page(). */
+	if (!node_isset(nid, sgx_numa_mask)) {
+		INIT_LIST_HEAD(&sgx_node->free_page_list);
+		node_set(nid, sgx_numa_mask);
+	}
+
 	list_add_tail(&page->list, &sgx_free_page_list);
+	list_add_tail(&page->numa_list, &sgx_node->free_page_list);
 	sgx_nr_free_pages++;
 	spin_unlock(&sgx_free_page_list_lock);
 }

@@ -626,8 +690,28 @@ static bool __init sgx_page_cache_init(struct list_head *laundry)
 {
 	u32 eax, ebx, ecx, edx, type;
 	u64 pa, size;
+	int nid;
 	int i;
 

arch/x86/Kconfig static bool __init sgx_page_cache_init(struct list_head *laundry) · line 696

Quoted 1 line

arch/x86/Kconfig +1 -0

+	nodes_clear(sgx_numa_mask);

Is this really required for a variable allocated in .bss?

arch/x86/Kconfig static bool __init sgx_page_cache_init(struct list_head *laundry) · line 697

Quoted 1 line

arch/x86/Kconfig +1 -0

+	sgx_numa_nodes = kmalloc_array(num_possible_nodes(), sizeof(*sgx_numa_nodes), GFP_KERNEL);

This is what I was looking for here, thanks!

Quoted 24 lines

arch/x86/Kconfig +16 -0

+	/*
+	 * Create NUMA node lookup table for sgx_free_epc_page() as the very
+	 * first step, as it is used to populate the free list's during the
+	 * initialization.
+	 */
+	for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) {
+		nid = numa_map_to_online_node(phys_to_target_node(pa));
+		if (nid == NUMA_NO_NODE) {
+			/* The physical address is already printed above. */
+			pr_warn(FW_BUG "Unable to map EPC section to online node. Fallback to the NUMA node 0.\n");
+			nid = 0;
+		}
+
+		sgx_section_to_numa_node_id[i] = nid;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) {
 		cpuid_count(SGX_CPUID, i + SGX_CPUID_EPC, &eax, &ebx, &ecx, &edx);
 

arch/x86/kernel/cpu/sgx/sgx.h +0 -0

index 41ca045a574a..3a3c07fc0c8e 100644

arch/x86/kernel/cpu/sgx/sgx.h struct sgx_epc_page { · line 31

Quoted 5 lines

arch/x86/kernel/cpu/sgx/sgx.h +1 -0

@@ -27,6 +27,7 @@ struct sgx_epc_page {
 	unsigned int flags;
 	struct sgx_encl_page *owner;
 	struct list_head list;
+	struct list_head numa_list;
 };

I'll say it again, explicitly: Each sgx_epc_page should be on one and only one free list: a per-NUMA-node list.

Quoted 5 lines

arch/x86/kernel/cpu/sgx/sgx.h +0 -0

 /*

@@ -43,6 +44,14 @@ struct sgx_epc_section {
 
 extern struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS];
 

arch/x86/kernel/cpu/sgx/sgx.h struct sgx_epc_section { · line 53

Quoted 7 lines

arch/x86/kernel/cpu/sgx/sgx.h +7 -0

+/*
+ * Contains the tracking data for NUMA nodes having EPC pages. Most importantly,
+ * the free page list local to the node is stored here.
+ */
+struct sgx_numa_node {
+	struct list_head free_page_list;
+};

I think it's unconscionable to leave this protected by a global lock. Please at least give us a per-node spinlock proteting this list.

Quoted 11 lines

arch/x86/kernel/cpu/sgx/main.c static DEFINE_SPINLOCK(sgx_reclaimer_lock); · line 43

Quoted 9 lines

+ * node.
+ */

arch/x86/kernel/cpu/sgx/main.c +7 -0

+static struct sgx_numa_node *sgx_numa_nodes;
+
+/*
+ * sgx_free_epc_page() uses this to find out the correct struct sgx_numa_node,
+ * to put the page in.
+ */
+static int sgx_section_to_numa_node_id[SGX_MAX_EPC_SECTIONS];

If this is per-section, why not put it in struct sgx_epc_section?

Because struct sgx_epc_page does not contain a pointer to struct sgx_epc_section.

Quoted 26 lines

arch/x86/kernel/cpu/sgx/main.c struct sgx_epc_page *__sgx_alloc_epc_page(void) · line 514

Quoted 4 lines

arch/x86/kernel/cpu/sgx/main.c +1 -0

 	page = list_first_entry(&sgx_free_page_list, struct sgx_epc_page, list);
+	list_del_init(&page->numa_list);
 	list_del_init(&page->list);
 	sgx_nr_free_pages--;

I would much rather prefer that this does what the real page allocator
does: kep the page on a single list. That list is maintained per-NUMA-node. Allocations try local NUMA node structures, then fall back to other structures (hopefully in a locality-aware fashion).

I wrote you the loop that I want to see this implement in an earlier review. This, basically:

	page = NULL;
	nid = numa_node_id();
	while (true) {
		page = __sgx_alloc_epc_page_from_node(nid);	
		if (page)
			break;

		nid = // ... some search here, next_node_in()...
		// check if we wrapped around:
		if (nid == numa_node_id())
			break;
	}

Quoted 6 lines

There's no global list. You just walk around nodes trying to find one with space. If you wrap around, you stop.

Please implement this. If you think it's a bad idea, or can't, let's talk about it in advance. Right now, it appears that my review comments aren't being incorporated into newer versions.

How I interpreted your earlier comments is that the fallback is unfair and this patch set version does fix that.

I can buy the above allocation scheme, but I don't think this patch set version is a step backwards. The things done to struct sgx_epc_section are exactly what should be done to it.

Implementation-wise you are asking me to squash 4/5 and 5/5 into a single patch, and remove global list. It's a tiny iteration from this patch version and I can do it.

Quoted 12 lines

Quoted 11 lines

arch/x86/kernel/cpu/sgx/main.c static DEFINE_SPINLOCK(sgx_reclaimer_lock); · line 43

Quoted 9 lines

+ * node.
+ */

arch/x86/kernel/cpu/sgx/main.c +7 -0

+static struct sgx_numa_node *sgx_numa_nodes;
+
+/*
+ * sgx_free_epc_page() uses this to find out the correct struct sgx_numa_node,
+ * to put the page in.
+ */
+static int sgx_section_to_numa_node_id[SGX_MAX_EPC_SECTIONS];

If this is per-section, why not put it in struct sgx_epc_section?

Quoted 2 lines

Because struct sgx_epc_page does not contain a pointer to struct sgx_epc_section.

Currently, we have epc_page->section. That's not changing. But, with the free list moving from sgx_epc_section to sgx_numa_node, we need a way to get from page->node, not just page->section.

We can either add that to:

	struct sgx_epc_section {
		...

(quoted diff) +1 -0

+		struct sgx_numa_node *node;

	}

so we can do epc_page->section->node to find the epc_page's free list, or we could just do:

struct sgx_epc_page {

arch/x86/kernel/cpu/sgx/sgx.h +1 -1

- 	unsigned int section;
+ 	unsigned int node;
 	unsigned int flags;
 	struct sgx_encl_page *owner;
 	struct list_head list;

	struct list_head numa_list;

};

and go from page->node directly.

Quoted 33 lines

Quoted 26 lines

arch/x86/kernel/cpu/sgx/main.c struct sgx_epc_page *__sgx_alloc_epc_page(void) · line 514

Quoted 4 lines

arch/x86/kernel/cpu/sgx/main.c +1 -0

 	page = list_first_entry(&sgx_free_page_list, struct sgx_epc_page, list);
+	list_del_init(&page->numa_list);
 	list_del_init(&page->list);
 	sgx_nr_free_pages--;

I would much rather prefer that this does what the real page allocator
does: kep the page on a single list. That list is maintained per-NUMA-node. Allocations try local NUMA node structures, then fall back to other structures (hopefully in a locality-aware fashion).

I wrote you the loop that I want to see this implement in an earlier review. This, basically:

	page = NULL;
	nid = numa_node_id();
	while (true) {
		page = __sgx_alloc_epc_page_from_node(nid);	
		if (page)
			break;

		nid = // ... some search here, next_node_in()...
		// check if we wrapped around:
		if (nid == numa_node_id())
			break;
	}

Quoted 6 lines

There's no global list. You just walk around nodes trying to find one with space. If you wrap around, you stop.

Please implement this. If you think it's a bad idea, or can't, let's talk about it in advance. Right now, it appears that my review comments aren't being incorporated into newer versions.

Quoted 6 lines

How I interpreted your earlier comments is that the fallback is unfair and this patch set version does fix that.

I can buy the above allocation scheme, but I don't think this patch set version is a step backwards. The things done to struct sgx_epc_section are exactly what should be done to it.

To me, it's a step backwards. It regresses in that it falls back to an entirely non-NUMA aware allocation mechanism. The global list is actually likely to be even worse than the per-section searches because it has a global lock as opposed to the at per-section locks. It also has the overhead of managing two lists instead of one.

So, yes, it is *fair* in terms of NUMA node pressure. But being fair in a NUMA-aware allocator by simply not doing NUMA at all is a regression.

Quoted 3 lines

Implementation-wise you are asking me to squash 4/5 and 5/5 into a single patch, and remove global list. It's a tiny iteration from this patch version and I can do it.

Sounds good.