| Message ID | 20140218235800.GC10844@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 18.02.2014 [15:58:00 -0800], Nishanth Aravamudan wrote: > On 18.02.2014 [15:34:05 -0800], 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? > > > > 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. > > How about the following? > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 5de4337..1a0eced 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -1854,7 +1854,8 @@ static void __paginginit init_zone_allows_reclaim(int nid) > int i; > > for_each_online_node(i) > - if (node_distance(nid, i) <= RECLAIM_DISTANCE) > + if (node_distance(nid, i) <= RECLAIM_DISTANCE || > + !NODE_DATA(nid)->node_present_pages) err s/nid/i/ above. -Nish
On Tue, 18 Feb 2014, Nishanth Aravamudan wrote: > How about the following? > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 5de4337..1a0eced 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -1854,7 +1854,8 @@ static void __paginginit init_zone_allows_reclaim(int nid) > int i; > > for_each_online_node(i) > - if (node_distance(nid, i) <= RECLAIM_DISTANCE) > + if (node_distance(nid, i) <= RECLAIM_DISTANCE || > + !NODE_DATA(i)->node_present_pages) > node_set(i, NODE_DATA(nid)->reclaim_nodes); > else > zone_reclaim_mode = 1; [ I changed the above from NODE_DATA(nid) -> NODE_DATA(i) as you caught so we're looking at the right code. ] That can't be right, it would allow reclaiming from a memoryless node. I think what you want is for_each_online_node(i) { if (!node_present_pages(i)) continue; if (node_distance(nid, i) <= RECLAIM_DISTANCE) { node_set(i, NODE_DATA(nid)->reclaim_nodes); continue; } /* Always try to reclaim locally */ zone_reclaim_mode = 1; } but we really should be able to do for_each_node_state(i, N_MEMORY) here and memoryless nodes should already be excluded from that mask. > @@ -4901,13 +4902,13 @@ void __paginginit free_area_init_node(int nid, unsigned long *zones_size, > > pgdat->node_id = nid; > pgdat->node_start_pfn = node_start_pfn; > - init_zone_allows_reclaim(nid); > #ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP > get_pfn_range_for_nid(nid, &start_pfn, &end_pfn); > #endif > calculate_node_totalpages(pgdat, start_pfn, end_pfn, > zones_size, zholes_size); > > + init_zone_allows_reclaim(nid); > alloc_node_mem_map(pgdat); > #ifdef CONFIG_FLAT_NODE_MEM_MAP > printk(KERN_DEBUG "free_area_init_node: node %d, pgdat %08lx, node_mem_map %08lx\n", > > I think it's safe to move init_zone_allows_reclaim, because I don't > think any allocates are occurring here that could cause us to reclaim > anyways, right? Moving it allows us to safely reference > node_present_pages. > Yeah, this is fine.
On Tue 18-02-14 17:43:38, David Rientjes wrote: > On Tue, 18 Feb 2014, Nishanth Aravamudan wrote: > > > How about the following? > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index 5de4337..1a0eced 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -1854,7 +1854,8 @@ static void __paginginit init_zone_allows_reclaim(int nid) > > int i; > > > > for_each_online_node(i) > > - if (node_distance(nid, i) <= RECLAIM_DISTANCE) > > + if (node_distance(nid, i) <= RECLAIM_DISTANCE || > > + !NODE_DATA(i)->node_present_pages) > > node_set(i, NODE_DATA(nid)->reclaim_nodes); > > else > > zone_reclaim_mode = 1; > > [ I changed the above from NODE_DATA(nid) -> NODE_DATA(i) as you caught > so we're looking at the right code. ] > > That can't be right, it would allow reclaiming from a memoryless node. I > think what you want is > > for_each_online_node(i) { > if (!node_present_pages(i)) > continue; > if (node_distance(nid, i) <= RECLAIM_DISTANCE) { > node_set(i, NODE_DATA(nid)->reclaim_nodes); > continue; > } > /* Always try to reclaim locally */ > zone_reclaim_mode = 1; > } > > but we really should be able to do for_each_node_state(i, N_MEMORY) here > and memoryless nodes should already be excluded from that mask. Agreed! Actually the code I am currently interested in is based on 3.0 kernel where zone_reclaim_mode is set in build_zonelists which relies on find_next_best_node which iterates only N_HIGH_MEMORY nodes which should have non 0 present pages. [...]
On 18.02.2014 [17:43:38 -0800], David Rientjes wrote: > On Tue, 18 Feb 2014, Nishanth Aravamudan wrote: > > > How about the following? > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index 5de4337..1a0eced 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -1854,7 +1854,8 @@ static void __paginginit init_zone_allows_reclaim(int nid) > > int i; > > > > for_each_online_node(i) > > - if (node_distance(nid, i) <= RECLAIM_DISTANCE) > > + if (node_distance(nid, i) <= RECLAIM_DISTANCE || > > + !NODE_DATA(i)->node_present_pages) > > node_set(i, NODE_DATA(nid)->reclaim_nodes); > > else > > zone_reclaim_mode = 1; > > [ I changed the above from NODE_DATA(nid) -> NODE_DATA(i) as you caught > so we're looking at the right code. ] > > That can't be right, it would allow reclaiming from a memoryless node. I > think what you want is Gah, you're right. > for_each_online_node(i) { > if (!node_present_pages(i)) > continue; > if (node_distance(nid, i) <= RECLAIM_DISTANCE) { > node_set(i, NODE_DATA(nid)->reclaim_nodes); > continue; > } > /* Always try to reclaim locally */ > zone_reclaim_mode = 1; > } > > but we really should be able to do for_each_node_state(i, N_MEMORY) here > and memoryless nodes should already be excluded from that mask. Yep, I found that afterwards, which simplifies the logic. I'll add this to my series :) <snip> > > I think it's safe to move init_zone_allows_reclaim, because I don't > > think any allocates are occurring here that could cause us to reclaim > > anyways, right? Moving it allows us to safely reference > > node_present_pages. > > > > Yeah, this is fine. Thanks, Nish
On 19.02.2014 [08:24:38 -0800], Nishanth Aravamudan wrote: > On 18.02.2014 [17:43:38 -0800], David Rientjes wrote: > > On Tue, 18 Feb 2014, Nishanth Aravamudan wrote: > > > > > How about the following? > > > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > > index 5de4337..1a0eced 100644 > > > --- a/mm/page_alloc.c > > > +++ b/mm/page_alloc.c > > > @@ -1854,7 +1854,8 @@ static void __paginginit init_zone_allows_reclaim(int nid) > > > int i; > > > > > > for_each_online_node(i) > > > - if (node_distance(nid, i) <= RECLAIM_DISTANCE) > > > + if (node_distance(nid, i) <= RECLAIM_DISTANCE || > > > + !NODE_DATA(i)->node_present_pages) > > > node_set(i, NODE_DATA(nid)->reclaim_nodes); > > > else > > > zone_reclaim_mode = 1; > > > > [ I changed the above from NODE_DATA(nid) -> NODE_DATA(i) as you caught > > so we're looking at the right code. ] > > > > That can't be right, it would allow reclaiming from a memoryless node. I > > think what you want is > > Gah, you're right. > > > for_each_online_node(i) { > > if (!node_present_pages(i)) > > continue; > > if (node_distance(nid, i) <= RECLAIM_DISTANCE) { > > node_set(i, NODE_DATA(nid)->reclaim_nodes); > > continue; > > } > > /* Always try to reclaim locally */ > > zone_reclaim_mode = 1; > > } > > > > but we really should be able to do for_each_node_state(i, N_MEMORY) here > > and memoryless nodes should already be excluded from that mask. > > Yep, I found that afterwards, which simplifies the logic. I'll add this > to my series :) In looking at the code, I am wondering about the following: init_zone_allows_reclaim() is called for each nid from free_area_init_node(). Which means that calculate_node_totalpages for other "later" nids and check_for_memory() [which sets up the N_MEMORY nodemask] hasn't been called yet. So, would it make sense to pull up the /* Any memory on that node */ if (pgdat->node_present_pages) node_set_state(nid, N_MEMORY); check_for_memory(pgdat, nid); into free_area_init_node()? Thanks, Nish
On Wed 19-02-14 08:33:45, Nishanth Aravamudan wrote: > On 19.02.2014 [08:24:38 -0800], Nishanth Aravamudan wrote: > > On 18.02.2014 [17:43:38 -0800], David Rientjes wrote: > > > On Tue, 18 Feb 2014, Nishanth Aravamudan wrote: > > > > > > > How about the following? > > > > > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > > > index 5de4337..1a0eced 100644 > > > > --- a/mm/page_alloc.c > > > > +++ b/mm/page_alloc.c > > > > @@ -1854,7 +1854,8 @@ static void __paginginit init_zone_allows_reclaim(int nid) > > > > int i; > > > > > > > > for_each_online_node(i) > > > > - if (node_distance(nid, i) <= RECLAIM_DISTANCE) > > > > + if (node_distance(nid, i) <= RECLAIM_DISTANCE || > > > > + !NODE_DATA(i)->node_present_pages) > > > > node_set(i, NODE_DATA(nid)->reclaim_nodes); > > > > else > > > > zone_reclaim_mode = 1; > > > > > > [ I changed the above from NODE_DATA(nid) -> NODE_DATA(i) as you caught > > > so we're looking at the right code. ] > > > > > > That can't be right, it would allow reclaiming from a memoryless node. I > > > think what you want is > > > > Gah, you're right. > > > > > for_each_online_node(i) { > > > if (!node_present_pages(i)) > > > continue; > > > if (node_distance(nid, i) <= RECLAIM_DISTANCE) { > > > node_set(i, NODE_DATA(nid)->reclaim_nodes); > > > continue; > > > } > > > /* Always try to reclaim locally */ > > > zone_reclaim_mode = 1; > > > } > > > > > > but we really should be able to do for_each_node_state(i, N_MEMORY) here > > > and memoryless nodes should already be excluded from that mask. > > > > Yep, I found that afterwards, which simplifies the logic. I'll add this > > to my series :) > > In looking at the code, I am wondering about the following: > > init_zone_allows_reclaim() is called for each nid from > free_area_init_node(). Which means that calculate_node_totalpages for > other "later" nids and check_for_memory() [which sets up the N_MEMORY > nodemask] hasn't been called yet. > > So, would it make sense to pull up the > /* Any memory on that node */ > if (pgdat->node_present_pages) > node_set_state(nid, N_MEMORY); > check_for_memory(pgdat, nid); > into free_area_init_node()? Dunno, but it shouldn't be needed because nodes are set N_MEMORY earlier in early_calculate_totalpages as mentioned in other email.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 5de4337..1a0eced 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -1854,7 +1854,8 @@ static void __paginginit init_zone_allows_reclaim(int nid) int i; for_each_online_node(i) - if (node_distance(nid, i) <= RECLAIM_DISTANCE) + if (node_distance(nid, i) <= RECLAIM_DISTANCE || + !NODE_DATA(nid)->node_present_pages) node_set(i, NODE_DATA(nid)->reclaim_nodes); else zone_reclaim_mode = 1; @@ -4901,13 +4902,13 @@ void __paginginit free_area_init_node(int nid, unsigned long *zones_size, pgdat->node_id = nid; pgdat->node_start_pfn = node_start_pfn; - init_zone_allows_reclaim(nid); #ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP get_pfn_range_for_nid(nid, &start_pfn, &end_pfn); #endif calculate_node_totalpages(pgdat, start_pfn, end_pfn, zones_size, zholes_size); + init_zone_allows_reclaim(nid); alloc_node_mem_map(pgdat); #ifdef CONFIG_FLAT_NODE_MEM_MAP printk(KERN_DEBUG "free_area_init_node: node %d, pgdat %08lx, node_mem_map %08lx\n",