| Message ID | 00e3eea06f5dde61734a53af797b190692060aab.camel@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: iaa - Account for cpu-less numa nodes | expand |
Hi-- On 12/26/23 12:53, Tom Zanussi wrote: > In some configurations e.g. systems with CXL, a numa node can have 0 > cpus and cpumask_nth() will return a cpu value that doesn't exist, > which will result in an attempt to add an entry to the wq table at a > bad index. > > To fix this, when iterating the cpus for a node, skip any node that > doesn't have cpus. > > Also, as a precaution, add a warning and bail if cpumask_nth() returns > a nonexistent cpu. > > Reported-by: Zhang, Rex <rex.zhang@intel.com> > Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com> > --- > drivers/crypto/intel/iaa/iaa_crypto_main.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/crypto/intel/iaa/iaa_crypto_main.c b/drivers/crypto/intel/iaa/iaa_crypto_main.c > index 5093361b0107..782157a74043 100644 > --- a/drivers/crypto/intel/iaa/iaa_crypto_main.c > +++ b/drivers/crypto/intel/iaa/iaa_crypto_main.c > @@ -1017,12 +1017,17 @@ static void rebalance_wq_table(void) > return; > } > > - for_each_online_node(node) { > + for_each_node_with_cpus(node) { > node_cpus = cpumask_of_node(node); > > for (cpu = 0; cpu < nr_cpus_per_node; cpu++) { > int node_cpu = cpumask_nth(cpu, node_cpus); > > + if (WARN_ON(node_cpu >= nr_cpu_ids)) { > + pr_debug("node_cpu %d doesn't exist!\n", node_cpu); > + return; > + } > + > if ((cpu % cpus_per_iaa) == 0) > iaa++; > > @@ -2095,10 +2100,13 @@ static struct idxd_device_driver iaa_crypto_driver = { > static int __init iaa_crypto_init_module(void) > { > int ret = 0; > + int node; > > nr_cpus = num_online_cpus(); > - nr_nodes = num_online_nodes(); > - nr_cpus_per_node = nr_cpus / nr_nodes; > + for_each_node_with_cpus(node) > + nr_nodes++; > + if (nr_nodes) > + nr_cpus_per_node = nr_cpus / nr_nodes; If nr_nodes == 0, nr_cpus_per_node is not initialized here. Is it initialized somewhere else, or just not used if nr_nodes is 0? > > if (crypto_has_comp("deflate-generic", 0, 0)) > deflate_generic_tfm = crypto_alloc_comp("deflate-generic", 0, 0);
Hi Randy, On Tue, 2023-12-26 at 13:09 -0800, Randy Dunlap wrote: > Hi-- > > On 12/26/23 12:53, Tom Zanussi wrote: > > In some configurations e.g. systems with CXL, a numa node can have > > 0 > > cpus and cpumask_nth() will return a cpu value that doesn't exist, > > which will result in an attempt to add an entry to the wq table at > > a > > bad index. > > > > To fix this, when iterating the cpus for a node, skip any node that > > doesn't have cpus. > > > > Also, as a precaution, add a warning and bail if cpumask_nth() > > returns > > a nonexistent cpu. > > > > Reported-by: Zhang, Rex <rex.zhang@intel.com> > > Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com> > > --- > > drivers/crypto/intel/iaa/iaa_crypto_main.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/crypto/intel/iaa/iaa_crypto_main.c > > b/drivers/crypto/intel/iaa/iaa_crypto_main.c > > index 5093361b0107..782157a74043 100644 > > --- a/drivers/crypto/intel/iaa/iaa_crypto_main.c > > +++ b/drivers/crypto/intel/iaa/iaa_crypto_main.c > > @@ -1017,12 +1017,17 @@ static void rebalance_wq_table(void) > > return; > > } > > > > - for_each_online_node(node) { > > + for_each_node_with_cpus(node) { > > node_cpus = cpumask_of_node(node); > > > > for (cpu = 0; cpu < nr_cpus_per_node; cpu++) { > > int node_cpu = cpumask_nth(cpu, node_cpus); > > > > + if (WARN_ON(node_cpu >= nr_cpu_ids)) { > > + pr_debug("node_cpu %d doesn't > > exist!\n", node_cpu); > > + return; > > + } > > + > > if ((cpu % cpus_per_iaa) == 0) > > iaa++; > > > > @@ -2095,10 +2100,13 @@ static struct idxd_device_driver > > iaa_crypto_driver = { > > static int __init iaa_crypto_init_module(void) > > { > > int ret = 0; > > + int node; > > > > nr_cpus = num_online_cpus(); > > - nr_nodes = num_online_nodes(); > > - nr_cpus_per_node = nr_cpus / nr_nodes; > > + for_each_node_with_cpus(node) > > + nr_nodes++; > > + if (nr_nodes) > > + nr_cpus_per_node = nr_cpus / nr_nodes; > > If nr_nodes == 0, nr_cpus_per_node is not initialized here. > Is it initialized somewhere else, or just not used if nr_nodes is 0? > nr_cpus_per_node is initialized to 0 elsewhere (as a static global). It seems to me nr_nodes should always be at least 1. From my testing with !CONFIG_NUMA, nr_nodes is set to 1 in that case; not sure how you can get actually get nr_nodes == 0 if you have any cpus working. The check is there to avoid dividing by 0 but maybe the right thing to is BUG_ON(!nr_nodes) and return an error, and remove that check... Thanks, Tom > > > > if (crypto_has_comp("deflate-generic", 0, 0)) > > deflate_generic_tfm = crypto_alloc_comp("deflate- > > generic", 0, 0); >
On 12/26/23 14:04, Tom Zanussi wrote: > Hi Randy, > > On Tue, 2023-12-26 at 13:09 -0800, Randy Dunlap wrote: >> Hi-- >> >> On 12/26/23 12:53, Tom Zanussi wrote: >>> In some configurations e.g. systems with CXL, a numa node can have >>> 0 >>> cpus and cpumask_nth() will return a cpu value that doesn't exist, >>> which will result in an attempt to add an entry to the wq table at >>> a >>> bad index. >>> >>> To fix this, when iterating the cpus for a node, skip any node that >>> doesn't have cpus. >>> >>> Also, as a precaution, add a warning and bail if cpumask_nth() >>> returns >>> a nonexistent cpu. >>> >>> Reported-by: Zhang, Rex <rex.zhang@intel.com> >>> Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com> >>> --- >>> drivers/crypto/intel/iaa/iaa_crypto_main.c | 14 +++++++++++--- >>> 1 file changed, 11 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/crypto/intel/iaa/iaa_crypto_main.c >>> b/drivers/crypto/intel/iaa/iaa_crypto_main.c >>> index 5093361b0107..782157a74043 100644 >>> --- a/drivers/crypto/intel/iaa/iaa_crypto_main.c >>> +++ b/drivers/crypto/intel/iaa/iaa_crypto_main.c >>> @@ -1017,12 +1017,17 @@ static void rebalance_wq_table(void) >>> return; >>> } >>> >>> - for_each_online_node(node) { >>> + for_each_node_with_cpus(node) { >>> node_cpus = cpumask_of_node(node); >>> >>> for (cpu = 0; cpu < nr_cpus_per_node; cpu++) { >>> int node_cpu = cpumask_nth(cpu, node_cpus); >>> >>> + if (WARN_ON(node_cpu >= nr_cpu_ids)) { >>> + pr_debug("node_cpu %d doesn't >>> exist!\n", node_cpu); >>> + return; >>> + } >>> + >>> if ((cpu % cpus_per_iaa) == 0) >>> iaa++; >>> >>> @@ -2095,10 +2100,13 @@ static struct idxd_device_driver >>> iaa_crypto_driver = { >>> static int __init iaa_crypto_init_module(void) >>> { >>> int ret = 0; >>> + int node; >>> >>> nr_cpus = num_online_cpus(); >>> - nr_nodes = num_online_nodes(); >>> - nr_cpus_per_node = nr_cpus / nr_nodes; >>> + for_each_node_with_cpus(node) >>> + nr_nodes++; >>> + if (nr_nodes) >>> + nr_cpus_per_node = nr_cpus / nr_nodes; >> >> If nr_nodes == 0, nr_cpus_per_node is not initialized here. >> Is it initialized somewhere else, or just not used if nr_nodes is 0? >> > > nr_cpus_per_node is initialized to 0 elsewhere (as a static global). > > It seems to me nr_nodes should always be at least 1. From my testing > with !CONFIG_NUMA, nr_nodes is set to 1 in that case; not sure how you > can get actually get nr_nodes == 0 if you have any cpus working. The > check is there to avoid dividing by 0 but maybe the right thing to is > BUG_ON(!nr_nodes) and return an error, and remove that check... I think it's OK as is then. and I hope that we never see the WARN_ON() up above. :) >>> >>> if (crypto_has_comp("deflate-generic", 0, 0)) >>> deflate_generic_tfm = crypto_alloc_comp("deflate- >>> generic", 0, 0); >> > Thanks.
On Tue, Dec 26, 2023 at 02:53:26PM -0600, Tom Zanussi wrote: > In some configurations e.g. systems with CXL, a numa node can have 0 > cpus and cpumask_nth() will return a cpu value that doesn't exist, > which will result in an attempt to add an entry to the wq table at a > bad index. > > To fix this, when iterating the cpus for a node, skip any node that > doesn't have cpus. > > Also, as a precaution, add a warning and bail if cpumask_nth() returns > a nonexistent cpu. > > Reported-by: Zhang, Rex <rex.zhang@intel.com> > Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com> > --- > drivers/crypto/intel/iaa/iaa_crypto_main.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) Patch applied. Thanks.
diff --git a/drivers/crypto/intel/iaa/iaa_crypto_main.c b/drivers/crypto/intel/iaa/iaa_crypto_main.c index 5093361b0107..782157a74043 100644 --- a/drivers/crypto/intel/iaa/iaa_crypto_main.c +++ b/drivers/crypto/intel/iaa/iaa_crypto_main.c @@ -1017,12 +1017,17 @@ static void rebalance_wq_table(void) return; } - for_each_online_node(node) { + for_each_node_with_cpus(node) { node_cpus = cpumask_of_node(node); for (cpu = 0; cpu < nr_cpus_per_node; cpu++) { int node_cpu = cpumask_nth(cpu, node_cpus); + if (WARN_ON(node_cpu >= nr_cpu_ids)) { + pr_debug("node_cpu %d doesn't exist!\n", node_cpu); + return; + } + if ((cpu % cpus_per_iaa) == 0) iaa++; @@ -2095,10 +2100,13 @@ static struct idxd_device_driver iaa_crypto_driver = { static int __init iaa_crypto_init_module(void) { int ret = 0; + int node; nr_cpus = num_online_cpus(); - nr_nodes = num_online_nodes(); - nr_cpus_per_node = nr_cpus / nr_nodes; + for_each_node_with_cpus(node) + nr_nodes++; + if (nr_nodes) + nr_cpus_per_node = nr_cpus / nr_nodes; if (crypto_has_comp("deflate-generic", 0, 0)) deflate_generic_tfm = crypto_alloc_comp("deflate-generic", 0, 0);
In some configurations e.g. systems with CXL, a numa node can have 0 cpus and cpumask_nth() will return a cpu value that doesn't exist, which will result in an attempt to add an entry to the wq table at a bad index. To fix this, when iterating the cpus for a node, skip any node that doesn't have cpus. Also, as a precaution, add a warning and bail if cpumask_nth() returns a nonexistent cpu. Reported-by: Zhang, Rex <rex.zhang@intel.com> Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com> --- drivers/crypto/intel/iaa/iaa_crypto_main.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)