| Message ID | e39502af91e12ba1a4bef3be4d05b11b2c7a7a9f.1636616548.git.baolin.wang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support multiple target nodes demotion | expand |
Baolin Wang <baolin.wang@linux.alibaba.com> writes: > For the worst case (MAX_NUMNODES=1024), the node_demotion structure can > consume 32k bytes, which appears too large, so we can change to allocate > node_demotion dynamically at initialization time. Meanwhile allocating > the target demotion nodes array dynamically to select a suitable size > according to the MAX_NUMNODES. > > Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> > --- > mm/migrate.c | 38 +++++++++++++++++++++++++++++--------- > 1 file changed, 29 insertions(+), 9 deletions(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index 126e9e6..0145b38 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1152,10 +1152,11 @@ static int __unmap_and_move(struct page *page, struct page *newpage, > #define DEFAULT_DEMOTION_TARGET_NODES 15 > struct demotion_nodes { > unsigned short nr; > - short nodes[DEFAULT_DEMOTION_TARGET_NODES]; > + short nodes[]; > }; > > -static struct demotion_nodes node_demotion[MAX_NUMNODES] __read_mostly; > +static struct demotion_nodes *node_demotion[MAX_NUMNODES] __read_mostly; > +static unsigned short target_nodes_max; I think we can use something as below, #if MAX_NUMNODES < DEFAULT_DEMOTION_TARGET_NODES #define DEMOTION_TARGET_NODES (MAX_NUMNODES - 1) #else #define DEMOTION_TARGET_NODES DEFAULT_DEMOTION_TARGET_NODES #endif static struct demotion_nodes *node_demotion; Then we can allocate nr_node_ids * sizeof(struct demotion_nodes) for node_demotion. Best Regards, Huang, Ying [snip]
On 2021/11/11 16:51, Huang, Ying wrote: > Baolin Wang <baolin.wang@linux.alibaba.com> writes: > >> For the worst case (MAX_NUMNODES=1024), the node_demotion structure can >> consume 32k bytes, which appears too large, so we can change to allocate >> node_demotion dynamically at initialization time. Meanwhile allocating >> the target demotion nodes array dynamically to select a suitable size >> according to the MAX_NUMNODES. >> >> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> >> --- >> mm/migrate.c | 38 +++++++++++++++++++++++++++++--------- >> 1 file changed, 29 insertions(+), 9 deletions(-) >> >> diff --git a/mm/migrate.c b/mm/migrate.c >> index 126e9e6..0145b38 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -1152,10 +1152,11 @@ static int __unmap_and_move(struct page *page, struct page *newpage, >> #define DEFAULT_DEMOTION_TARGET_NODES 15 >> struct demotion_nodes { >> unsigned short nr; >> - short nodes[DEFAULT_DEMOTION_TARGET_NODES]; >> + short nodes[]; >> }; >> >> -static struct demotion_nodes node_demotion[MAX_NUMNODES] __read_mostly; >> +static struct demotion_nodes *node_demotion[MAX_NUMNODES] __read_mostly; >> +static unsigned short target_nodes_max; > > I think we can use something as below, > > #if MAX_NUMNODES < DEFAULT_DEMOTION_TARGET_NODES > #define DEMOTION_TARGET_NODES (MAX_NUMNODES - 1) > #else > #define DEMOTION_TARGET_NODES DEFAULT_DEMOTION_TARGET_NODES > #endif Yes, looks better. > > static struct demotion_nodes *node_demotion; > > Then we can allocate nr_node_ids * sizeof(struct demotion_nodes) for node_demotion. Yeah, this is simple. The reason I want to declare the structure like "struct demotion_nodes *node_demotion[MAX_NUMNODES]" is that, we can validate the non-possible nodes which are invalid to demote memory, and in case the node_demotion[nid] is failed to be allocated which can be validated, though this is unlikely. However, I agree with you to keep things simple now and can be merged into patch 1. Will do in next version. Thanks.
Hi Baolin,
I love your patch! Yet something to improve:
[auto build test ERROR on hnaz-mm/master]
[also build test ERROR on v5.15 next-20211111]
[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/Baolin-Wang/Support-multiple-target-nodes-demotion/20211111-154935
base: https://github.com/hnaz/linux-mm master
config: m68k-allyesconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 11.2.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
# https://github.com/0day-ci/linux/commit/fda28dda2298e1ef984376940071c8435e9b1399
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Baolin-Wang/Support-multiple-target-nodes-demotion/20211111-154935
git checkout fda28dda2298e1ef984376940071c8435e9b1399
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> mm/migrate.c:1159:23: error: 'target_nodes_max' defined but not used [-Werror=unused-variable]
1159 | static unsigned short target_nodes_max;
| ^~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
vim +/target_nodes_max +1159 mm/migrate.c
1157
1158 static struct demotion_nodes *node_demotion[MAX_NUMNODES] __read_mostly;
> 1159 static unsigned short target_nodes_max;
1160
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Baolin Wang <baolin.wang@linux.alibaba.com> writes: > On 2021/11/11 16:51, Huang, Ying wrote: >> Baolin Wang <baolin.wang@linux.alibaba.com> writes: >> >>> For the worst case (MAX_NUMNODES=1024), the node_demotion structure can >>> consume 32k bytes, which appears too large, so we can change to allocate >>> node_demotion dynamically at initialization time. Meanwhile allocating >>> the target demotion nodes array dynamically to select a suitable size >>> according to the MAX_NUMNODES. >>> >>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> >>> --- >>> mm/migrate.c | 38 +++++++++++++++++++++++++++++--------- >>> 1 file changed, 29 insertions(+), 9 deletions(-) >>> >>> diff --git a/mm/migrate.c b/mm/migrate.c >>> index 126e9e6..0145b38 100644 >>> --- a/mm/migrate.c >>> +++ b/mm/migrate.c >>> @@ -1152,10 +1152,11 @@ static int __unmap_and_move(struct page *page, struct page *newpage, >>> #define DEFAULT_DEMOTION_TARGET_NODES 15 >>> struct demotion_nodes { >>> unsigned short nr; >>> - short nodes[DEFAULT_DEMOTION_TARGET_NODES]; >>> + short nodes[]; >>> }; >>> -static struct demotion_nodes node_demotion[MAX_NUMNODES] >>> __read_mostly; >>> +static struct demotion_nodes *node_demotion[MAX_NUMNODES] __read_mostly; >>> +static unsigned short target_nodes_max; >> I think we can use something as below, >> #if MAX_NUMNODES < DEFAULT_DEMOTION_TARGET_NODES >> #define DEMOTION_TARGET_NODES (MAX_NUMNODES - 1) >> #else >> #define DEMOTION_TARGET_NODES DEFAULT_DEMOTION_TARGET_NODES >> #endif > > Yes, looks better. > >> static struct demotion_nodes *node_demotion; >> Then we can allocate nr_node_ids * sizeof(struct demotion_nodes) for >> node_demotion. > > Yeah, this is simple. The reason I want to declare the structure like > "struct demotion_nodes *node_demotion[MAX_NUMNODES]" is that, we can > validate the non-possible nodes which are invalid to demote memory, > and in case the node_demotion[nid] is failed to be allocated which can > be validated, though this is unlikely. In case allocation failure, we can still check "node_demotion == NULL". Best Regards, Huang, Ying > However, I agree with you to > keep things simple now and can be merged into patch 1. Will do in next > version. Thanks.
diff --git a/mm/migrate.c b/mm/migrate.c index 126e9e6..0145b38 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1152,10 +1152,11 @@ static int __unmap_and_move(struct page *page, struct page *newpage, #define DEFAULT_DEMOTION_TARGET_NODES 15 struct demotion_nodes { unsigned short nr; - short nodes[DEFAULT_DEMOTION_TARGET_NODES]; + short nodes[]; }; -static struct demotion_nodes node_demotion[MAX_NUMNODES] __read_mostly; +static struct demotion_nodes *node_demotion[MAX_NUMNODES] __read_mostly; +static unsigned short target_nodes_max; /** * next_demotion_node() - Get the next node in the demotion path @@ -1168,10 +1169,13 @@ struct demotion_nodes { */ int next_demotion_node(int node) { - struct demotion_nodes *nd = &node_demotion[node]; + struct demotion_nodes *nd = node_demotion[node]; unsigned short target_nr, index; int target; + if (!nd) + return NUMA_NO_NODE; + /* * node_demotion[] is updated without excluding this * function from running. RCU doesn't provide any @@ -3014,9 +3018,9 @@ static void __disable_all_migrate_targets(void) int node, i; for_each_online_node(node) { - node_demotion[node].nr = 0; - for (i = 0; i < DEFAULT_DEMOTION_TARGET_NODES; i++) - node_demotion[node].nodes[i] = NUMA_NO_NODE; + node_demotion[node]->nr = 0; + for (i = 0; i < target_nodes_max; i++) + node_demotion[node]->nodes[i] = NUMA_NO_NODE; } } @@ -3048,7 +3052,10 @@ static int establish_migrate_target(int node, nodemask_t *used, int best_distance) { int migration_target, index, val; - struct demotion_nodes *nd = &node_demotion[node]; + struct demotion_nodes *nd = node_demotion[node]; + + if (WARN_ONCE(!nd, "Can not set up migration path for node:%d\n", node)) + return NUMA_NO_NODE; migration_target = find_next_best_node(node, used); if (migration_target == NUMA_NO_NODE) @@ -3067,7 +3074,7 @@ static int establish_migrate_target(int node, nodemask_t *used, } index = nd->nr; - if (WARN_ONCE(index >= DEFAULT_DEMOTION_TARGET_NODES, + if (WARN_ONCE(index >= target_nodes_max, "Exceeds maximum demotion target nodes\n")) return NUMA_NO_NODE; @@ -3256,7 +3263,20 @@ static int migration_offline_cpu(unsigned int cpu) static int __init migrate_on_reclaim_init(void) { - int ret; + struct demotion_nodes *nd; + int ret, node; + + /* Keep the maximum target demotion nodes are less than MAX_NUMNODES. */ + target_nodes_max = min_t(unsigned short, DEFAULT_DEMOTION_TARGET_NODES, + MAX_NUMNODES - 1); + for_each_node(node) { + nd = kmalloc(struct_size(nd, nodes, target_nodes_max), + GFP_KERNEL); + if (!nd) + continue; + + node_demotion[node] = nd; + } ret = cpuhp_setup_state_nocalls(CPUHP_MM_DEMOTION_DEAD, "mm/demotion:offline", NULL, migration_offline_cpu);
For the worst case (MAX_NUMNODES=1024), the node_demotion structure can consume 32k bytes, which appears too large, so we can change to allocate node_demotion dynamically at initialization time. Meanwhile allocating the target demotion nodes array dynamically to select a suitable size according to the MAX_NUMNODES. Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> --- mm/migrate.c | 38 +++++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-)