Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1687135 > unrolled thread

[PATCH 6/9] mm, page_alloc: simplify zonelist initialization

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 6/9] mm, page_alloc: simplify zonelist initialization Michal Hocko <mhocko@kernel.org> - 2017-07-14 10:10 +0200
    Re: [PATCH 6/9] mm, page_alloc: simplify zonelist initialization Mel Gorman <mgorman@suse.de> - 2017-07-14 12:00 +0200
      Re: [PATCH 6/9] mm, page_alloc: simplify zonelist initialization Michal Hocko <mhocko@kernel.org> - 2017-07-14 13:00 +0200
    Re: [PATCH 6/9] mm, page_alloc: simplify zonelist initialization Mel Gorman <mgorman@suse.de> - 2017-07-14 14:50 +0200
      Re: [PATCH 6/9] mm, page_alloc: simplify zonelist initialization Michal Hocko <mhocko@kernel.org> - 2017-07-14 15:10 +0200
        Re: [PATCH 6/9] mm, page_alloc: simplify zonelist initialization Mel Gorman <mgorman@suse.de> - 2017-07-14 16:20 +0200
          Re: [PATCH 6/9] mm, page_alloc: simplify zonelist initialization Michal Hocko <mhocko@kernel.org> - 2017-07-17 08:10 +0200
            Re: [PATCH 6/9] mm, page_alloc: simplify zonelist initialization Mel Gorman <mgorman@suse.de> - 2017-07-17 10:10 +0200
              Re: [PATCH 6/9] mm, page_alloc: simplify zonelist initialization Michal Hocko <mhocko@kernel.org> - 2017-07-17 10:20 +0200
                Re: [PATCH 6/9] mm, page_alloc: simplify zonelist initialization Mel Gorman <mgorman@suse.de> - 2017-07-17 11:00 +0200
                  Re: [PATCH 6/9] mm, page_alloc: simplify zonelist initialization Michal Hocko <mhocko@kernel.org> - 2017-07-17 11:20 +0200

#1687135 — [PATCH 6/9] mm, page_alloc: simplify zonelist initialization

From: Michal Hocko <mhocko@suse.com>

build_zonelists gradually builds zonelists from the nearest to the most
distant node. As we do not know how many populated zones we will have in
each node we rely on the _zoneref to terminate initialized part of the
zonelist by a NULL zone. While this is functionally correct it is quite
suboptimal because we cannot allow updaters to race with zonelists
users because they could see an empty zonelist and fail the allocation
or hit the OOM killer in the worst case.

We can do much better, though. We can store the node ordering into an
already existing node_order array and then give this array to
build_zonelists_in_node_order and do the whole initialization at once.
zonelists consumers still might see halfway initialized state but that
should be much more tolerateable because the list will not be empty and
they would either see some zone twice or skip over some zone(s) in the
worst case which shouldn't lead to immediate failures.

This patch alone doesn't introduce any functional change yet, though, it
is merely a preparatory work for later changes.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/page_alloc.c | 42 ++++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 00e117922b3f..78bd62418380 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4913,17 +4913,20 @@ static int find_next_best_node(int node, nodemask_t *used_node_mask)
  * This results in maximum locality--normal zone overflows into local
  * DMA zone, if any--but risks exhausting DMA zone.
  */
