[PATCH v2 0/2] SGX NUMA fix

[PATCH v2 0/2] SGX NUMA fix

[Aaron Lu]

Hi,

A soft lockup issue was found during some SGX testing and it turned
out to be a problem in SGX code related to NUMA, please see patch1/2
for details.

While Dave reviewed v1 of patch1/2, he suggested dumping an info when
a node has both CPUs and memory but no EPC section, so that's patch2/2.

Comments are welcome, thanks.

v2:
- Enhance changelog for patch1/2 according to Kai, Dave and Jarkko's
  suggestions;
- Fix Reported-by tag, it should be Gerardo. Sorry for the mistake.
- Collect review tags.
- Add patch2/2.

Aaron Lu (2):
  x86/sgx: Fix deadlock in SGX NUMA node search
  x86/sgx: Log information when a node lacks an EPC section

 arch/x86/kernel/cpu/sgx/main.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

-- 
2.45.2

[PATCH v2 1/2] x86/sgx: Fix deadlock in SGX NUMA node search

[Aaron Lu]

When the current node doesn't have an EPC section configured by firmware
and all other EPC sections are used up, CPU can get stuck inside the
while loop that looks for an available EPC page from remote nodes
indefinitely, leading to a soft lockup. Note how nid_of_current will
never be equal to nid in that while loop because nid_of_current is not
set in sgx_numa_mask.

Also worth mentioning is that it's perfectly fine for the firmware not
to setup an EPC section on a node. While setting up an EPC section on
each node can enhance performance, it is not a requirement for
functionality.

Rework the loop to start and end on *a* node that has SGX memory. This
avoids the deadlock looking for the current SGX-lacking node to show up
in the loop when it never will.

Fixes: 901ddbb9ecf5 ("x86/sgx: Add a basic NUMA allocation scheme to sgx_alloc_epc_page()")
Reported-by: "Molina Sabido, Gerardo" <gerardo.molina.sabido@intel.com>
Tested-by: Zhimin Luo <zhimin.luo@intel.com>
Reviewed-by: Kai Huang <kai.huang@intel.com>
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 arch/x86/kernel/cpu/sgx/main.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 1a000acd933a..694fcf7a5e3a 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -475,24 +475,25 @@ struct sgx_epc_page *__sgx_alloc_epc_page(void)
 {
 	struct sgx_epc_page *page;
 	int nid_of_current = numa_node_id();
-	int nid = nid_of_current;
+	int nid_start, nid;
 
-	if (node_isset(nid_of_current, sgx_numa_mask)) {
-		page = __sgx_alloc_epc_page_from_node(nid_of_current);
-		if (page)
-			return page;
-	}
-
-	/* Fall back to the non-local NUMA nodes: */
-	while (true) {
-		nid = next_node_in(nid, sgx_numa_mask);
-		if (nid == nid_of_current)
-			break;
+	/*
+	 * Try local node first. If it doesn't have an EPC section,
+	 * fall back to the non-local NUMA nodes.
+	 */
+	if (node_isset(nid_of_current, sgx_numa_mask))
+		nid_start = nid_of_current;
+	else
+		nid_start = next_node_in(nid_of_current, sgx_numa_mask);
 
+	nid = nid_start;
+	do {
 		page = __sgx_alloc_epc_page_from_node(nid);
 		if (page)
 			return page;
-	}
+
+		nid = next_node_in(nid, sgx_numa_mask);
+	} while (nid != nid_start);
 
 	return ERR_PTR(-ENOMEM);
 }
-- 
2.45.2

Re: [PATCH v2 1/2] x86/sgx: Fix deadlock in SGX NUMA node search

[Jarkko Sakkinen]

On Thu Sep 5, 2024 at 11:08 AM EEST, Aaron Lu wrote:
> When the current node doesn't have an EPC section configured by firmware
> and all other EPC sections are used up, CPU can get stuck inside the
> while loop that looks for an available EPC page from remote nodes
> indefinitely, leading to a soft lockup. Note how nid_of_current will
> never be equal to nid in that while loop because nid_of_current is not
> set in sgx_numa_mask.
>
> Also worth mentioning is that it's perfectly fine for the firmware not
> to setup an EPC section on a node. While setting up an EPC section on
> each node can enhance performance, it is not a requirement for
> functionality.
>
> Rework the loop to start and end on *a* node that has SGX memory. This
> avoids the deadlock looking for the current SGX-lacking node to show up
> in the loop when it never will.
>
> Fixes: 901ddbb9ecf5 ("x86/sgx: Add a basic NUMA allocation scheme to sgx_alloc_epc_page()")
> Reported-by: "Molina Sabido, Gerardo" <gerardo.molina.sabido@intel.com>
> Tested-by: Zhimin Luo <zhimin.luo@intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 1a000acd933a..694fcf7a5e3a 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -475,24 +475,25 @@ struct sgx_epc_page *__sgx_alloc_epc_page(void)
>  {
>  	struct sgx_epc_page *page;
>  	int nid_of_current = numa_node_id();
> -	int nid = nid_of_current;
> +	int nid_start, nid;
>  
> -	if (node_isset(nid_of_current, sgx_numa_mask)) {
> -		page = __sgx_alloc_epc_page_from_node(nid_of_current);
> -		if (page)
> -			return page;
> -	}
> -
> -	/* Fall back to the non-local NUMA nodes: */
> -	while (true) {
> -		nid = next_node_in(nid, sgx_numa_mask);
> -		if (nid == nid_of_current)
> -			break;
> +	/*
> +	 * Try local node first. If it doesn't have an EPC section,
> +	 * fall back to the non-local NUMA nodes.
> +	 */
> +	if (node_isset(nid_of_current, sgx_numa_mask))
> +		nid_start = nid_of_current;
> +	else
> +		nid_start = next_node_in(nid_of_current, sgx_numa_mask);
>  
> +	nid = nid_start;
> +	do {
>  		page = __sgx_alloc_epc_page_from_node(nid);
>  		if (page)
>  			return page;
> -	}
> +
> +		nid = next_node_in(nid, sgx_numa_mask);
> +	} while (nid != nid_start);
>  
>  	return ERR_PTR(-ENOMEM);
>  }

