Re: [PATCH 1/2] mm, memory_hotplug: display allowed zones in the preferred ordering

[Vlastimil Babka]

On 06/29/2017 09:35 AM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@xxxxxxxx>
> 
> Prior to "mm, memory_hotplug: do not associate hotadded memory to zones
> until online" we used to allow to change the valid zone types of a
> memory block if it is adjacent to a different zone type. This fact was
> reflected in memoryNN/valid_zones by the ordering of printed zones.
> The first one was default (echo online > memoryNN/state) and the other
> one could be onlined explicitly by online_{movable,kernel}. This
> behavior was removed by the said patch and as such the ordering was
> not all that important. In most cases a kernel zone would be default
> anyway. The only exception is movable_node handled by "mm,
> memory_hotplug: support movable_node for hotpluggable nodes".
> 
> Let's reintroduce this behavior again because later patch will remove
> the zone overlap restriction and so user will be allowed to online
> kernel resp. movable block regardless of its placement. Original
> behavior will then become significant again because it would be
> non-trivial for users to see what is the default zone to online into.
> 
> Implementation is really simple. Pull out zone selection out of
> move_pfn_range into zone_for_pfn_range helper and use it in
> show_valid_zones to display the zone for default onlining and then
> both kernel and movable if they are allowed. Default online zone is not
> duplicated.

Hm I wouldn't call this maze of functions simple, but seems to be correct.
Maybe Patch 2/2 will simplify the code...

> Signed-off-by: Michal Hocko <mhocko@xxxxxxxx>

Acked-by: Vlastimil Babka <vbabka@xxxxxxx>