-static void build_zonelists_in_node_order(pg_data_t *pgdat, int node)
+static void build_zonelists_in_node_order(pg_data_t *pgdat, int *node_order)
 {
-	int j;
 	struct zonelist *zonelist;
+	int i, zoneref_idx = 0;
 
 	zonelist = &pgdat->node_zonelists[ZONELIST_FALLBACK];
-	for (j = 0; zonelist->_zonerefs[j].zone != NULL; j++)
-		;
-	j = build_zonelists_node(NODE_DATA(node), zonelist, j);
-	zonelist->_zonerefs[j].zone = NULL;
-	zonelist->_zonerefs[j].zone_idx = 0;
+
+	for (i = 0; i < MAX_NUMNODES; i++) {
+		pg_data_t *node = NODE_DATA(node_order[i]);
+
+		zoneref_idx = build_zonelists_node(node, zonelist, zoneref_idx);
+	}
+	zonelist->_zonerefs[zoneref_idx].zone = NULL;
+	zonelist->_zonerefs[zoneref_idx].zone_idx = 0;
 }
 
 /*
@@ -4931,13 +4934,13 @@ static void build_zonelists_in_node_order(pg_data_t *pgdat, int node)
  */
 static void build_thisnode_zonelists(pg_data_t *pgdat)
 {
-	int j;
 	struct zonelist *zonelist;
+	int zoneref_idx = 0;
 
 	zonelist = &pgdat->node_zonelists[ZONELIST_NOFALLBACK];
-	j = build_zonelists_node(pgdat, zonelist, 0);
-	zonelist->_zonerefs[j].zone = NULL;
-	zonelist->_zonerefs[j].zone_idx = 0;
+	zoneref_idx = build_zonelists_node(pgdat, zonelist, zoneref_idx);
+	zonelist->_zonerefs[zoneref_idx].zone = NULL;
+	zonelist->_zonerefs[zoneref_idx].zone_idx = 0;
 }
 
 /*
@@ -4946,21 +4949,13 @@ static void build_thisnode_zonelists(pg_data_t *pgdat)
  * exhausted, but results in overflowing to remote node while memory
  * may still exist in local DMA zone.
  */