Looks good to me:

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

[PATCH v2 2/2] x86/sgx: Log information when a node lacks an EPC section

[Aaron Lu]

For optimized performance, firmware typically distributes EPC sections
evenly across different NUMA nodes. However, there are scenarios where
a node may have both CPUs and memory but no EPC section configured. For
example, in an 8-socket system with a Sub-Numa-Cluster=2 setup, there
are a total of 16 nodes. Given that the maximum number of supported EPC
sections is 8, it is simply not feasible to assign one EPC section to
each node. This configuration is not incorrect - SGX will still operate
correctly; it is just not optimized from a NUMA standpoint.

For this reason, log a message when a node with both CPUs and memory
lacks an EPC section. This will provide users with a hint as to why they
might be experiencing less-than-ideal performance when running SGX
enclaves.

Suggested-by: Dave Hansen <dave.hansen@intel.com>
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 arch/x86/kernel/cpu/sgx/main.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 694fcf7a5e3a..3a79105455f1 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -848,6 +848,13 @@ static bool __init sgx_page_cache_init(void)
 		return false;
 	}
 
+	for_each_online_node(nid) {
+		if (!node_isset(nid, sgx_numa_mask) &&
+		    node_state(nid, N_MEMORY) && node_state(nid, N_CPU))
+			pr_info("node%d has both CPUs and memory but doesn't have an EPC section\n",
+				nid);
+	}
+
 	return true;
 }
 
-- 
2.45.2

Re: [PATCH v2 2/2] x86/sgx: Log information when a node lacks an EPC section

[Jarkko Sakkinen]

On Thu Sep 5, 2024 at 11:08 AM EEST, Aaron Lu wrote:
> For optimized performance, firmware typically distributes EPC sections
> evenly across different NUMA nodes. However, there are scenarios where
> a node may have both CPUs and memory but no EPC section configured. For
> example, in an 8-socket system with a Sub-Numa-Cluster=2 setup, there
> are a total of 16 nodes. Given that the maximum number of supported EPC
> sections is 8, it is simply not feasible to assign one EPC section to
> each node. This configuration is not incorrect - SGX will still operate
> correctly; it is just not optimized from a NUMA standpoint.
>
> For this reason, log a message when a node with both CPUs and memory
> lacks an EPC section. This will provide users with a hint as to why they
> might be experiencing less-than-ideal performance when running SGX
> enclaves.
>
> Suggested-by: Dave Hansen <dave.hansen@intel.com>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 694fcf7a5e3a..3a79105455f1 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -848,6 +848,13 @@ static bool __init sgx_page_cache_init(void)
>  		return false;
>  	}
>  
> +	for_each_online_node(nid) {
> +		if (!node_isset(nid, sgx_numa_mask) &&
> +		    node_state(nid, N_MEMORY) && node_state(nid, N_CPU))
> +			pr_info("node%d has both CPUs and memory but doesn't have an EPC section\n",
> +				nid);

Is this enough, or is there anything that would need to be done
automatically if this happens? With a tracepoint you could react to such
even but I'm totally fine with this.

> +	}
> +
>  	return true;
>  }
>  

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

Re: [PATCH v2 2/2] x86/sgx: Log information when a node lacks an EPC section

[Dave Hansen]

On 9/5/24 07:24, Jarkko Sakkinen wrote:
>> +	for_each_online_node(nid) {
>> +		if (!node_isset(nid, sgx_numa_mask) &&
>> +		    node_state(nid, N_MEMORY) && node_state(nid, N_CPU))
>> +			pr_info("node%d has both CPUs and memory but doesn't have an EPC section\n",
>> +				nid);
> Is this enough, or is there anything that would need to be done
> automatically if this happens? With a tracepoint you could react to such
> even but I'm totally fine with this.

There's always the theoretical chance that there are nodes being
hotplugged or that this is all running in a VM that has some whacky
topology.

But this simple pr_info() provides good coverage for the common cases
and shouldn't be too much of a burden for the weridos.

Re: [PATCH v2 2/2] x86/sgx: Log information when a node lacks an EPC section

[Huang, Kai]

On 5/09/2024 8:08 pm, Aaron Lu wrote:
> For optimized performance, firmware typically distributes EPC sections
> evenly across different NUMA nodes. However, there are scenarios where
> a node may have both CPUs and memory but no EPC section configured. For
> example, in an 8-socket system with a Sub-Numa-Cluster=2 setup, there
> are a total of 16 nodes. Given that the maximum number of supported EPC
> sections is 8, it is simply not feasible to assign one EPC section to
> each node. This configuration is not incorrect - SGX will still operate
> correctly; it is just not optimized from a NUMA standpoint.
> 
> For this reason, log a message when a node with both CPUs and memory
> lacks an EPC section. This will provide users with a hint as to why they
> might be experiencing less-than-ideal performance when running SGX
> enclaves.
> 
> Suggested-by: Dave Hansen <dave.hansen@intel.com>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>

Acked-by: Kai Huang <kai.huang@intel.com>