| Message ID | 20200308013511.12792-1-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mm: Remove the redundant conditional check | expand |
On Sun 08-03-20 09:35:11, Baoquan He wrote: > In commit f70029bbaacbfa8f0 ("mm, memory_hotplug: drop CONFIG_MOVABLE_NODE"), > the dependency on CONFIG_MOVABLE_NODE was removed for N_MEMORY, so the > conditional check in paging_init() doesn't make any sense any more. > Remove it. Please expand more. I would really have to refresh the intention of the code but from a quick look at the code CONFIG_HIGHMEM still makes N_MEMORY != N_NORMAL_MEMORY. So what what does this change mean for that config? > Signed-off-by: Baoquan He <bhe@redhat.com> > --- > arch/x86/mm/init_64.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c > index abbdecb75fad..0a14711d3a93 100644 > --- a/arch/x86/mm/init_64.c > +++ b/arch/x86/mm/init_64.c > @@ -818,8 +818,7 @@ void __init paging_init(void) > * will not set it back. > */ > node_clear_state(0, N_MEMORY); > - if (N_MEMORY != N_NORMAL_MEMORY) > - node_clear_state(0, N_NORMAL_MEMORY); > + node_clear_state(0, N_NORMAL_MEMORY); > > zone_sizes_init(); > } > -- > 2.17.2 >
On 03/10/20 at 11:10am, Michal Hocko wrote: > On Sun 08-03-20 09:35:11, Baoquan He wrote: > > In commit f70029bbaacbfa8f0 ("mm, memory_hotplug: drop CONFIG_MOVABLE_NODE"), > > the dependency on CONFIG_MOVABLE_NODE was removed for N_MEMORY, so the > > conditional check in paging_init() doesn't make any sense any more. > > Remove it. > > Please expand more. I would really have to refresh the intention of the > code but from a quick look at the code CONFIG_HIGHMEM still makes > N_MEMORY != N_NORMAL_MEMORY. So what what does this change mean for that > config? Thanks for looking into this. I was trying to explain that CONFIG_MOVABLE_NODE made N_MEMORY have chance to take different enum value. Do you think the below saying is OK to you? ~~~ In commit f70029bbaacb ("mm, memory_hotplug: drop CONFIG_MOVABLE_NODE"), the dependency on CONFIG_MOVABLE_NODE was removed for N_MEMORY. Before commit f70029bbaacb, CONFIG_HIGHMEM && !CONFIG_MOVABLE_NODE could make (N_MEMORY == N_NORMAL_MEMORY) be true. After commit f70029bbaacb, N_MEMORY doesn't have any chance to be equal to N_NORMAL_MEMORY. So the conditional check in paging_init() doesn't make any sense any more. Let's remove it. ~~~ Thanks Baoquan
On Tue 10-03-20 22:23:41, Baoquan He wrote: > On 03/10/20 at 11:10am, Michal Hocko wrote: > > On Sun 08-03-20 09:35:11, Baoquan He wrote: > > > In commit f70029bbaacbfa8f0 ("mm, memory_hotplug: drop CONFIG_MOVABLE_NODE"), > > > the dependency on CONFIG_MOVABLE_NODE was removed for N_MEMORY, so the > > > conditional check in paging_init() doesn't make any sense any more. > > > Remove it. > > > > Please expand more. I would really have to refresh the intention of the > > code but from a quick look at the code CONFIG_HIGHMEM still makes > > N_MEMORY != N_NORMAL_MEMORY. So what what does this change mean for that > > config? > > Thanks for looking into this. I was trying to explain that > CONFIG_MOVABLE_NODE made N_MEMORY have chance to take different enum > value. > > Do you think the below saying is OK to you? > > ~~~ > In commit f70029bbaacb ("mm, memory_hotplug: drop CONFIG_MOVABLE_NODE"), > the dependency on CONFIG_MOVABLE_NODE was removed for N_MEMORY. Before > commit f70029bbaacb, CONFIG_HIGHMEM && !CONFIG_MOVABLE_NODE could make > (N_MEMORY == N_NORMAL_MEMORY) be true. After commit f70029bbaacb, N_MEMORY > doesn't have any chance to be equal to N_NORMAL_MEMORY. So the conditional > check in paging_init() doesn't make any sense any more. Let's remove it. Yes this describes the matter much better. I have obviously misread the code when looking at it this morning. Being explicit in the changelog would have helped at least me. Thanks!
On 03/10/20 at 03:32pm, Michal Hocko wrote: > On Tue 10-03-20 22:23:41, Baoquan He wrote: > > On 03/10/20 at 11:10am, Michal Hocko wrote: > > > On Sun 08-03-20 09:35:11, Baoquan He wrote: > > > > In commit f70029bbaacbfa8f0 ("mm, memory_hotplug: drop CONFIG_MOVABLE_NODE"), > > > > the dependency on CONFIG_MOVABLE_NODE was removed for N_MEMORY, so the > > > > conditional check in paging_init() doesn't make any sense any more. > > > > Remove it. > > > > > > Please expand more. I would really have to refresh the intention of the > > > code but from a quick look at the code CONFIG_HIGHMEM still makes > > > N_MEMORY != N_NORMAL_MEMORY. So what what does this change mean for that > > > config? > > > > Thanks for looking into this. I was trying to explain that > > CONFIG_MOVABLE_NODE made N_MEMORY have chance to take different enum > > value. > > > > Do you think the below saying is OK to you? > > > > ~~~ > > In commit f70029bbaacb ("mm, memory_hotplug: drop CONFIG_MOVABLE_NODE"), > > the dependency on CONFIG_MOVABLE_NODE was removed for N_MEMORY. Before > > commit f70029bbaacb, CONFIG_HIGHMEM && !CONFIG_MOVABLE_NODE could make > > (N_MEMORY == N_NORMAL_MEMORY) be true. After commit f70029bbaacb, N_MEMORY > > doesn't have any chance to be equal to N_NORMAL_MEMORY. So the conditional > > check in paging_init() doesn't make any sense any more. Let's remove it. > > Yes this describes the matter much better. I have obviously misread the > code when looking at it this morning. Being explicit in the changelog > would have helped at least me. Thanks! Will update log and repost, thanks.
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c index abbdecb75fad..0a14711d3a93 100644 --- a/arch/x86/mm/init_64.c +++ b/arch/x86/mm/init_64.c @@ -818,8 +818,7 @@ void __init paging_init(void) * will not set it back. */ node_clear_state(0, N_MEMORY); - if (N_MEMORY != N_NORMAL_MEMORY) - node_clear_state(0, N_NORMAL_MEMORY); + node_clear_state(0, N_NORMAL_MEMORY); zone_sizes_init(); }
In commit f70029bbaacbfa8f0 ("mm, memory_hotplug: drop CONFIG_MOVABLE_NODE"), the dependency on CONFIG_MOVABLE_NODE was removed for N_MEMORY, so the conditional check in paging_init() doesn't make any sense any more. Remove it. Signed-off-by: Baoquan He <bhe@redhat.com> --- arch/x86/mm/init_64.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)