mm/page_alloc.c: leverage compiler to zero out used_mask

PATCH In review Wei Yang 3 participants 6 messages Mar 26, 2020 – Mar 27, 2020

Subsystems (2)

Memory management Page allocator

lore

Share & tools

lore

Wei Yang

author

2020-03-26 22:24 UTC #0 lore raw

PATCH 1/2 mm/page_alloc.c: leverage compiler to zero out used_mask

Since we always clear used_mask before getting node order, we can leverage compiler to do this instead of at run time.

Signed-off-by Wei Yang <[email protected]>

---
 mm/page_alloc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

mm/page_alloc.c +1 -2

index 0e823bca3f2f..2144b6ceb119 100644
@@ -5587,14 +5587,13 @@ static void build_zonelists(pg_data_t *pgdat)
 {
 	static int node_order[MAX_NUMNODES];
 	int node, load, nr_nodes = 0;
-	nodemask_t used_mask;
+	nodemask_t used_mask = {.bits = {0}};
 	int local_node, prev_node;
 
 	/* NUMA-aware ordering of nodes */
 	local_node = pgdat->node_id;
 	load = nr_online_nodes;
 	prev_node = local_node;
-	nodes_clear(used_mask);
 
 	memset(node_order, 0, sizeof(node_order));
 	while ((node = find_next_best_node(local_node, &used_mask)) >= 0) {

Signature · 1 line

2.23.0

Wei Yang

author

2020-03-26 22:24 UTC #1 lore raw

PATCH 2/2 mm/page_alloc.c: define node_order with all zero

Since we always clear node_order before getting it, we can leverage compiler to do this instead of at run time.

Signed-off-by Wei Yang <[email protected]>

---
 mm/page_alloc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

mm/page_alloc.c +1 -2

index 2144b6ceb119..7a6435cffdb7 100644
@@ -5585,7 +5585,7 @@ static void build_thisnode_zonelists(pg_data_t *pgdat)
 
 static void build_zonelists(pg_data_t *pgdat)
 {
-	static int node_order[MAX_NUMNODES];
+	static int node_order[MAX_NUMNODES] = {0};
 	int node, load, nr_nodes = 0;
 	nodemask_t used_mask = {.bits = {0}};
 	int local_node, prev_node;

@@ -5595,7 +5595,6 @@ static void build_zonelists(pg_data_t *pgdat)
 	load = nr_online_nodes;
 	prev_node = local_node;
 
-	memset(node_order, 0, sizeof(node_order));
 	while ((node = find_next_best_node(local_node, &used_mask)) >= 0) {
 		/*
 		 * We don't want to pressure a particular node.

Signature · 1 line

2.23.0

Jason Gunthorpe

2020-03-26 22:36 UTC #2 lore raw

In reply to Wei Yang

Quoted 11 lines · Wei Yang

Since we always clear used_mask before getting node order, we can leverage compiler to do this instead of at run time.
Signed-off-by Wei Yang <[email protected]>
 mm/page_alloc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
mm/page_alloc.c +1 -0
index 0e823bca3f2f..2144b6ceb119 100644
+++ b/mm/page_alloc.c

mm/page_alloc.c static void build_zonelists(pg_data_t *pgdat) · line 5590

Quoted 5 lines

mm/page_alloc.c +1 -1

@@ -5587,14 +5587,13 @@ static void build_zonelists(pg_data_t *pgdat)
 {
 	static int node_order[MAX_NUMNODES];
 	int node, load, nr_nodes = 0;
-	nodemask_t used_mask;
+	nodemask_t used_mask = {.bits = {0}};

If this style is to be done it should just be '= {}';

This case demonstrates why the popular '= {0}' idiom is not such a good idea, as it only works if the first member is an integral type.

Jason

Wei Yang

author

2020-03-27 13:27 UTC #3 lore raw

Quoted 20 lines · Jason Gunthorpe

Quoted 11 lines · Wei Yang
Since we always clear used_mask before getting node order, we can leverage compiler to do this instead of at run time.
Signed-off-by Wei Yang <[email protected]>
 mm/page_alloc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
mm/page_alloc.c +1 -0
index 0e823bca3f2f..2144b6ceb119 100644
+++ b/mm/page_alloc.c
mm/page_alloc.c static void build_zonelists(pg_data_t *pgdat) · line 5590
Quoted 5 lines
mm/page_alloc.c +1 -1
@@ -5587,14 +5587,13 @@ static void build_zonelists(pg_data_t *pgdat)
 {
 	static int node_order[MAX_NUMNODES];
 	int node, load, nr_nodes = 0;
-	nodemask_t used_mask;
+	nodemask_t used_mask = {.bits = {0}};
If this style is to be done it should just be '= {}';

Quoted 3 lines

This case demonstrates why the popular '= {0}' idiom is not such a good idea, as it only works if the first member is an integral type.

Thanks for your comment. I think David found a better solution.

Quoted 1 line

Jason

Signature · 2 lines

Wei Yang
Help you, Help me

David Hildenbrand

2020-03-27 09:32 UTC #4 lore raw

In reply to Wei Yang

Quoted 19 lines · Wei Yang

Since we always clear used_mask before getting node order, we can leverage compiler to do this instead of at run time.
Signed-off-by Wei Yang <[email protected]>
---
 mm/page_alloc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
mm/page_alloc.c +1 -1
index 0e823bca3f2f..2144b6ceb119 100644
@@ -5587,14 +5587,13 @@ static void build_zonelists(pg_data_t *pgdat)
 {
 	static int node_order[MAX_NUMNODES];
 	int node, load, nr_nodes = 0;
-	nodemask_t used_mask;
+	nodemask_t used_mask = {.bits = {0}};

mm/page_alloc.c static void build_zonelists(pg_data_t *pgdat) · line 5599

Quoted 10 lines

mm/page_alloc.c +0 -1

 	int local_node, prev_node;
 
 	/* NUMA-aware ordering of nodes */
 	local_node = pgdat->node_id;
 	load = nr_online_nodes;
 	prev_node = local_node;
-	nodes_clear(used_mask);
 
 	memset(node_order, 0, sizeof(node_order));
 	while ((node = find_next_best_node(local_node, &used_mask)) >= 0) {

t480s: ~/git/linux default_online_type $ git grep "nodemask_t " | grep "="
arch/x86/mm/numa.c:     nodemask_t reserved_nodemask = NODE_MASK_NONE;
arch/x86/mm/numa_emulation.c:   nodemask_t physnode_mask = numa_nodes_parsed;
arch/x86/mm/numa_emulation.c:   nodemask_t physnode_mask = numa_nodes_parsed;
arch/x86/mm/numa_emulation.c:           nodemask_t physnode_mask = numa_nodes_parsed;
drivers/acpi/numa/srat.c:static nodemask_t nodes_found_map = NODE_MASK_NONE;
kernel/irq/affinity.c:  nodemask_t nodemsk = NODE_MASK_NONE;
kernel/sched/fair.c:            nodemask_t max_group = NODE_MASK_NONE;
mm/memory_hotplug.c:    nodemask_t nmask = node_states[N_MEMORY];
mm/mempolicy.c:         nodemask_t mems = cpuset_mems_allowed(current);
mm/mempolicy.c: nodemask_t nodes = NODE_MASK_NONE;
mm/oom_kill.c:  const nodemask_t *mask = oc->nodemask;

mm/page_alloc.c:nodemask_t node_states[NR_NODE_STATES] __read_mostly = {
mm/page_alloc.c: nodemask_t saved_node_state = node_states[N_MEMORY];

Should this be NODE_MASK_NONE?

Signature · 3 lines

Thanks,

David / dhildenb

Wei Yang

author

2020-03-27 13:22 UTC #5 lore raw

Quoted 43 lines · David Hildenbrand

Quoted 19 lines · Wei Yang

Since we always clear used_mask before getting node order, we can leverage compiler to do this instead of at run time.
Signed-off-by Wei Yang <[email protected]>
---
 mm/page_alloc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
mm/page_alloc.c +1 -1
index 0e823bca3f2f..2144b6ceb119 100644
@@ -5587,14 +5587,13 @@ static void build_zonelists(pg_data_t *pgdat)
 {
 	static int node_order[MAX_NUMNODES];
 	int node, load, nr_nodes = 0;
-	nodemask_t used_mask;
+	nodemask_t used_mask = {.bits = {0}};

mm/page_alloc.c static void build_zonelists(pg_data_t *pgdat) · line 5599

Quoted 10 lines

mm/page_alloc.c +0 -1

 	int local_node, prev_node;
 
 	/* NUMA-aware ordering of nodes */
 	local_node = pgdat->node_id;
 	load = nr_online_nodes;
 	prev_node = local_node;
-	nodes_clear(used_mask);
 
 	memset(node_order, 0, sizeof(node_order));
 	while ((node = find_next_best_node(local_node, &used_mask)) >= 0) {

t480s: ~/git/linux default_online_type $ git grep "nodemask_t " | grep "="
arch/x86/mm/numa.c:     nodemask_t reserved_nodemask = NODE_MASK_NONE;
arch/x86/mm/numa_emulation.c:   nodemask_t physnode_mask = numa_nodes_parsed;
arch/x86/mm/numa_emulation.c:   nodemask_t physnode_mask = numa_nodes_parsed;
arch/x86/mm/numa_emulation.c:           nodemask_t physnode_mask = numa_nodes_parsed;
drivers/acpi/numa/srat.c:static nodemask_t nodes_found_map = NODE_MASK_NONE;
kernel/irq/affinity.c:  nodemask_t nodemsk = NODE_MASK_NONE;
kernel/sched/fair.c:            nodemask_t max_group = NODE_MASK_NONE;
mm/memory_hotplug.c:    nodemask_t nmask = node_states[N_MEMORY];
mm/mempolicy.c:         nodemask_t mems = cpuset_mems_allowed(current);
mm/mempolicy.c: nodemask_t nodes = NODE_MASK_NONE;
mm/oom_kill.c:  const nodemask_t *mask = oc->nodemask;

Quoted 4 lines

mm/page_alloc.c:nodemask_t node_states[NR_NODE_STATES] __read_mostly = {
mm/page_alloc.c: nodemask_t saved_node_state = node_states[N_MEMORY];
Should this be NODE_MASK_NONE?

Thanks, this is gcc extension. Learn something new.

I would update this in v2.

Quoted 5 lines

Signature · 4 lines
-- 
Thanks,

David / dhildenb

Signature · 2 lines

Wei Yang
Help you, Help me