Message ID | 20200910175637.2865160-1-cheloha@linux.ibm.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v1] pseries/hotplug-memory: hot-add: skip redundant LMB lookup | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (4b552a4cbf286ff9dcdab19153f3c1c7d1680fab) |
snowpatch_ozlabs/build-ppc64le | warning | Upstream build failed, couldn't test patch |
snowpatch_ozlabs/build-ppc64be | warning | Upstream build failed, couldn't test patch |
snowpatch_ozlabs/build-ppc64e | warning | Upstream build failed, couldn't test patch |
snowpatch_ozlabs/build-pmac32 | warning | Upstream build failed, couldn't test patch |
snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 1 warnings, 0 checks, 28 lines checked |
snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Hi Scott, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on powerpc/next] [also build test WARNING on next-20200910] [cannot apply to v5.9-rc4] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Scott-Cheloha/pseries-hotplug-memory-hot-add-skip-redundant-LMB-lookup/20200911-015744 base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next config: powerpc-randconfig-r034-20200911 (attached as .config) compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 0448d11a06b451a63a8f60408fec613ad24801ba) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install powerpc cross compiling tool for clang build # apt-get install binutils-powerpc-linux-gnu # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> arch/powerpc/mm/numa.c:433:5: warning: no previous prototype for function 'of_drconf_to_nid_single' [-Wmissing-prototypes] int of_drconf_to_nid_single(struct drmem_lmb *lmb) ^ arch/powerpc/mm/numa.c:433:1: note: declare 'static' if the function is not intended to be used outside of this translation unit int of_drconf_to_nid_single(struct drmem_lmb *lmb) ^ static 1 warning generated. # https://github.com/0day-ci/linux/commit/e6847f3f58460841a28885578cc3547735cfa50c git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Scott-Cheloha/pseries-hotplug-memory-hot-add-skip-redundant-LMB-lookup/20200911-015744 git checkout e6847f3f58460841a28885578cc3547735cfa50c vim +/of_drconf_to_nid_single +433 arch/powerpc/mm/numa.c 428 429 /* 430 * This is like of_node_to_nid_single() for memory represented in the 431 * ibm,dynamic-reconfiguration-memory node. 432 */ > 433 int of_drconf_to_nid_single(struct drmem_lmb *lmb) 434 { 435 struct assoc_arrays aa = { .arrays = NULL }; 436 int default_nid = NUMA_NO_NODE; 437 int nid = default_nid; 438 int rc, index; 439 440 if ((min_common_depth < 0) || !numa_enabled) 441 return default_nid; 442 443 rc = of_get_assoc_arrays(&aa); 444 if (rc) 445 return default_nid; 446 447 if (min_common_depth <= aa.array_sz && 448 !(lmb->flags & DRCONF_MEM_AI_INVALID) && lmb->aa_index < aa.n_arrays) { 449 index = lmb->aa_index * aa.array_sz + min_common_depth - 1; 450 nid = of_read_number(&aa.arrays[index], 1); 451 452 if (nid == 0xffff || nid >= nr_node_ids) 453 nid = default_nid; 454 455 if (nid > 0) { 456 index = lmb->aa_index * aa.array_sz; 457 initialize_distance_lookup_table(nid, 458 &aa.arrays[index]); 459 } 460 } 461 462 return nid; 463 } 464 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Scott, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on powerpc/next] [also build test WARNING on next-20200910] [cannot apply to v5.9-rc4] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Scott-Cheloha/pseries-hotplug-memory-hot-add-skip-redundant-LMB-lookup/20200911-015744 base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next config: powerpc-allyesconfig (attached as .config) compiler: powerpc64-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> arch/powerpc/mm/numa.c:433:5: warning: no previous prototype for 'of_drconf_to_nid_single' [-Wmissing-prototypes] 433 | int of_drconf_to_nid_single(struct drmem_lmb *lmb) | ^~~~~~~~~~~~~~~~~~~~~~~ # https://github.com/0day-ci/linux/commit/e6847f3f58460841a28885578cc3547735cfa50c git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Scott-Cheloha/pseries-hotplug-memory-hot-add-skip-redundant-LMB-lookup/20200911-015744 git checkout e6847f3f58460841a28885578cc3547735cfa50c vim +/of_drconf_to_nid_single +433 arch/powerpc/mm/numa.c 428 429 /* 430 * This is like of_node_to_nid_single() for memory represented in the 431 * ibm,dynamic-reconfiguration-memory node. 432 */ > 433 int of_drconf_to_nid_single(struct drmem_lmb *lmb) 434 { 435 struct assoc_arrays aa = { .arrays = NULL }; 436 int default_nid = NUMA_NO_NODE; 437 int nid = default_nid; 438 int rc, index; 439 440 if ((min_common_depth < 0) || !numa_enabled) 441 return default_nid; 442 443 rc = of_get_assoc_arrays(&aa); 444 if (rc) 445 return default_nid; 446 447 if (min_common_depth <= aa.array_sz && 448 !(lmb->flags & DRCONF_MEM_AI_INVALID) && lmb->aa_index < aa.n_arrays) { 449 index = lmb->aa_index * aa.array_sz + min_common_depth - 1; 450 nid = of_read_number(&aa.arrays[index], 1); 451 452 if (nid == 0xffff || nid >= nr_node_ids) 453 nid = default_nid; 454 455 if (nid > 0) { 456 index = lmb->aa_index * aa.array_sz; 457 initialize_distance_lookup_table(nid, 458 &aa.arrays[index]); 459 } 460 } 461 462 return nid; 463 } 464 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Scott, Scott Cheloha <cheloha@linux.ibm.com> writes: > During memory hot-add, dlpar_add_lmb() calls memory_add_physaddr_to_nid() > to determine which node id (nid) to use when later calling __add_memory(). > ... > > Consider an LPAR with 126976 LMBs. In one test, hot-adding 126000 Nice little machine you got there :P > LMBs on an upatched kernel took ~3.5 hours while a patched kernel > completed the same operation in ~2 hours: ... > diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c > index 0ea976d1cac4..9cd572440175 100644 > --- a/arch/powerpc/platforms/pseries/hotplug-memory.c > +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c > @@ -595,6 +595,8 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index) > } > #endif /* CONFIG_MEMORY_HOTREMOVE */ > > +extern int of_drconf_to_nid_single(struct drmem_lmb *); > + This needs to go in a header. It should probably go in arch/powerpc/include/asm/topology.h cheers > static int dlpar_add_lmb(struct drmem_lmb *lmb) > { > unsigned long block_sz; > @@ -611,8 +613,10 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb) > > block_sz = memory_block_size_bytes(); > > - /* Find the node id for this address. */ > - nid = memory_add_physaddr_to_nid(lmb->base_addr); > + /* Find the node id for this address. Fake one if necessary. */ > + nid = of_drconf_to_nid_single(lmb); > + if (nid < 0 || !node_possible(nid)) > + nid = first_online_node; > > /* Add the memory */ > rc = __add_memory(nid, lmb->base_addr, block_sz); > -- > 2.24.1
diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c index 1f61fa2148b5..63507b47164d 100644 --- a/arch/powerpc/mm/numa.c +++ b/arch/powerpc/mm/numa.c @@ -430,7 +430,7 @@ static int of_get_assoc_arrays(struct assoc_arrays *aa) * This is like of_node_to_nid_single() for memory represented in the * ibm,dynamic-reconfiguration-memory node. */ -static int of_drconf_to_nid_single(struct drmem_lmb *lmb) +int of_drconf_to_nid_single(struct drmem_lmb *lmb) { struct assoc_arrays aa = { .arrays = NULL }; int default_nid = NUMA_NO_NODE; diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c index 0ea976d1cac4..9cd572440175 100644 --- a/arch/powerpc/platforms/pseries/hotplug-memory.c +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c @@ -595,6 +595,8 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index) } #endif /* CONFIG_MEMORY_HOTREMOVE */ +extern int of_drconf_to_nid_single(struct drmem_lmb *); + static int dlpar_add_lmb(struct drmem_lmb *lmb) { unsigned long block_sz; @@ -611,8 +613,10 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb) block_sz = memory_block_size_bytes(); - /* Find the node id for this address. */ - nid = memory_add_physaddr_to_nid(lmb->base_addr); + /* Find the node id for this address. Fake one if necessary. */ + nid = of_drconf_to_nid_single(lmb); + if (nid < 0 || !node_possible(nid)) + nid = first_online_node; /* Add the memory */ rc = __add_memory(nid, lmb->base_addr, block_sz);
During memory hot-add, dlpar_add_lmb() calls memory_add_physaddr_to_nid() to determine which node id (nid) to use when later calling __add_memory(). This is wasteful. On pseries, memory_add_physaddr_to_nid() finds an appropriate nid for a given address by looking up the LMB containing the address and then passing that LMB to of_drconf_to_nid_single() to get the nid. In dlpar_add_lmb() we get this address from the LMB itself. In short, we have a pointer to an LMB and then we are searching for that LMB *again* in order to find its nid. If we call of_drconf_to_nid_single() directly from dlpar_add_lmb() we can skip the redundant lookup. The only error handling we need to duplicate from memory_add_physaddr_to_nid() is the fallback to the default nid when drconf_to_nid_single() returns -1 (NUMA_NO_NODE) or an invalid nid. Skipping the extra lookup makes hot-add operations faster, especially on machines with many LMBs. Consider an LPAR with 126976 LMBs. In one test, hot-adding 126000 LMBs on an upatched kernel took ~3.5 hours while a patched kernel completed the same operation in ~2 hours: Unpatched (12450 seconds): Sep 9 04:06:31 ltc-brazos1 drmgr[810169]: drmgr: -c mem -a -q 126000 Sep 9 04:06:31 ltc-brazos1 kernel: pseries-hotplug-mem: Attempting to hot-add 126000 LMB(s) [...] Sep 9 07:34:01 ltc-brazos1 kernel: pseries-hotplug-mem: Memory at 20000000 (drc index 80000002) was hot-added Patched (7065 seconds): Sep 8 21:49:57 ltc-brazos1 drmgr[877703]: drmgr: -c mem -a -q 126000 Sep 8 21:49:57 ltc-brazos1 kernel: pseries-hotplug-mem: Attempting to hot-add 126000 LMB(s) [...] Sep 8 23:27:42 ltc-brazos1 kernel: pseries-hotplug-mem: Memory at 20000000 (drc index 80000002) was hot-added It should be noted that the speedup grows more substantial when hot-adding LMBs at the end of the drconf range. This is because we are skipping a linear LMB search. To see the distinction, consider smaller hot-add test on the same LPAR. A perf-stat run with 10 iterations showed that hot-adding 4096 LMBs completed less than 1 second faster on a patched kernel: Unpatched: Performance counter stats for 'drmgr -c mem -a -q 4096' (10 runs): 104,753.42 msec task-clock # 0.992 CPUs utilized ( +- 0.55% ) 4,708 context-switches # 0.045 K/sec ( +- 0.69% ) 2,444 cpu-migrations # 0.023 K/sec ( +- 1.25% ) 394 page-faults # 0.004 K/sec ( +- 0.22% ) 445,902,503,057 cycles # 4.257 GHz ( +- 0.55% ) (66.67%) 8,558,376,740 stalled-cycles-frontend # 1.92% frontend cycles idle ( +- 0.88% ) (49.99%) 300,346,181,651 stalled-cycles-backend # 67.36% backend cycles idle ( +- 0.76% ) (50.01%) 258,091,488,691 instructions # 0.58 insn per cycle # 1.16 stalled cycles per insn ( +- 0.22% ) (66.67%) 70,568,169,256 branches # 673.660 M/sec ( +- 0.17% ) (50.01%) 3,100,725,426 branch-misses # 4.39% of all branches ( +- 0.20% ) (49.99%) 105.583 +- 0.589 seconds time elapsed ( +- 0.56% ) Patched: Performance counter stats for 'drmgr -c mem -a -q 4096' (10 runs): 104,055.69 msec task-clock # 0.993 CPUs utilized ( +- 0.32% ) 4,606 context-switches # 0.044 K/sec ( +- 0.20% ) 2,463 cpu-migrations # 0.024 K/sec ( +- 0.93% ) 394 page-faults # 0.004 K/sec ( +- 0.25% ) 442,951,129,921 cycles # 4.257 GHz ( +- 0.32% ) (66.66%) 8,710,413,329 stalled-cycles-frontend # 1.97% frontend cycles idle ( +- 0.47% ) (50.06%) 299,656,905,836 stalled-cycles-backend # 67.65% backend cycles idle ( +- 0.39% ) (50.02%) 252,731,168,193 instructions # 0.57 insn per cycle # 1.19 stalled cycles per insn ( +- 0.20% ) (66.66%) 68,902,851,121 branches # 662.173 M/sec ( +- 0.13% ) (49.94%) 3,100,242,882 branch-misses # 4.50% of all branches ( +- 0.15% ) (49.98%) 104.829 +- 0.325 seconds time elapsed ( +- 0.31% ) This is consistent. An add-by-count hot-add operation adds LMBs greedily, so LMBs near the start of the drconf range are considered first. On an otherwise idle LPAR with so many LMBs we would expect to find the LMBs we need near the start of the drconf range, hence the smaller speedup. Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com> --- arch/powerpc/mm/numa.c | 2 +- arch/powerpc/platforms/pseries/hotplug-memory.c | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-)