On Tue 18-08-20 09:52:02, Anshuman Khandual wrote:
> Currently a debug message is printed describing the reason for memory range
> offline failure. This just enumerates existing reason codes which improves
> overall readability and makes it cleaner. This does not add any functional
> change.
Wasn't something like that posted already? To be honest I do not think
this is worth the additional LOC. We are talking about few strings used
at a single place. I really do not see any simplification, constants are
sometimes even longer than the strings they are describing.
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: David Hildenbrand <david@xxxxxxxxxx>
> Cc: Michal Hocko <mhocko@xxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: linux-mm@xxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> ---
> This is based on 5.9-rc1
>
> include/linux/memory.h | 28 ++++++++++++++++++++++++++++
> mm/memory_hotplug.c | 18 +++++++++---------
> 2 files changed, 37 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/memory.h b/include/linux/memory.h
> index 439a89e758d8..4b52d706edc1 100644
> --- a/include/linux/memory.h
> +++ b/include/linux/memory.h
> @@ -44,6 +44,34 @@ int set_memory_block_size_order(unsigned int order);
> #define MEM_CANCEL_ONLINE (1<<4)
> #define MEM_CANCEL_OFFLINE (1<<5)
>
> +/*
> + * Memory offline failure reasons
> + */
> +enum offline_failure_reason {
> + OFFLINE_FAILURE_MEMHOLES,
> + OFFLINE_FAILURE_MULTIZONE,
> + OFFLINE_FAILURE_ISOLATE,
> + OFFLINE_FAILURE_NOTIFIER,
> + OFFLINE_FAILURE_SIGNAL,
> + OFFLINE_FAILURE_UNMOVABLE,
> + OFFLINE_FAILURE_DISSOLVE,
> +};
> +
> +static const char *const offline_failure_names[] = {
> + [OFFLINE_FAILURE_MEMHOLES] = "memory holes",
> + [OFFLINE_FAILURE_MULTIZONE] = "multizone range",
> + [OFFLINE_FAILURE_ISOLATE] = "failure to isolate range",
> + [OFFLINE_FAILURE_NOTIFIER] = "notifier failure",
> + [OFFLINE_FAILURE_SIGNAL] = "signal backoff",
> + [OFFLINE_FAILURE_UNMOVABLE] = "unmovable page",
> + [OFFLINE_FAILURE_DISSOLVE] = "failure to dissolve huge pages",
> +};
> +
> +static inline const char *offline_failure(enum offline_failure_reason reason)
> +{
> + return offline_failure_names[reason];
> +}
> +
> struct memory_notify {
> unsigned long start_pfn;
> unsigned long nr_pages;
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index e9d5ab5d3ca0..b3fa36a09d7f 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1484,7 +1484,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
> unsigned long flags;
> struct zone *zone;
> struct memory_notify arg;
> - char *reason;
> + enum offline_failure_reason reason;
>
> mem_hotplug_begin();
>
> @@ -1500,7 +1500,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
> count_system_ram_pages_cb);
> if (nr_pages != end_pfn - start_pfn) {
> ret = -EINVAL;
> - reason = "memory holes";
> + reason = OFFLINE_FAILURE_MEMHOLES;
> goto failed_removal;
> }
>
> @@ -1509,7 +1509,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
> zone = test_pages_in_a_zone(start_pfn, end_pfn);
> if (!zone) {
> ret = -EINVAL;
> - reason = "multizone range";
> + reason = OFFLINE_FAILURE_MULTIZONE;
> goto failed_removal;
> }
> node = zone_to_nid(zone);
> @@ -1519,7 +1519,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
> MIGRATE_MOVABLE,
> MEMORY_OFFLINE | REPORT_FAILURE);
> if (ret < 0) {
> - reason = "failure to isolate range";
> + reason = OFFLINE_FAILURE_ISOLATE;
> goto failed_removal;
> }
> nr_isolate_pageblock = ret;
> @@ -1531,7 +1531,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
> ret = memory_notify(MEM_GOING_OFFLINE, &arg);
> ret = notifier_to_errno(ret);
> if (ret) {
> - reason = "notifier failure";
> + reason = OFFLINE_FAILURE_NOTIFIER;
> goto failed_removal_isolated;
> }
>
> @@ -1540,7 +1540,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
> do {
> if (signal_pending(current)) {
> ret = -EINTR;
> - reason = "signal backoff";
> + reason = OFFLINE_FAILURE_SIGNAL;
> goto failed_removal_isolated;
> }
>
> @@ -1558,7 +1558,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
> } while (!ret);
>
> if (ret != -ENOENT) {
> - reason = "unmovable page";
> + reason = OFFLINE_FAILURE_UNMOVABLE;
> goto failed_removal_isolated;
> }
>
> @@ -1569,7 +1569,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
> */
> ret = dissolve_free_huge_pages(start_pfn, end_pfn);
> if (ret) {
> - reason = "failure to dissolve huge pages";
> + reason = OFFLINE_FAILURE_DISSOLVE;
> goto failed_removal_isolated;
> }
> /* check again */
> @@ -1627,7 +1627,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
> pr_debug("memory offlining [mem %#010llx-%#010llx] failed due to %s\n",
> (unsigned long long) start_pfn << PAGE_SHIFT,
> ((unsigned long long) end_pfn << PAGE_SHIFT) - 1,
> - reason);
> + offline_failure(reason));
> /* pushback to free area */
> mem_hotplug_done();
> return ret;
> --
> 2.20.1
>
--
Michal Hocko
SUSE Labs
