| Message ID | 20180717131837.18411-1-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue 17-07-18 21:18:37, Baoquan He wrote: > We can still use 'kernelcore=mirror' or 'movable_node' for the usage > of hotplug and movable zone. If somebody shows up with a valid usecase > we can reconsider. Well this doesn't really explain why to deprecate this functionality. It is a rather ugly hack that has been originally introduced for large order allocations. But we do have compaction these days. Even though the compaction cannot solve all the fragmentation issues the zone movable is not a great answer as it introduces other issues (basically highmem kind of issues we used to have on 32b systems). The current code doesn't work with KASLR and the code is too subtle to work properly in other cases as well. E.g. movablecore range might cover already used memory (e.g. bootmem allocations) and therefore it doesn't comply with the basic assumption that the memory is movable and that confuses memory hotplug (e.g. 15c30bc09085 ("mm, memory_hotplug: make has_unmovable_pages more robust"). There are probably other issues I am not aware of but primarily the code adds a maintenance burden which would be better to get rid of. I would also go further and remove all the code the feature is using at one go. If somebody really needs this functionality we would need to revert the whole thing anyway. > Suggested-by: Michal Hocko <mhocko@kernel.org> > Signed-off-by: Baoquan He <bhe@redhat.com> > --- > Documentation/admin-guide/kernel-parameters.txt | 2 ++ > mm/page_alloc.c | 3 +++ > 2 files changed, 5 insertions(+) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index efc7aa7a0670..1e22c49866a2 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -1855,6 +1855,7 @@ > keepinitrd [HW,ARM] > > kernelcore= [KNL,X86,IA-64,PPC] > + [Usage of kernelcore=nn[KMGTPE] | nn% is deprecated] > Format: nn[KMGTPE] | nn% | "mirror" > This parameter specifies the amount of memory usable by > the kernel for non-movable allocations. The requested > @@ -2395,6 +2396,7 @@ > reporting absolute coordinates, such as tablets > > movablecore= [KNL,X86,IA-64,PPC] > + [Deprecated] > Format: nn[KMGTPE] | nn% > This parameter is the complement to kernelcore=, it > specifies the amount of memory used for migratable > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 1521100f1e63..86cf05f48b5f 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -6899,6 +6899,8 @@ static int __init cmdline_parse_kernelcore(char *p) > return 0; > } > > + pr_warn("Only kernelcore=mirror supported, " > + "usage of kernelcore=nn[KMGTPE]|nn%% is deprecated.\n"); > return cmdline_parse_core(p, &required_kernelcore, > &required_kernelcore_percent); > } > @@ -6909,6 +6911,7 @@ static int __init cmdline_parse_kernelcore(char *p) > */ > static int __init cmdline_parse_movablecore(char *p) > { > + pr_warn("Option movablecore= is deprecated.\n"); > return cmdline_parse_core(p, &required_movablecore, > &required_movablecore_percent); > } > -- > 2.13.6
Hi Michal, On 07/17/18 at 03:31pm, Michal Hocko wrote: > On Tue 17-07-18 21:18:37, Baoquan He wrote: > > We can still use 'kernelcore=mirror' or 'movable_node' for the usage > > of hotplug and movable zone. If somebody shows up with a valid usecase > > we can reconsider. > > Well this doesn't really explain why to deprecate this functionality. > It is a rather ugly hack that has been originally introduced for large > order allocations. But we do have compaction these days. Even though the > compaction cannot solve all the fragmentation issues the zone movable is > not a great answer as it introduces other issues (basically highmem kind > of issues we used to have on 32b systems). > The current code doesn't work with KASLR and the code is too subtle to > work properly in other cases as well. E.g. movablecore range might cover > already used memory (e.g. bootmem allocations) and therefore it doesn't > comply with the basic assumption that the memory is movable and that > confuses memory hotplug (e.g. 15c30bc09085 ("mm, memory_hotplug: make > has_unmovable_pages more robust"). > > There are probably other issues I am not aware of but primarily the code > adds a maintenance burden which would be better to get rid of. > > I would also go further and remove all the code the feature is using at > one go. If somebody really needs this functionality we would need to > revert the whole thing anyway. Thanks for these details. I can arrange your above saying and rewrite patch log. Are you suggesting removing the code "kernelcore=nn" and "movablecore=" are using? If yes, I can repost with these changes. Just saw some deprecated codes are still there for future cleaning up. So posted this v1 patch. Thanks Baoquan > > --- > > Documentation/admin-guide/kernel-parameters.txt | 2 ++ > > mm/page_alloc.c | 3 +++ > > 2 files changed, 5 insertions(+) > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > index efc7aa7a0670..1e22c49866a2 100644 > > --- a/Documentation/admin-guide/kernel-parameters.txt > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > @@ -1855,6 +1855,7 @@ > > keepinitrd [HW,ARM] > > > > kernelcore= [KNL,X86,IA-64,PPC] > > + [Usage of kernelcore=nn[KMGTPE] | nn% is deprecated] > > Format: nn[KMGTPE] | nn% | "mirror" > > This parameter specifies the amount of memory usable by > > the kernel for non-movable allocations. The requested > > @@ -2395,6 +2396,7 @@ > > reporting absolute coordinates, such as tablets > > > > movablecore= [KNL,X86,IA-64,PPC] > > + [Deprecated] > > Format: nn[KMGTPE] | nn% > > This parameter is the complement to kernelcore=, it > > specifies the amount of memory used for migratable > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index 1521100f1e63..86cf05f48b5f 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -6899,6 +6899,8 @@ static int __init cmdline_parse_kernelcore(char *p) > > return 0; > > } > > > > + pr_warn("Only kernelcore=mirror supported, " > > + "usage of kernelcore=nn[KMGTPE]|nn%% is deprecated.\n"); > > return cmdline_parse_core(p, &required_kernelcore, > > &required_kernelcore_percent); > > } > > @@ -6909,6 +6911,7 @@ static int __init cmdline_parse_kernelcore(char *p) > > */ > > static int __init cmdline_parse_movablecore(char *p) > > { > > + pr_warn("Option movablecore= is deprecated.\n"); > > return cmdline_parse_core(p, &required_movablecore, > > &required_movablecore_percent); > > } > > -- > > 2.13.6 > > -- > Michal Hocko > SUSE Labs
On Tue 17-07-18 22:24:43, Baoquan He wrote: > Hi Michal, > > On 07/17/18 at 03:31pm, Michal Hocko wrote: > > On Tue 17-07-18 21:18:37, Baoquan He wrote: > > > We can still use 'kernelcore=mirror' or 'movable_node' for the usage > > > of hotplug and movable zone. If somebody shows up with a valid usecase > > > we can reconsider. > > > > Well this doesn't really explain why to deprecate this functionality. > > It is a rather ugly hack that has been originally introduced for large > > order allocations. But we do have compaction these days. Even though the > > compaction cannot solve all the fragmentation issues the zone movable is > > not a great answer as it introduces other issues (basically highmem kind > > of issues we used to have on 32b systems). > > The current code doesn't work with KASLR and the code is too subtle to > > work properly in other cases as well. E.g. movablecore range might cover > > already used memory (e.g. bootmem allocations) and therefore it doesn't > > comply with the basic assumption that the memory is movable and that > > confuses memory hotplug (e.g. 15c30bc09085 ("mm, memory_hotplug: make > > has_unmovable_pages more robust"). > > > > There are probably other issues I am not aware of but primarily the code > > adds a maintenance burden which would be better to get rid of. > > > > I would also go further and remove all the code the feature is using at > > one go. If somebody really needs this functionality we would need to > > revert the whole thing anyway. > > Thanks for these details. I can arrange your above saying and rewrite > patch log. Are you suggesting removing the code "kernelcore=nn" and > "movablecore=" are using? If yes, I can repost with these changes. Yes.
On Tue, 17 Jul 2018, Baoquan He wrote: > We can still use 'kernelcore=mirror' or 'movable_node' for the usage > of hotplug and movable zone. If somebody shows up with a valid usecase > we can reconsider. > We actively use kernelcore=n%, I had recently added support for the option in the first place in 4.17. It's certainly not deprecated. commit a5c6d6509342785bef53bf9508e1842b303f1878 Author: David Rientjes <rientjes@google.com> Date: Thu Apr 5 16:23:09 2018 -0700 mm, page_alloc: extend kernelcore and movablecore for percent
On 07/17/18 at 01:46pm, David Rientjes wrote: > On Tue, 17 Jul 2018, Baoquan He wrote: > > > We can still use 'kernelcore=mirror' or 'movable_node' for the usage > > of hotplug and movable zone. If somebody shows up with a valid usecase > > we can reconsider. > > > > We actively use kernelcore=n%, I had recently added support for the option > in the first place in 4.17. It's certainly not deprecated. Thanks for telling. Just for curiosity, could you tell the scenario you are using kernelcore=n%? Since it evenly spread movable area on nodes, we may not be able to physically hot unplug/plug RAM. Thanks Baoquan
On Tue 17-07-18 13:46:43, David Rientjes wrote: > On Tue, 17 Jul 2018, Baoquan He wrote: > > > We can still use 'kernelcore=mirror' or 'movable_node' for the usage > > of hotplug and movable zone. If somebody shows up with a valid usecase > > we can reconsider. > > > > We actively use kernelcore=n%, I had recently added support for the option > in the first place in 4.17. It's certainly not deprecated. > > commit a5c6d6509342785bef53bf9508e1842b303f1878 > Author: David Rientjes <rientjes@google.com> > Date: Thu Apr 5 16:23:09 2018 -0700 > > mm, page_alloc: extend kernelcore and movablecore for percent What kind of functionality do you need to not depend on this knob? I mean it is a gross hack and you are basically working around fragmentation issues. ZONE_MOVABLE doesn't seem to be a long term solution here IMHO.
On Wed, 18 Jul 2018, Baoquan He wrote: > > > We can still use 'kernelcore=mirror' or 'movable_node' for the usage > > > of hotplug and movable zone. If somebody shows up with a valid usecase > > > we can reconsider. > > > > > > > We actively use kernelcore=n%, I had recently added support for the option > > in the first place in 4.17. It's certainly not deprecated. > > Thanks for telling. Just for curiosity, could you tell the scenario you > are using kernelcore=n%? Since it evenly spread movable area on nodes, > we may not be able to physically hot unplug/plug RAM. > To evenly distribute ZONE_MOVABLE over a set of nodes regardless of the many memory capacities of systems that we have where individual command lines cannot be tuned. But you want to deprecate kernelcore=nn, not just the percent version, so I assume that's not the answer you're looking for. We do not enable CONFIG_MEMORY_HOTPLUG so this deprecation would break our userspace such that we cannot use ZONE_MOVABLE.
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index efc7aa7a0670..1e22c49866a2 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -1855,6 +1855,7 @@ keepinitrd [HW,ARM] kernelcore= [KNL,X86,IA-64,PPC] + [Usage of kernelcore=nn[KMGTPE] | nn% is deprecated] Format: nn[KMGTPE] | nn% | "mirror" This parameter specifies the amount of memory usable by the kernel for non-movable allocations. The requested @@ -2395,6 +2396,7 @@ reporting absolute coordinates, such as tablets movablecore= [KNL,X86,IA-64,PPC] + [Deprecated] Format: nn[KMGTPE] | nn% This parameter is the complement to kernelcore=, it specifies the amount of memory used for migratable diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 1521100f1e63..86cf05f48b5f 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -6899,6 +6899,8 @@ static int __init cmdline_parse_kernelcore(char *p) return 0; } + pr_warn("Only kernelcore=mirror supported, " + "usage of kernelcore=nn[KMGTPE]|nn%% is deprecated.\n"); return cmdline_parse_core(p, &required_kernelcore, &required_kernelcore_percent); } @@ -6909,6 +6911,7 @@ static int __init cmdline_parse_kernelcore(char *p) */ static int __init cmdline_parse_movablecore(char *p) { + pr_warn("Option movablecore= is deprecated.\n"); return cmdline_parse_core(p, &required_movablecore, &required_movablecore_percent); }
We can still use 'kernelcore=mirror' or 'movable_node' for the usage of hotplug and movable zone. If somebody shows up with a valid usecase we can reconsider. Suggested-by: Michal Hocko <mhocko@kernel.org> Signed-off-by: Baoquan He <bhe@redhat.com> --- Documentation/admin-guide/kernel-parameters.txt | 2 ++ mm/page_alloc.c | 3 +++ 2 files changed, 5 insertions(+)