| Message ID | 20180828210158.4617-1-osalvador@techadventures.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/page_alloc: Clean up check_for_memory | expand |
On Tue, 28 Aug 2018 23:01:58 +0200 Oscar Salvador <osalvador@techadventures.net> wrote: > From: Oscar Salvador <osalvador@suse.de> > > check_for_memory looks a bit confusing. > First of all, we have this: > > if (N_MEMORY == N_NORMAL_MEMORY) > return; > > Checking the ENUM declaration, looks like N_MEMORY canot be equal to > N_NORMAL_MEMORY. > I could not find where N_MEMORY is set to N_NORMAL_MEMORY, or the other > way around either, so unless I am missing something, this condition > will never evaluate to true. > It makes sense to get rid of it. Added by commit 4b0ef1fe8a626f0ba7f649764f979d0dc9eab86b Author: Lai Jiangshan <laijs@cn.fujitsu.com> AuthorDate: Wed Dec 12 13:51:46 2012 -0800 Commit: Linus Torvalds <torvalds@linux-foundation.org> CommitDate: Wed Dec 12 17:38:33 2012 -0800 page_alloc: use N_MEMORY instead N_HIGH_MEMORY change the node_states initia lization Let's cc Lai Jiangshan, see if he can remmeber the reasoning. But yes, it does look like im-not-sure-whats-going-on-here defensiveness. > Moving forward, the operations whithin the loop look a bit confusing > as well. > > We set N_HIGH_MEMORY unconditionally, and then we set N_NORMAL_MEMORY > in case we have CONFIG_HIGHMEM (N_NORMAL_MEMORY != N_HIGH_MEMORY) > and zone <= ZONE_NORMAL. > (N_HIGH_MEMORY falls back to N_NORMAL_MEMORY on !CONFIG_HIGHMEM systems, > and that is why we can just go ahead and set N_HIGH_MEMORY unconditionally) > > Although this works, it is a bit subtle. > > I think that this could be easier to follow: > > First, we should only set N_HIGH_MEMORY in case we have > CONFIG_HIGHMEM. Why? Just a teeny optimization? > And then we should set N_NORMAL_MEMORY in case zone <= ZONE_NORMAL, > without further checking whether we have CONFIG_HIGHMEM or not. > > Signed-off-by: Oscar Salvador <osalvador@suse.de> > --- > mm/page_alloc.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 839e0cc17f2c..6aa947f9e614 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -6819,15 +6819,12 @@ static void check_for_memory(pg_data_t *pgdat, int nid) > { > enum zone_type zone_type; > > - if (N_MEMORY == N_NORMAL_MEMORY) > - return; > - > for (zone_type = 0; zone_type <= ZONE_MOVABLE - 1; zone_type++) { > struct zone *zone = &pgdat->node_zones[zone_type]; > if (populated_zone(zone)) { > - node_set_state(nid, N_HIGH_MEMORY); > - if (N_NORMAL_MEMORY != N_HIGH_MEMORY && > - zone_type <= ZONE_NORMAL) > + if (IS_ENABLED(CONFIG_HIGHMEM)) > + node_set_state(nid, N_HIGH_MEMORY); > + if (zone_type <= ZONE_NORMAL) > node_set_state(nid, N_NORMAL_MEMORY); > break; > }
On Tue, Aug 28, 2018 at 02:35:30PM -0700, Andrew Morton wrote: > > First, we should only set N_HIGH_MEMORY in case we have > > CONFIG_HIGHMEM. > > Why? Just a teeny optimization? Hi Andrew, Optimization was not really my point here, my point was to make the code less subtle and more clear. One may wonder why we set N_HIGH_MEMORY unconditionally when __only__ CONFIG_HIGHMEM matters for this case, and why we set N_NORMAL_MEMORY __only__ for CONFIG_HIGHMEM when we should not care about that at all. I do not really expect a big impact here, mainly because check_for_memory is only being used during boot.
On 8/28/18 5:01 PM, Oscar Salvador wrote: > From: Oscar Salvador <osalvador@suse.de> > > check_for_memory looks a bit confusing. > First of all, we have this: > > if (N_MEMORY == N_NORMAL_MEMORY) > return; > > Checking the ENUM declaration, looks like N_MEMORY canot be equal to > N_NORMAL_MEMORY. > I could not find where N_MEMORY is set to N_NORMAL_MEMORY, or the other > way around either, so unless I am missing something, this condition > will never evaluate to true. > It makes sense to get rid of it. > > Moving forward, the operations whithin the loop look a bit confusing > as well. > > We set N_HIGH_MEMORY unconditionally, and then we set N_NORMAL_MEMORY > in case we have CONFIG_HIGHMEM (N_NORMAL_MEMORY != N_HIGH_MEMORY) > and zone <= ZONE_NORMAL. > (N_HIGH_MEMORY falls back to N_NORMAL_MEMORY on !CONFIG_HIGHMEM systems, > and that is why we can just go ahead and set N_HIGH_MEMORY unconditionally) > > Although this works, it is a bit subtle. > > I think that this could be easier to follow: > > First, we should only set N_HIGH_MEMORY in case we have > CONFIG_HIGHMEM. > And then we should set N_NORMAL_MEMORY in case zone <= ZONE_NORMAL, > without further checking whether we have CONFIG_HIGHMEM or not. > > Signed-off-by: Oscar Salvador <osalvador@suse.de> > --- > mm/page_alloc.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 839e0cc17f2c..6aa947f9e614 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -6819,15 +6819,12 @@ static void check_for_memory(pg_data_t *pgdat, int nid) > { > enum zone_type zone_type; > > - if (N_MEMORY == N_NORMAL_MEMORY) > - return; > - > for (zone_type = 0; zone_type <= ZONE_MOVABLE - 1; zone_type++) { > struct zone *zone = &pgdat->node_zones[zone_type]; > if (populated_zone(zone)) { > - node_set_state(nid, N_HIGH_MEMORY); > - if (N_NORMAL_MEMORY != N_HIGH_MEMORY && > - zone_type <= ZONE_NORMAL) > + if (IS_ENABLED(CONFIG_HIGHMEM)) > + node_set_state(nid, N_HIGH_MEMORY); > + if (zone_type <= ZONE_NORMAL) > node_set_state(nid, N_NORMAL_MEMORY); > break; > } > I would re-write the above function like this: static void check_for_memory(pg_data_t *pgdat, int nid) { enum zone_type zone_type; for (zone_type = 0; zone_type < ZONE_MOVABLE; zone_type++) { if (populated_zone(&pgdat->node_zones[zone_type])) { node_set_state(nid, zone_type <= ZONE_NORMAL ? N_NORMAL_MEMORY: N_HIGH_MEMORY); break; } } } zone_type <= ZONE_MOVABLE - 1 is the same as: zone_type < ZONE_MOVABLE If zone > ZONE_NORMAL, it means that CONFIG_HIGHMEM is enabled, no need to check for it. Pavel
On Thu, Aug 30, 2018 at 01:55:29AM +0000, Pasha Tatashin wrote: > I would re-write the above function like this: > static void check_for_memory(pg_data_t *pgdat, int nid) > { > enum zone_type zone_type; > > for (zone_type = 0; zone_type < ZONE_MOVABLE; zone_type++) { > if (populated_zone(&pgdat->node_zones[zone_type])) { > node_set_state(nid, zone_type <= ZONE_NORMAL ? > N_NORMAL_MEMORY: N_HIGH_MEMORY); > break; > } > } > } Hi Pavel, the above would not work fine. You set either N_NORMAL_MEMORY or N_HIGH_MEMORY, but a node can have both types of memory at the same time (on CONFIG_HIGHMEM systems). N_HIGH_MEMORY stands for regular or high memory while N_NORMAL_MEMORY stands only for regular memory, that is why we set it only in case the zone is <= ZONE_NORMAL. > zone_type <= ZONE_MOVABLE - 1 > is the same as: > zone_type < ZONE_MOVABLE This makes sense.
On 8/31/18 8:24 AM, Oscar Salvador wrote: > On Thu, Aug 30, 2018 at 01:55:29AM +0000, Pasha Tatashin wrote: >> I would re-write the above function like this: >> static void check_for_memory(pg_data_t *pgdat, int nid) >> { >> enum zone_type zone_type; >> >> for (zone_type = 0; zone_type < ZONE_MOVABLE; zone_type++) { >> if (populated_zone(&pgdat->node_zones[zone_type])) { >> node_set_state(nid, zone_type <= ZONE_NORMAL ? >> N_NORMAL_MEMORY: N_HIGH_MEMORY); >> break; >> } >> } >> } > > Hi Pavel, > > the above would not work fine. > You set either N_NORMAL_MEMORY or N_HIGH_MEMORY, but a node can have both > types of memory at the same time (on CONFIG_HIGHMEM systems). > > N_HIGH_MEMORY stands for regular or high memory > while N_NORMAL_MEMORY stands only for regular memory, > that is why we set it only in case the zone is <= ZONE_NORMAL. Hi Oscar, Are you saying the code that is in mainline is broken? Because we set node_set_state(nid, N_NORMAL_MEMORY); even on node with N_HIGH_MEMORY: 6826 if (N_NORMAL_MEMORY != N_HIGH_MEMORY && 6827 zone_type <= ZONE_NORMAL) 6828 node_set_state(nid, N_NORMAL_MEMORY); Thank you, Pavel
On Fri, Aug 31, 2018 at 02:04:59PM +0000, Pasha Tatashin wrote: > Are you saying the code that is in mainline is broken? Because we set > node_set_state(nid, N_NORMAL_MEMORY); even on node with N_HIGH_MEMORY: > > 6826 if (N_NORMAL_MEMORY != N_HIGH_MEMORY && > 6827 zone_type <= ZONE_NORMAL) > 6828 node_set_state(nid, N_NORMAL_MEMORY); Yes, and that is fine. Although the curent code is subtle for the reasons I expplained in the changelog. What I am saying is that the code you suggested would not work because your code either sets N_NORMAL_MEMORY or N_HIGH_MEMORY and then breaks the loop. That is wrong because when we are on a CONFIG_HIGHMEM system, it can happen that we have a node with both types, so we have to set both types of memory. N_HIGH_MEMORY, and N_NORMAL_MEMORY if the zone is <= ZONE_NORMAL. Thanks
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 839e0cc17f2c..6aa947f9e614 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -6819,15 +6819,12 @@ static void check_for_memory(pg_data_t *pgdat, int nid) { enum zone_type zone_type; - if (N_MEMORY == N_NORMAL_MEMORY) - return; - for (zone_type = 0; zone_type <= ZONE_MOVABLE - 1; zone_type++) { struct zone *zone = &pgdat->node_zones[zone_type]; if (populated_zone(zone)) { - node_set_state(nid, N_HIGH_MEMORY); - if (N_NORMAL_MEMORY != N_HIGH_MEMORY && - zone_type <= ZONE_NORMAL) + if (IS_ENABLED(CONFIG_HIGHMEM)) + node_set_state(nid, N_HIGH_MEMORY); + if (zone_type <= ZONE_NORMAL) node_set_state(nid, N_NORMAL_MEMORY); break; }