-static int node_order[MAX_NUMNODES];
 
 static void build_zonelists(pg_data_t *pgdat)
 {
-	int i, node, load;
+	static int node_order[MAX_NUMNODES];
+	int node, load, i = 0;
 	nodemask_t used_mask;
 	int local_node, prev_node;
-	struct zonelist *zonelist;
-
-	/* initialize zonelists */
-	for (i = 0; i < MAX_ZONELISTS; i++) {
-		zonelist = pgdat->node_zonelists + i;
-		zonelist->_zonerefs[0].zone = NULL;
-		zonelist->_zonerefs[0].zone_idx = 0;
-	}
 
 	/* NUMA-aware ordering of nodes */
 	local_node = pgdat->node_id;
@@ -4969,8 +4964,6 @@ static void build_zonelists(pg_data_t *pgdat)
 	nodes_clear(used_mask);
 
 	memset(node_order, 0, sizeof(node_order));
-	i = 0;
-
 	while ((node = find_next_best_node(local_node, &used_mask)) >= 0) {
 		/*
 		 * We don't want to pressure a particular node.
@@ -4981,11 +4974,12 @@ static void build_zonelists(pg_data_t *pgdat)
 		    node_distance(local_node, prev_node))
 			node_load[node] = load;
 
+		node_order[i++] = node;
 		prev_node = node;
 		load--;
-		build_zonelists_in_node_order(pgdat, node);
 	}
 
+	build_zonelists_in_node_order(pgdat, node_order);
 	build_thisnode_zonelists(pgdat);
 }
 
-- 
2.11.0

[toc] | [next] | [standalone]

#1687213

On Fri, Jul 14, 2017 at 10:00:03AM +0200, Michal Hocko wrote:
>  
>  	zonelist = &pgdat->node_zonelists[ZONELIST_NOFALLBACK];
> -	j = build_zonelists_node(pgdat, zonelist, 0);
> -	zonelist->_zonerefs[j].zone = NULL;
> -	zonelist->_zonerefs[j].zone_idx = 0;
> +	zoneref_idx = build_zonelists_node(pgdat, zonelist, zoneref_idx);
> +	zonelist->_zonerefs[zoneref_idx].zone = NULL;
> +	zonelist->_zonerefs[zoneref_idx].zone_idx = 0;
>  }
>  
>  /*
> @@ -4946,21 +4949,13 @@ static void build_thisnode_zonelists(pg_data_t *pgdat)
>   * exhausted, but results in overflowing to remote node while memory
>   * may still exist in local DMA zone.
>   */
> -static int node_order[MAX_NUMNODES];
>  
>  static void build_zonelists(pg_data_t *pgdat)
>  {
> -	int i, node, load;
> +	static int node_order[MAX_NUMNODES];
> +	int node, load, i = 0;

Emm, node_order can be large. The first distro config I checked
indicated that this is 8K. I got hung up on that part and didn't look
closely at the rest of the patch.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1687246

On Fri 14-07-17 10:55:34, Mel Gorman wrote:
> On Fri, Jul 14, 2017 at 10:00:03AM +0200, Michal Hocko wrote:
> >  
> >  	zonelist = &pgdat->node_zonelists[ZONELIST_NOFALLBACK];
> > -	j = build_zonelists_node(pgdat, zonelist, 0);
> > -	zonelist->_zonerefs[j].zone = NULL;
> > -	zonelist->_zonerefs[j].zone_idx = 0;
> > +	zoneref_idx = build_zonelists_node(pgdat, zonelist, zoneref_idx);
> > +	zonelist->_zonerefs[zoneref_idx].zone = NULL;
> > +	zonelist->_zonerefs[zoneref_idx].zone_idx = 0;
> >  }
> >  
> >  /*
> > @@ -4946,21 +4949,13 @@ static void build_thisnode_zonelists(pg_data_t *pgdat)
> >   * exhausted, but results in overflowing to remote node while memory
> >   * may still exist in local DMA zone.
> >   */
> > -static int node_order[MAX_NUMNODES];
> >  
> >  static void build_zonelists(pg_data_t *pgdat)
> >  {
> > -	int i, node, load;
> > +	static int node_order[MAX_NUMNODES];
> > +	int node, load, i = 0;
> 
> Emm, node_order can be large. The first distro config I checked
> indicated that this is 8K. I got hung up on that part and didn't look
> closely at the rest of the patch.

yes, that's why I kept it static. I just placed it into the function to
make it clear what the scope is.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1687363

On Fri, Jul 14, 2017 at 10:00:03AM +0200, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> build_zonelists gradually builds zonelists from the nearest to the most
> distant node. As we do not know how many populated zones we will have in
> each node we rely on the _zoneref to terminate initialized part of the
> zonelist by a NULL zone. While this is functionally correct it is quite
> suboptimal because we cannot allow updaters to race with zonelists
> users because they could see an empty zonelist and fail the allocation
> or hit the OOM killer in the worst case.
> 
> We can do much better, though. We can store the node ordering into an
> already existing node_order array and then give this array to
> build_zonelists_in_node_order and do the whole initialization at once.
> zonelists consumers still might see halfway initialized state but that
> should be much more tolerateable because the list will not be empty and
> they would either see some zone twice or skip over some zone(s) in the
> worst case which shouldn't lead to immediate failures.
> 
> This patch alone doesn't introduce any functional change yet, though, it
> is merely a preparatory work for later changes.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  mm/page_alloc.c | 42 ++++++++++++++++++------------------------
>  1 file changed, 18 insertions(+), 24 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 00e117922b3f..78bd62418380 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4913,17 +4913,20 @@ static int find_next_best_node(int node, nodemask_t *used_node_mask)
>   * This results in maximum locality--normal zone overflows into local
>   * DMA zone, if any--but risks exhausting DMA zone.
>   */
> -static void build_zonelists_in_node_order(pg_data_t *pgdat, int node)
> +static void build_zonelists_in_node_order(pg_data_t *pgdat, int *node_order)
>  {
> -	int j;
>  	struct zonelist *zonelist;
> +	int i, zoneref_idx = 0;
>  
>  	zonelist = &pgdat->node_zonelists[ZONELIST_FALLBACK];
> -	for (j = 0; zonelist->_zonerefs[j].zone != NULL; j++)
> -		;
> -	j = build_zonelists_node(NODE_DATA(node), zonelist, j);
> -	zonelist->_zonerefs[j].zone = NULL;
> -	zonelist->_zonerefs[j].zone_idx = 0;
> +
> +	for (i = 0; i < MAX_NUMNODES; i++) {
> +		pg_data_t *node = NODE_DATA(node_order[i]);
> +
> +		zoneref_idx = build_zonelists_node(node, zonelist, zoneref_idx);
> +	}

The naming here is weird to say the least and makes this a lot more
confusing than it needs to be. Primarily, it's because the zoneref_idx
parameter gets renamed to nr_zones in build_zonelists_node where it's
nothing to do with the number of zones at all.

It also iterates for longer than it needs to. MAX_NUMNODES can be a
large value of mostly empty nodes but it happily goes through them
anyway. Pass zoneref_idx in as a pointer that is updated by the function
and use the return value to break the loop when an empty node is
encountered?

> +	zonelist->_zonerefs[zoneref_idx].zone = NULL;
> +	zonelist->_zonerefs[zoneref_idx].zone_idx = 0;
>  }
>  

It *might* be safer given the next patch to zero out the remainder of
the _zonerefs to that there is no combination of node add/remove that has
an iterator working with a semi-valid _zoneref which is beyond the last
correct value. It *should* be safe as the very last entry will always
be null but if you don't zero it out, it is possible for iterators to be
working beyond the "end" of the zonelist for a short window.

Otherwise think it's ok including my stupid comment about node_order
stack usage.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1687398

On Fri 14-07-17 13:46:46, Mel Gorman wrote:
> On Fri, Jul 14, 2017 at 10:00:03AM +0200, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > build_zonelists gradually builds zonelists from the nearest to the most
> > distant node. As we do not know how many populated zones we will have in
> > each node we rely on the _zoneref to terminate initialized part of the
> > zonelist by a NULL zone. While this is functionally correct it is quite
> > suboptimal because we cannot allow updaters to race with zonelists
> > users because they could see an empty zonelist and fail the allocation
> > or hit the OOM killer in the worst case.
> > 
> > We can do much better, though. We can store the node ordering into an
> > already existing node_order array and then give this array to
> > build_zonelists_in_node_order and do the whole initialization at once.
> > zonelists consumers still might see halfway initialized state but that
> > should be much more tolerateable because the list will not be empty and
> > they would either see some zone twice or skip over some zone(s) in the
> > worst case which shouldn't lead to immediate failures.
> > 
> > This patch alone doesn't introduce any functional change yet, though, it
> > is merely a preparatory work for later changes.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > ---
> >  mm/page_alloc.c | 42 ++++++++++++++++++------------------------
> >  1 file changed, 18 insertions(+), 24 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 00e117922b3f..78bd62418380 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -4913,17 +4913,20 @@ static int find_next_best_node(int node, nodemask_t *used_node_mask)
> >   * This results in maximum locality--normal zone overflows into local
> >   * DMA zone, if any--but risks exhausting DMA zone.
> >   */
> > -static void build_zonelists_in_node_order(pg_data_t *pgdat, int node)
> > +static void build_zonelists_in_node_order(pg_data_t *pgdat, int *node_order)
> >  {
> > -	int j;
> >  	struct zonelist *zonelist;
> > +	int i, zoneref_idx = 0;
> >  
> >  	zonelist = &pgdat->node_zonelists[ZONELIST_FALLBACK];
> > -	for (j = 0; zonelist->_zonerefs[j].zone != NULL; j++)
> > -		;
> > -	j = build_zonelists_node(NODE_DATA(node), zonelist, j);
> > -	zonelist->_zonerefs[j].zone = NULL;
> > -	zonelist->_zonerefs[j].zone_idx = 0;
> > +
> > +	for (i = 0; i < MAX_NUMNODES; i++) {
> > +		pg_data_t *node = NODE_DATA(node_order[i]);
> > +
> > +		zoneref_idx = build_zonelists_node(node, zonelist, zoneref_idx);
> > +	}
> 
> The naming here is weird to say the least and makes this a lot more
> confusing than it needs to be. Primarily, it's because the zoneref_idx
> parameter gets renamed to nr_zones in build_zonelists_node where it's
> nothing to do with the number of zones at all.

you are right. I just wanted to get rid of `j' and didn't realize
nr_zones would fit much better.

> It also iterates for longer than it needs to. MAX_NUMNODES can be a
> large value of mostly empty nodes but it happily goes through them
> anyway. Pass zoneref_idx in as a pointer that is updated by the function
> and use the return value to break the loop when an empty node is
> encountered?
> 
> > +	zonelist->_zonerefs[zoneref_idx].zone = NULL;
> > +	zonelist->_zonerefs[zoneref_idx].zone_idx = 0;
> >  }
> >  
> 
> It *might* be safer given the next patch to zero out the remainder of
> the _zonerefs to that there is no combination of node add/remove that has
> an iterator working with a semi-valid _zoneref which is beyond the last
> correct value. It *should* be safe as the very last entry will always
> be null but if you don't zero it out, it is possible for iterators to be
> working beyond the "end" of the zonelist for a short window.

yes that is true but there will always be terminating NULL zone and I
found that acceptable. It is basically the same thing as accessing an
empty zone or a zone twice. Or do you think this is absolutely necessary
to handle?

> Otherwise think it's ok including my stupid comment about node_order
> stack usage.

What do you think about this on top?
---
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 49bade7ff049..3b98524c04ec 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4913,20 +4913,21 @@ static int find_next_best_node(int node, nodemask_t *used_node_mask)
  * This results in maximum locality--normal zone overflows into local
  * DMA zone, if any--but risks exhausting DMA zone.
  */
-static void build_zonelists_in_node_order(pg_data_t *pgdat, int *node_order)
+static void build_zonelists_in_node_order(pg_data_t *pgdat, int *node_order,
+		unsigned nr_nodes)
 {
 	struct zonelist *zonelist;
-	int i, zoneref_idx = 0;
+	int i, nr_zones = 0;
 
 	zonelist = &pgdat->node_zonelists[ZONELIST_FALLBACK];
 
-	for (i = 0; i < MAX_NUMNODES; i++) {
+	for (i = 0; i < nr_nodes; i++) {
 		pg_data_t *node = NODE_DATA(node_order[i]);
 
-		zoneref_idx = build_zonelists_node(node, zonelist, zoneref_idx);
+		nr_zones = build_zonelists_node(node, zonelist, nr_zones);
 	}
-	zonelist->_zonerefs[zoneref_idx].zone = NULL;
-	zonelist->_zonerefs[zoneref_idx].zone_idx = 0;
+	zonelist->_zonerefs[nr_zones].zone = NULL;
+	zonelist->_zonerefs[nr_zones].zone_idx = 0;
 }
 
 /*
@@ -4935,12 +4936,12 @@ static void build_zonelists_in_node_order(pg_data_t *pgdat, int *node_order)
 static void build_thisnode_zonelists(pg_data_t *pgdat)
 {
 	struct zonelist *zonelist;
-	int zoneref_idx = 0;
+	int nr_zones = 0;
 
 	zonelist = &pgdat->node_zonelists[ZONELIST_NOFALLBACK];
-	zoneref_idx = build_zonelists_node(pgdat, zonelist, zoneref_idx);
-	zonelist->_zonerefs[zoneref_idx].zone = NULL;
-	zonelist->_zonerefs[zoneref_idx].zone_idx = 0;
+	nr_zones = build_zonelists_node(pgdat, zonelist, nr_zones);
+	zonelist->_zonerefs[nr_zones].zone = NULL;
+	zonelist->_zonerefs[nr_zones].zone_idx = 0;
 }
 
 /*
@@ -4979,7 +4980,7 @@ static void build_zonelists(pg_data_t *pgdat)
 		load--;
 	}
 
-	build_zonelists_in_node_order(pgdat, node_order);
+	build_zonelists_in_node_order(pgdat, node_order, i);
 	build_thisnode_zonelists(pgdat);
 }
 
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1687468

On Fri, Jul 14, 2017 at 03:02:42PM +0200, Michal Hocko wrote:
> > It *might* be safer given the next patch to zero out the remainder of
> > the _zonerefs to that there is no combination of node add/remove that has
> > an iterator working with a semi-valid _zoneref which is beyond the last
> > correct value. It *should* be safe as the very last entry will always
> > be null but if you don't zero it out, it is possible for iterators to be
> > working beyond the "end" of the zonelist for a short window.
> 
> yes that is true but there will always be terminating NULL zone and I
> found that acceptable. It is basically the same thing as accessing an
> empty zone or a zone twice. Or do you think this is absolutely necessary
> to handle?
> 

I don't think it's absolutely necessary. While you could construct some
odd behaviour for iterators currently past the end of the list, they would
eventually encounter a NULL.

> > Otherwise think it's ok including my stupid comment about node_order
> > stack usage.
> 
> What do you think about this on top?
> ---
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 49bade7ff049..3b98524c04ec 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4913,20 +4913,21 @@ static int find_next_best_node(int node, nodemask_t *used_node_mask)
>   * This results in maximum locality--normal zone overflows into local
>   * DMA zone, if any--but risks exhausting DMA zone.
>   */
> -static void build_zonelists_in_node_order(pg_data_t *pgdat, int *node_order)
> +static void build_zonelists_in_node_order(pg_data_t *pgdat, int *node_order,
> +		unsigned nr_nodes)
>  {
>  	struct zonelist *zonelist;
> -	int i, zoneref_idx = 0;
> +	int i, nr_zones = 0;
>  
>  	zonelist = &pgdat->node_zonelists[ZONELIST_FALLBACK];
>  
> -	for (i = 0; i < MAX_NUMNODES; i++) {
> +	for (i = 0; i < nr_nodes; i++) {

The first iteration is then -- for (i = 0; i < 0; i++)

Fairly sure that's not what you meant.


>  		pg_data_t *node = NODE_DATA(node_order[i]);
>  
> -		zoneref_idx = build_zonelists_node(node, zonelist, zoneref_idx);
> +		nr_zones = build_zonelists_node(node, zonelist, nr_zones);

I meant converting build_zonelists_node and passing in &nr_zones and
returning false when an empty node is encountered. In this context,
it's also not about zones, it really is nr_zonerefs. Rename nr_zones in
build_zonelists_node as well.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1688684

On Fri 14-07-17 15:18:23, Mel Gorman wrote:
> On Fri, Jul 14, 2017 at 03:02:42PM +0200, Michal Hocko wrote:
[...]
> > What do you think about this on top?
> > ---
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 49bade7ff049..3b98524c04ec 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -4913,20 +4913,21 @@ static int find_next_best_node(int node, nodemask_t *used_node_mask)
> >   * This results in maximum locality--normal zone overflows into local
> >   * DMA zone, if any--but risks exhausting DMA zone.
> >   */
> > -static void build_zonelists_in_node_order(pg_data_t *pgdat, int *node_order)
> > +static void build_zonelists_in_node_order(pg_data_t *pgdat, int *node_order,
> > +		unsigned nr_nodes)
> >  {
> >  	struct zonelist *zonelist;
> > -	int i, zoneref_idx = 0;
> > +	int i, nr_zones = 0;
> >  
> >  	zonelist = &pgdat->node_zonelists[ZONELIST_FALLBACK];
> >  
> > -	for (i = 0; i < MAX_NUMNODES; i++) {
> > +	for (i = 0; i < nr_nodes; i++) {
> 
> The first iteration is then -- for (i = 0; i < 0; i++)

find_next_best_node always returns at least one node (the local one) so
I believe that nr_nodes should never be 0.

> Fairly sure that's not what you meant.
> 
> 
> >  		pg_data_t *node = NODE_DATA(node_order[i]);
> >  
> > -		zoneref_idx = build_zonelists_node(node, zonelist, zoneref_idx);
> > +		nr_zones = build_zonelists_node(node, zonelist, nr_zones);
> 
> I meant converting build_zonelists_node and passing in &nr_zones and
> returning false when an empty node is encountered. In this context,
> it's also not about zones, it really is nr_zonerefs. Rename nr_zones in
> build_zonelists_node as well.

hmm, why don't we rather make it zonerefs based then. Something
like the following?
---
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3b98524c04ec..01e67e629351 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4772,18 +4772,17 @@ static void zoneref_set_zone(struct zone *zone, struct zoneref *zoneref)
  *
  * Add all populated zones of a node to the zonelist.
  */
-static int build_zonelists_node(pg_data_t *pgdat, struct zonelist *zonelist,
-				int nr_zones)
+static int build_zonerefs_node(pg_data_t *pgdat, struct zoneref *zonerefs)
 {
 	struct zone *zone;
 	enum zone_type zone_type = MAX_NR_ZONES;
+	int nr_zones = 0;
 
 	do {
 		zone_type--;
 		zone = pgdat->node_zones + zone_type;
 		if (managed_zone(zone)) {
-			zoneref_set_zone(zone,
-				&zonelist->_zonerefs[nr_zones++]);
+			zoneref_set_zone(zone, &zonerefs[nr_zones++]);
 			check_highest_zone(zone_type);
 		}
 	} while (zone_type);
@@ -4916,18 +4915,21 @@ static int find_next_best_node(int node, nodemask_t *used_node_mask)
 static void build_zonelists_in_node_order(pg_data_t *pgdat, int *node_order,
 		unsigned nr_nodes)
 {
-	struct zonelist *zonelist;
-	int i, nr_zones = 0;
+	struct zoneref *zonerefs;
+	int i;
 
-	zonelist = &pgdat->node_zonelists[ZONELIST_FALLBACK];
+	zonerefs = pgdat->node_zonelists[ZONELIST_FALLBACK]._zonerefs;
 
 	for (i = 0; i < nr_nodes; i++) {
+		int nr_zones;
+
 		pg_data_t *node = NODE_DATA(node_order[i]);
 
-		nr_zones = build_zonelists_node(node, zonelist, nr_zones);
+		nr_zones = build_zonerefs_node(node, zonerefs);
+		zonerefs += nr_zones;
 	}
-	zonelist->_zonerefs[nr_zones].zone = NULL;
-	zonelist->_zonerefs[nr_zones].zone_idx = 0;
+	zonerefs->zone = NULL;
+	zonerefs->zone_idx = 0;
 }
 
 /*
@@ -4935,13 +4937,14 @@ static void build_zonelists_in_node_order(pg_data_t *pgdat, int *node_order,
  */
 static void build_thisnode_zonelists(pg_data_t *pgdat)
 {
-	struct zonelist *zonelist;
-	int nr_zones = 0;
+	struct zoneref *zonerefs;
+	int nr_zones;
 
-	zonelist = &pgdat->node_zonelists[ZONELIST_NOFALLBACK];
-	nr_zones = build_zonelists_node(pgdat, zonelist, nr_zones);
-	zonelist->_zonerefs[nr_zones].zone = NULL;
-	zonelist->_zonerefs[nr_zones].zone_idx = 0;
+	zonerefs = pgdat->node_zonelists[ZONELIST_NOFALLBACK]._zonerefs;
+	nr_zones = build_zonerefs_node(pgdat, zonerefs);
+	zonerefs += nr_zones;
+	zonerefs->zone = NULL;
+	zonerefs->zone_idx = 0;
 }
 
 /*
@@ -5009,13 +5012,13 @@ static void setup_min_slab_ratio(void);
 static void build_zonelists(pg_data_t *pgdat)
 {
 	int node, local_node;
-	enum zone_type j;
-	struct zonelist *zonelist;
+	struct zoneref *zonerefs;
 
 	local_node = pgdat->node_id;
 
-	zonelist = &pgdat->node_zonelists[ZONELIST_FALLBACK];
-	j = build_zonelists_node(pgdat, zonelist, 0);
+	zonrefs = pgdat->node_zonelists[ZONELIST_FALLBACK]._zonerefs;
+	nr_zones = build_zonerefs_node(pgdat, zonerefs);
+	zonerefs += nr_zones;
 
 	/*
 	 * Now we build the zonelist so that it contains the zones
@@ -5028,16 +5031,18 @@ static void build_zonelists(pg_data_t *pgdat)
 	for (node = local_node + 1; node < MAX_NUMNODES; node++) {
 		if (!node_online(node))
 			continue;
-		j = build_zonelists_node(NODE_DATA(node), zonelist, j);
+		nr_zones = build_zonerefs_node(NODE_DATA(node), zonerefs);
+		zonerefs += nr_zones;
 	}
 	for (node = 0; node < local_node; node++) {
 		if (!node_online(node))
 			continue;
-		j = build_zonelists_node(NODE_DATA(node), zonelist, j);
+		nr_zones = build_zonerefs_node(NODE_DATA(node), zonerefs);
+		zonerefs += nr_zones;
 	}
 
-	zonelist->_zonerefs[j].zone = NULL;
-	zonelist->_zonerefs[j].zone_idx = 0;
+	zonerefs->zone = NULL;
+	zonerefs->zone_idx = 0;
 }
 
 #endif	/* CONFIG_NUMA */

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1688742

On Mon, Jul 17, 2017 at 08:06:40AM +0200, Michal Hocko wrote:
> On Fri 14-07-17 15:18:23, Mel Gorman wrote:
> > Fairly sure that's not what you meant.
> > 
> > 
> > >  		pg_data_t *node = NODE_DATA(node_order[i]);
> > >  
> > > -		zoneref_idx = build_zonelists_node(node, zonelist, zoneref_idx);
> > > +		nr_zones = build_zonelists_node(node, zonelist, nr_zones);
> > 
> > I meant converting build_zonelists_node and passing in &nr_zones and
> > returning false when an empty node is encountered. In this context,
> > it's also not about zones, it really is nr_zonerefs. Rename nr_zones in
> > build_zonelists_node as well.
> 
> hmm, why don't we rather make it zonerefs based then. Something
> like the following?

Works for me.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1688756

On Mon 17-07-17 09:07:23, Mel Gorman wrote:
> On Mon, Jul 17, 2017 at 08:06:40AM +0200, Michal Hocko wrote:
> > On Fri 14-07-17 15:18:23, Mel Gorman wrote:
> > > Fairly sure that's not what you meant.
> > > 
> > > 
> > > >  		pg_data_t *node = NODE_DATA(node_order[i]);
> > > >  
> > > > -		zoneref_idx = build_zonelists_node(node, zonelist, zoneref_idx);
> > > > +		nr_zones = build_zonelists_node(node, zonelist, nr_zones);
> > > 
> > > I meant converting build_zonelists_node and passing in &nr_zones and
> > > returning false when an empty node is encountered. In this context,
> > > it's also not about zones, it really is nr_zonerefs. Rename nr_zones in
> > > build_zonelists_node as well.
> > 
> > hmm, why don't we rather make it zonerefs based then. Something
> > like the following?
> 
> Works for me.

Should I fold it to the patch or make it a patch on its own?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1688773

On Mon, Jul 17, 2017 at 10:19:42AM +0200, Michal Hocko wrote:
> On Mon 17-07-17 09:07:23, Mel Gorman wrote:
> > On Mon, Jul 17, 2017 at 08:06:40AM +0200, Michal Hocko wrote:
> > > On Fri 14-07-17 15:18:23, Mel Gorman wrote:
> > > > Fairly sure that's not what you meant.
> > > > 
> > > > 
> > > > >  		pg_data_t *node = NODE_DATA(node_order[i]);
> > > > >  
> > > > > -		zoneref_idx = build_zonelists_node(node, zonelist, zoneref_idx);
> > > > > +		nr_zones = build_zonelists_node(node, zonelist, nr_zones);
> > > > 
> > > > I meant converting build_zonelists_node and passing in &nr_zones and
> > > > returning false when an empty node is encountered. In this context,
> > > > it's also not about zones, it really is nr_zonerefs. Rename nr_zones in
> > > > build_zonelists_node as well.
> > > 
> > > hmm, why don't we rather make it zonerefs based then. Something
> > > like the following?
> > 
> > Works for me.
> 
> Should I fold it to the patch or make it a patch on its own?

I have no strong feelings either way but if it was folded then the
overall naming should be easier to follow (at least for me).

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1688798

On Mon 17-07-17 09:58:04, Mel Gorman wrote:
> On Mon, Jul 17, 2017 at 10:19:42AM +0200, Michal Hocko wrote:
> > On Mon 17-07-17 09:07:23, Mel Gorman wrote:
> > > On Mon, Jul 17, 2017 at 08:06:40AM +0200, Michal Hocko wrote:
> > > > On Fri 14-07-17 15:18:23, Mel Gorman wrote:
> > > > > Fairly sure that's not what you meant.
> > > > > 
> > > > > 
> > > > > >  		pg_data_t *node = NODE_DATA(node_order[i]);
> > > > > >  
> > > > > > -		zoneref_idx = build_zonelists_node(node, zonelist, zoneref_idx);
> > > > > > +		nr_zones = build_zonelists_node(node, zonelist, nr_zones);
> > > > > 
> > > > > I meant converting build_zonelists_node and passing in &nr_zones and
> > > > > returning false when an empty node is encountered. In this context,
> > > > > it's also not about zones, it really is nr_zonerefs. Rename nr_zones in
> > > > > build_zonelists_node as well.
> > > > 
> > > > hmm, why don't we rather make it zonerefs based then. Something
> > > > like the following?
> > > 
> > > Works for me.
> > 
> > Should I fold it to the patch or make it a patch on its own?
> 
> I have no strong feelings either way but if it was folded then the
> overall naming should be easier to follow (at least for me).

OK, I will fold it in then. Unless there are more issues/feedback to
address I will repost the full series in few days.

Thanks!
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web