| Message ID | 20140218233404.GB10844@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue 18-02-14 15:34:05, Nishanth Aravamudan wrote: > Hi Michal, > > On 18.02.2014 [10:06:58 +0100], Michal Hocko wrote: > > Hi, > > I have just noticed that ppc has RECLAIM_DISTANCE reduced to 10 set by > > 56608209d34b (powerpc/numa: Set a smaller value for RECLAIM_DISTANCE to > > enable zone reclaim). The commit message suggests that the zone reclaim > > is desirable for all NUMA configurations. > > > > History has shown that the zone reclaim is more often harmful than > > helpful and leads to performance problems. The default RECLAIM_DISTANCE > > for generic case has been increased from 20 to 30 around 3.0 > > (32e45ff43eaf mm: increase RECLAIM_DISTANCE to 30). > > Interesting. > > > I strongly suspect that the patch is incorrect and it should be > > reverted. Before I will send a revert I would like to understand what > > led to the patch in the first place. I do not see why would PPC use only > > LOCAL_DISTANCE and REMOTE_DISTANCE distances and in fact machines I have > > seen use different values. > > > > Anton, could you comment please? > > I'll let Anton comment here, but in looking into this issue in working > on CONFIG_HAVE_MEMORYLESS_NODE support, I realized that any LPAR with > memoryless nodes will set zone_reclaim_mode to 1. I think we want to > ignore memoryless nodes when we set up the reclaim mode like the > following? I'll send it as a proper patch if you agree? Funny enough, ppc memoryless node setup is what led me to this code. We had a setup like this: node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 node 0 size: 0 MB node 0 free: 0 MB node 2 cpus: node 2 size: 7168 MB node 2 free: 6019 MB node distances: node 0 2 0: 10 40 2: 40 10 Which ends up enabling zone_reclaim although there is only a single node with memory. Not that RECLAIM_DISTANCE would make any difference here as the distance is even above default RECLAIM_DISTANCE. > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 5de4337..4f6ff6f 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -1853,8 +1853,9 @@ static void __paginginit init_zone_allows_reclaim(int nid) > { > int i; > > - for_each_online_node(i) > - if (node_distance(nid, i) <= RECLAIM_DISTANCE) > + for_each_online_node(i) { > + if (node_distance(nid, i) <= RECLAIM_DISTANCE || > + local_memory_node(nid) != nid) > node_set(i, NODE_DATA(nid)->reclaim_nodes); > else > zone_reclaim_mode = 1; > > Note, this won't actually do anything if CONFIG_HAVE_MEMORYLESS_NODES is > not set, but if it is, I think semantically it will indicate that > memoryless nodes *have* to reclaim remotely. > > And actually the above won't work, because the callpath is > > start_kernel -> setup_arch -> paging_init [-> free_area_init_nodes -> > free_area_init_node -> init_zone_allows_reclaim] which is called before > build_all_zonelists. This is a similar ordering problem as I'm having > with the MEMORYLESS_NODE support, will work on it. I think you just want for_each_node_state(nid, N_MEMORY) and skip all the memory less nodes, no?
On 19.02.2014 [09:23:13 +0100], Michal Hocko wrote: > On Tue 18-02-14 15:34:05, Nishanth Aravamudan wrote: > > Hi Michal, > > > > On 18.02.2014 [10:06:58 +0100], Michal Hocko wrote: > > > Hi, > > > I have just noticed that ppc has RECLAIM_DISTANCE reduced to 10 set by > > > 56608209d34b (powerpc/numa: Set a smaller value for RECLAIM_DISTANCE to > > > enable zone reclaim). The commit message suggests that the zone reclaim > > > is desirable for all NUMA configurations. > > > > > > History has shown that the zone reclaim is more often harmful than > > > helpful and leads to performance problems. The default RECLAIM_DISTANCE > > > for generic case has been increased from 20 to 30 around 3.0 > > > (32e45ff43eaf mm: increase RECLAIM_DISTANCE to 30). > > > > Interesting. > > > > > I strongly suspect that the patch is incorrect and it should be > > > reverted. Before I will send a revert I would like to understand what > > > led to the patch in the first place. I do not see why would PPC use only > > > LOCAL_DISTANCE and REMOTE_DISTANCE distances and in fact machines I have > > > seen use different values. > > > > > > Anton, could you comment please? > > > > I'll let Anton comment here, but in looking into this issue in working > > on CONFIG_HAVE_MEMORYLESS_NODE support, I realized that any LPAR with > > memoryless nodes will set zone_reclaim_mode to 1. I think we want to > > ignore memoryless nodes when we set up the reclaim mode like the > > following? I'll send it as a proper patch if you agree? > > Funny enough, ppc memoryless node setup is what led me to this code. > We had a setup like this: > node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 > node 0 size: 0 MB > node 0 free: 0 MB > node 2 cpus: > node 2 size: 7168 MB > node 2 free: 6019 MB > node distances: > node 0 2 > 0: 10 40 > 2: 40 10 Yeah, I think this happens fairly often ... and we didn't properly support it (particularly with SLUB) on powerpc. I'll cc you on my patchset. > Which ends up enabling zone_reclaim although there is only a single node > with memory. Not that RECLAIM_DISTANCE would make any difference here as > the distance is even above default RECLAIM_DISTANCE. > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index 5de4337..4f6ff6f 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -1853,8 +1853,9 @@ static void __paginginit init_zone_allows_reclaim(int nid) > > { > > int i; > > > > - for_each_online_node(i) > > - if (node_distance(nid, i) <= RECLAIM_DISTANCE) > > + for_each_online_node(i) { > > + if (node_distance(nid, i) <= RECLAIM_DISTANCE || > > + local_memory_node(nid) != nid) > > node_set(i, NODE_DATA(nid)->reclaim_nodes); > > else > > zone_reclaim_mode = 1; > > > > Note, this won't actually do anything if CONFIG_HAVE_MEMORYLESS_NODES is > > not set, but if it is, I think semantically it will indicate that > > memoryless nodes *have* to reclaim remotely. > > > > And actually the above won't work, because the callpath is > > > > start_kernel -> setup_arch -> paging_init [-> free_area_init_nodes -> > > free_area_init_node -> init_zone_allows_reclaim] which is called before > > build_all_zonelists. This is a similar ordering problem as I'm having > > with the MEMORYLESS_NODE support, will work on it. > > I think you just want for_each_node_state(nid, N_MEMORY) and skip all > the memory less nodes, no? Yep, thanks! -Nish
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 5de4337..4f6ff6f 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -1853,8 +1853,9 @@ static void __paginginit init_zone_allows_reclaim(int nid) { int i; - for_each_online_node(i) - if (node_distance(nid, i) <= RECLAIM_DISTANCE) + for_each_online_node(i) { + if (node_distance(nid, i) <= RECLAIM_DISTANCE || + local_memory_node(nid) != nid) node_set(i, NODE_DATA(nid)->reclaim_nodes); else zone_reclaim_mode = 1;