> 
> fold me "mm, memory_hotplug: display allowed zones in the preferred ordering"
> ---
>  drivers/base/memory.c          | 33 +++++++++++++------
>  include/linux/memory_hotplug.h |  2 +-
>  mm/memory_hotplug.c            | 73 ++++++++++++++++++++++++------------------
>  3 files changed, 65 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index c7c4e0325cdb..26383af9900c 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -388,6 +388,22 @@ static ssize_t show_phys_device(struct device *dev,
>  }
>  
>  #ifdef CONFIG_MEMORY_HOTREMOVE
> +static void print_allowed_zone(char *buf, int nid, unsigned long start_pfn,
> +		unsigned long nr_pages, int online_type,
> +		struct zone *default_zone)
> +{
> +	struct zone *zone;
> +
> +	if (!allow_online_pfn_range(nid, start_pfn, nr_pages, online_type))
> +		return;
> +
> +	zone = zone_for_pfn_range(online_type, nid, start_pfn, nr_pages);
> +	if (zone != default_zone) {
> +		strcat(buf, " ");
> +		strcat(buf, zone->name);
> +	}
> +}
> +
>  static ssize_t show_valid_zones(struct device *dev,
>  				struct device_attribute *attr, char *buf)
>  {
> @@ -395,7 +411,7 @@ static ssize_t show_valid_zones(struct device *dev,
>  	unsigned long start_pfn = section_nr_to_pfn(mem->start_section_nr);
>  	unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
>  	unsigned long valid_start_pfn, valid_end_pfn;
> -	bool append = false;
> +	struct zone *default_zone;
>  	int nid;
>  
>  	/*
> @@ -418,16 +434,13 @@ static ssize_t show_valid_zones(struct device *dev,
>  	}
>  
>  	nid = pfn_to_nid(start_pfn);
> -	if (allow_online_pfn_range(nid, start_pfn, nr_pages, MMOP_ONLINE_KERNEL)) {
> -		strcat(buf, default_zone_for_pfn(nid, start_pfn, nr_pages)->name);
> -		append = true;
> -	}
> +	default_zone = zone_for_pfn_range(MMOP_ONLINE_KEEP, nid, start_pfn, nr_pages);
> +	strcat(buf, default_zone->name);
>  
> -	if (allow_online_pfn_range(nid, start_pfn, nr_pages, MMOP_ONLINE_MOVABLE)) {
> -		if (append)
> -			strcat(buf, " ");
> -		strcat(buf, NODE_DATA(nid)->node_zones[ZONE_MOVABLE].name);
> -	}
> +	print_allowed_zone(buf, nid, start_pfn, nr_pages, MMOP_ONLINE_KERNEL,
> +			default_zone);
> +	print_allowed_zone(buf, nid, start_pfn, nr_pages, MMOP_ONLINE_MOVABLE,
> +			default_zone);
>  out:
>  	strcat(buf, "\n");
>  
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index c8a5056a5ae0..5e6e4cc36ff4 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -319,6 +319,6 @@ extern struct page *sparse_decode_mem_map(unsigned long coded_mem_map,
>  					  unsigned long pnum);
>  extern bool allow_online_pfn_range(int nid, unsigned long pfn, unsigned long nr_pages,
>  		int online_type);
> -extern struct zone *default_zone_for_pfn(int nid, unsigned long pfn,
> +extern struct zone *zone_for_pfn_range(int online_type, int nid, unsigned start_pfn,
>  		unsigned long nr_pages);
>  #endif /* __LINUX_MEMORY_HOTPLUG_H */
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index b4015a39d108..6b9a60115e37 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -836,31 +836,6 @@ static void node_states_set_node(int node, struct memory_notify *arg)
>  	node_set_state(node, N_MEMORY);
>  }
>  
> -bool allow_online_pfn_range(int nid, unsigned long pfn, unsigned long nr_pages, int online_type)
> -{
> -	struct pglist_data *pgdat = NODE_DATA(nid);
> -	struct zone *movable_zone = &pgdat->node_zones[ZONE_MOVABLE];
> -	struct zone *default_zone = default_zone_for_pfn(nid, pfn, nr_pages);
> -
> -	/*
> -	 * TODO there shouldn't be any inherent reason to have ZONE_NORMAL
> -	 * physically before ZONE_MOVABLE. All we need is they do not
> -	 * overlap. Historically we didn't allow ZONE_NORMAL after ZONE_MOVABLE
> -	 * though so let's stick with it for simplicity for now.
> -	 * TODO make sure we do not overlap with ZONE_DEVICE
> -	 */
> -	if (online_type == MMOP_ONLINE_KERNEL) {
> -		if (zone_is_empty(movable_zone))
> -			return true;
> -		return movable_zone->zone_start_pfn >= pfn + nr_pages;
> -	} else if (online_type == MMOP_ONLINE_MOVABLE) {
> -		return zone_end_pfn(default_zone) <= pfn;
> -	}
> -
> -	/* MMOP_ONLINE_KEEP will always succeed and inherits the current zone */
> -	return online_type == MMOP_ONLINE_KEEP;
> -}
> -
>  static void __meminit resize_zone_range(struct zone *zone, unsigned long start_pfn,
>  		unsigned long nr_pages)
>  {
> @@ -919,7 +894,7 @@ void __ref move_pfn_range_to_zone(struct zone *zone,
>   * If no kernel zone covers this pfn range it will automatically go
>   * to the ZONE_NORMAL.
>   */
> -struct zone *default_zone_for_pfn(int nid, unsigned long start_pfn,
> +static struct zone *default_zone_for_pfn(int nid, unsigned long start_pfn,
>  		unsigned long nr_pages)
>  {
>  	struct pglist_data *pgdat = NODE_DATA(nid);
> @@ -935,6 +910,31 @@ struct zone *default_zone_for_pfn(int nid, unsigned long start_pfn,
>  	return &pgdat->node_zones[ZONE_NORMAL];
>  }
>  
> +bool allow_online_pfn_range(int nid, unsigned long pfn, unsigned long nr_pages, int online_type)
> +{
> +	struct pglist_data *pgdat = NODE_DATA(nid);
> +	struct zone *movable_zone = &pgdat->node_zones[ZONE_MOVABLE];
> +	struct zone *default_zone = default_zone_for_pfn(nid, pfn, nr_pages);
> +
> +	/*
> +	 * TODO there shouldn't be any inherent reason to have ZONE_NORMAL
> +	 * physically before ZONE_MOVABLE. All we need is they do not
> +	 * overlap. Historically we didn't allow ZONE_NORMAL after ZONE_MOVABLE
> +	 * though so let's stick with it for simplicity for now.
> +	 * TODO make sure we do not overlap with ZONE_DEVICE
> +	 */
> +	if (online_type == MMOP_ONLINE_KERNEL) {
> +		if (zone_is_empty(movable_zone))
> +			return true;
> +		return movable_zone->zone_start_pfn >= pfn + nr_pages;
> +	} else if (online_type == MMOP_ONLINE_MOVABLE) {
> +		return zone_end_pfn(default_zone) <= pfn;
> +	}
> +
> +	/* MMOP_ONLINE_KEEP will always succeed and inherits the current zone */
> +	return online_type == MMOP_ONLINE_KEEP;
> +}
> +
>  static inline bool movable_pfn_range(int nid, struct zone *default_zone,
>  		unsigned long start_pfn, unsigned long nr_pages)
>  {
> @@ -948,12 +948,8 @@ static inline bool movable_pfn_range(int nid, struct zone *default_zone,
>  	return !zone_intersects(default_zone, start_pfn, nr_pages);
>  }
>  
> -/*
> - * Associates the given pfn range with the given node and the zone appropriate
> - * for the given online type.
> - */
> -static struct zone * __meminit move_pfn_range(int online_type, int nid,
> -		unsigned long start_pfn, unsigned long nr_pages)
> +struct zone * zone_for_pfn_range(int online_type, int nid, unsigned start_pfn,
> +		unsigned long nr_pages)
>  {
>  	struct pglist_data *pgdat = NODE_DATA(nid);
>  	struct zone *zone = default_zone_for_pfn(nid, start_pfn, nr_pages);
> @@ -972,6 +968,19 @@ static struct zone * __meminit move_pfn_range(int online_type, int nid,
>  		zone = &pgdat->node_zones[ZONE_MOVABLE];
>  	}
>  
> +	return zone;
> +}
> +
> +/*
> + * Associates the given pfn range with the given node and the zone appropriate
> + * for the given online type.
> + */
> +static struct zone * __meminit move_pfn_range(int online_type, int nid,
> +		unsigned long start_pfn, unsigned long nr_pages)
> +{
> +	struct zone *zone;
> +
> +	zone = zone_for_pfn_range(online_type, nid, start_pfn, nr_pages);
>  	move_pfn_range_to_zone(zone, start_pfn, nr_pages);
>  	return zone;
>  }
>