Message ID | 20240905080855.1699814-2-aaron.lu@intel.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | SGX NUMA fix | expand |
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
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); }