linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/9] rcu: Cleanup RCU tree initialization
@ 2015-05-29  9:53 Alexander Gordeev
  2015-05-29  9:53 ` [PATCH v2 1/9] rcu: Panic if RCU tree can not accommodate all CPUs Alexander Gordeev
                   ` (9 more replies)
  0 siblings, 10 replies; 14+ messages in thread
From: Alexander Gordeev @ 2015-05-29  9:53 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexander Gordeev, Paul E. McKenney, Steven Rostedt

Hello Paul,

This is 2nd attempt to make RCU tree initialization bit more
clear and optimize memory footprint of data associated with
the tree.

Changes since v1:
  - patch 3 fixed to accomodate nr_cpus=1, otherwise rcu_num_lvls
    would yield zero, which is wrong;

The series is against "linux-rcu" tree "rcu/dev" branch

Thanks!

Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>

Alexander Gordeev (9):
  rcu: Panic if RCU tree can not accommodate all CPUs
  rcu: Remove superfluous local variable in rcu_init_geometry()
  rcu: Cleanup rcu_init_geometry() code and arithmetics
  rcu: Simplify rcu_init_geometry() capacity arithmetics
  rcu: Limit rcu_state::levelcnt[] to RCU_NUM_LVLS items
  rcu: Limit rcu_capacity[] size to RCU_NUM_LVLS items
  rcu: Remove unnecessary fields from rcu_state structure
  rcu: Limit count of static data to the number of RCU levels
  rcu: Simplify arithmetic to calculate number of RCU nodes

 kernel/rcu/tree.c        | 99 +++++++++++++++++++++---------------------------
 kernel/rcu/tree.h        | 33 ++++++++--------
 kernel/rcu/tree_plugin.h |  4 +-
 3 files changed, 61 insertions(+), 75 deletions(-)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 1/9] rcu: Panic if RCU tree can not accommodate all CPUs
  2015-05-29  9:53 [PATCH v2 0/9] rcu: Cleanup RCU tree initialization Alexander Gordeev
@ 2015-05-29  9:53 ` Alexander Gordeev
  2015-06-01 18:37   ` Paul E. McKenney
  2015-05-29  9:53 ` [PATCH v2 2/9] rcu: Remove superfluous local variable in rcu_init_geometry() Alexander Gordeev
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 14+ messages in thread
From: Alexander Gordeev @ 2015-05-29  9:53 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexander Gordeev, Paul E. McKenney, Steven Rostedt

Currently a condition when RCU tree is unable to accommodate
the configured number of CPUs is not permitted and causes
a fall back to compile-time values. However, the code has no
means to exceed the RCU tree capacity neither at compile-time
nor in run-time. Therefore, if the condition is met in run-
time then it indicates a serios problem elsewhere and should
be handled with a panic.

Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 kernel/rcu/tree.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 2fce662..66a4230 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -4117,16 +4117,19 @@ static void __init rcu_init_geometry(void)
 		rcu_capacity[i] = rcu_capacity[i - 1] * RCU_FANOUT;
 
 	/*
+	 * The tree must be able to accommodate the configured number of CPUs.
+	 * If this limit is exceeded than we have a serious problem elsewhere.
+	 *
 	 * The boot-time rcu_fanout_leaf parameter is only permitted
 	 * to increase the leaf-level fanout, not decrease it.  Of course,
 	 * the leaf-level fanout cannot exceed the number of bits in
-	 * the rcu_node masks.  Finally, the tree must be able to accommodate
-	 * the configured number of CPUs.  Complain and fall back to the
-	 * compile-time values if these limits are exceeded.
+	 * the rcu_node masks.  Complain and fall back to the compile-
+	 * time values if these limits are exceeded.
 	 */
-	if (rcu_fanout_leaf < RCU_FANOUT_LEAF ||
-	    rcu_fanout_leaf > sizeof(unsigned long) * 8 ||
-	    n > rcu_capacity[MAX_RCU_LVLS]) {
+	if (n > rcu_capacity[MAX_RCU_LVLS])
+		panic("rcu_init_geometry: rcu_capacity[] is too small");
+	else if (rcu_fanout_leaf < RCU_FANOUT_LEAF ||
+		 rcu_fanout_leaf > sizeof(unsigned long) * 8) {
 		WARN_ON(1);
 		return;
 	}
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 2/9] rcu: Remove superfluous local variable in rcu_init_geometry()
  2015-05-29  9:53 [PATCH v2 0/9] rcu: Cleanup RCU tree initialization Alexander Gordeev
  2015-05-29  9:53 ` [PATCH v2 1/9] rcu: Panic if RCU tree can not accommodate all CPUs Alexander Gordeev
@ 2015-05-29  9:53 ` Alexander Gordeev
  2015-05-29  9:53 ` [PATCH v2 3/9] rcu: Cleanup rcu_init_geometry() code and arithmetics Alexander Gordeev
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Alexander Gordeev @ 2015-05-29  9:53 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexander Gordeev, Paul E. McKenney, Steven Rostedt

Local variable 'n' mimics 'nr_cpu_ids' while the both are
used within one function. There is no reason for 'n' to
exist whatsoever.

Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 kernel/rcu/tree.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 66a4230..6a418be 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -4083,7 +4083,6 @@ static void __init rcu_init_geometry(void)
 	ulong d;
 	int i;
 	int j;
-	int n = nr_cpu_ids;
 	int rcu_capacity[MAX_RCU_LVLS + 1];
 
 	/*
@@ -4126,7 +4125,7 @@ static void __init rcu_init_geometry(void)
 	 * the rcu_node masks.  Complain and fall back to the compile-
 	 * time values if these limits are exceeded.
 	 */
-	if (n > rcu_capacity[MAX_RCU_LVLS])
+	if (nr_cpu_ids > rcu_capacity[MAX_RCU_LVLS])
 		panic("rcu_init_geometry: rcu_capacity[] is too small");
 	else if (rcu_fanout_leaf < RCU_FANOUT_LEAF ||
 		 rcu_fanout_leaf > sizeof(unsigned long) * 8) {
@@ -4136,10 +4135,11 @@ static void __init rcu_init_geometry(void)
 
 	/* Calculate the number of rcu_nodes at each level of the tree. */
 	for (i = 1; i <= MAX_RCU_LVLS; i++)
-		if (n <= rcu_capacity[i]) {
-			for (j = 0; j <= i; j++)
-				num_rcu_lvl[j] =
-					DIV_ROUND_UP(n, rcu_capacity[i - j]);
+		if (nr_cpu_ids <= rcu_capacity[i]) {
+			for (j = 0; j <= i; j++) {
+				int cap = rcu_capacity[i - j];
+				num_rcu_lvl[j] = DIV_ROUND_UP(nr_cpu_ids, cap);
+			}
 			rcu_num_lvls = i;
 			for (j = i + 1; j <= MAX_RCU_LVLS; j++)
 				num_rcu_lvl[j] = 0;
@@ -4150,7 +4150,7 @@ static void __init rcu_init_geometry(void)
 	rcu_num_nodes = 0;
 	for (i = 0; i <= MAX_RCU_LVLS; i++)
 		rcu_num_nodes += num_rcu_lvl[i];
-	rcu_num_nodes -= n;
+	rcu_num_nodes -= nr_cpu_ids;
 }
 
 /*
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 3/9] rcu: Cleanup rcu_init_geometry() code and arithmetics
  2015-05-29  9:53 [PATCH v2 0/9] rcu: Cleanup RCU tree initialization Alexander Gordeev
  2015-05-29  9:53 ` [PATCH v2 1/9] rcu: Panic if RCU tree can not accommodate all CPUs Alexander Gordeev
  2015-05-29  9:53 ` [PATCH v2 2/9] rcu: Remove superfluous local variable in rcu_init_geometry() Alexander Gordeev
@ 2015-05-29  9:53 ` Alexander Gordeev
  2015-05-29  9:53 ` [PATCH v2 4/9] rcu: Simplify rcu_init_geometry() capacity arithmetics Alexander Gordeev
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Alexander Gordeev @ 2015-05-29  9:53 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexander Gordeev, Paul E. McKenney, Steven Rostedt

This update simplifies rcu_init_geometry() code flow
and makes calculation of the total number of rcu_node
structures more easy to read.

The update relies on the fact num_rcu_lvl[] is never
accessed beyond rcu_num_lvls index by the rest of the
code. Therefore, there is no need initialize the whole
num_rcu_lvl[].

Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 kernel/rcu/tree.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 6a418be..0de0a34 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -4082,7 +4082,6 @@ static void __init rcu_init_geometry(void)
 {
 	ulong d;
 	int i;
-	int j;
 	int rcu_capacity[MAX_RCU_LVLS + 1];
 
 	/*
@@ -4133,24 +4132,21 @@ static void __init rcu_init_geometry(void)
 		return;
 	}
 
+	/* Calculate the number of levels in the tree. */
+	for (i = 1; nr_cpu_ids > rcu_capacity[i]; i++) {
+	}
+	rcu_num_lvls = i;
+
 	/* Calculate the number of rcu_nodes at each level of the tree. */
-	for (i = 1; i <= MAX_RCU_LVLS; i++)
-		if (nr_cpu_ids <= rcu_capacity[i]) {
-			for (j = 0; j <= i; j++) {
-				int cap = rcu_capacity[i - j];
-				num_rcu_lvl[j] = DIV_ROUND_UP(nr_cpu_ids, cap);
-			}
-			rcu_num_lvls = i;
-			for (j = i + 1; j <= MAX_RCU_LVLS; j++)
-				num_rcu_lvl[j] = 0;
-			break;
-		}
+	for (i = 0; i < rcu_num_lvls; i++) {
+		int cap = rcu_capacity[rcu_num_lvls - i];
+		num_rcu_lvl[i] = DIV_ROUND_UP(nr_cpu_ids, cap);
+	}
 
 	/* Calculate the total number of rcu_node structures. */
 	rcu_num_nodes = 0;
-	for (i = 0; i <= MAX_RCU_LVLS; i++)
+	for (i = 0; i < rcu_num_lvls; i++)
 		rcu_num_nodes += num_rcu_lvl[i];
-	rcu_num_nodes -= nr_cpu_ids;
 }
 
 /*
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 4/9] rcu: Simplify rcu_init_geometry() capacity arithmetics
  2015-05-29  9:53 [PATCH v2 0/9] rcu: Cleanup RCU tree initialization Alexander Gordeev
                   ` (2 preceding siblings ...)
  2015-05-29  9:53 ` [PATCH v2 3/9] rcu: Cleanup rcu_init_geometry() code and arithmetics Alexander Gordeev
@ 2015-05-29  9:53 ` Alexander Gordeev
  2015-05-29  9:53 ` [PATCH v2 5/9] rcu: Limit rcu_state::levelcnt[] to RCU_NUM_LVLS items Alexander Gordeev
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Alexander Gordeev @ 2015-05-29  9:53 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexander Gordeev, Paul E. McKenney, Steven Rostedt

Current code suggests that introducing the extra level to
rcu_capacity[] array makes some of the arithmetic easier.
Well, in fact it appears rather confusing and unnecessary.

Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 kernel/rcu/tree.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 0de0a34..b10e48e 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -4082,7 +4082,7 @@ static void __init rcu_init_geometry(void)
 {
 	ulong d;
 	int i;
-	int rcu_capacity[MAX_RCU_LVLS + 1];
+	int rcu_capacity[MAX_RCU_LVLS];
 
 	/*
 	 * Initialize any unspecified boot parameters.
@@ -4106,12 +4106,10 @@ static void __init rcu_init_geometry(void)
 
 	/*
 	 * Compute number of nodes that can be handled an rcu_node tree
-	 * with the given number of levels.  Setting rcu_capacity[0] makes
-	 * some of the arithmetic easier.
+	 * with the given number of levels.
 	 */
-	rcu_capacity[0] = 1;
-	rcu_capacity[1] = rcu_fanout_leaf;
-	for (i = 2; i <= MAX_RCU_LVLS; i++)
+	rcu_capacity[0] = rcu_fanout_leaf;
+	for (i = 1; i < MAX_RCU_LVLS; i++)
 		rcu_capacity[i] = rcu_capacity[i - 1] * RCU_FANOUT;
 
 	/*
@@ -4124,7 +4122,7 @@ static void __init rcu_init_geometry(void)
 	 * the rcu_node masks.  Complain and fall back to the compile-
 	 * time values if these limits are exceeded.
 	 */
-	if (nr_cpu_ids > rcu_capacity[MAX_RCU_LVLS])
+	if (nr_cpu_ids > rcu_capacity[MAX_RCU_LVLS - 1])
 		panic("rcu_init_geometry: rcu_capacity[] is too small");
 	else if (rcu_fanout_leaf < RCU_FANOUT_LEAF ||
 		 rcu_fanout_leaf > sizeof(unsigned long) * 8) {
@@ -4133,13 +4131,13 @@ static void __init rcu_init_geometry(void)
 	}
 
 	/* Calculate the number of levels in the tree. */
-	for (i = 1; nr_cpu_ids > rcu_capacity[i]; i++) {
+	for (i = 0; nr_cpu_ids > rcu_capacity[i]; i++) {
 	}
-	rcu_num_lvls = i;
+	rcu_num_lvls = i + 1;
 
 	/* Calculate the number of rcu_nodes at each level of the tree. */
 	for (i = 0; i < rcu_num_lvls; i++) {
-		int cap = rcu_capacity[rcu_num_lvls - i];
+		int cap = rcu_capacity[(rcu_num_lvls - 1) - i];
 		num_rcu_lvl[i] = DIV_ROUND_UP(nr_cpu_ids, cap);
 	}
 
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 5/9] rcu: Limit rcu_state::levelcnt[] to RCU_NUM_LVLS items
  2015-05-29  9:53 [PATCH v2 0/9] rcu: Cleanup RCU tree initialization Alexander Gordeev
                   ` (3 preceding siblings ...)
  2015-05-29  9:53 ` [PATCH v2 4/9] rcu: Simplify rcu_init_geometry() capacity arithmetics Alexander Gordeev
@ 2015-05-29  9:53 ` Alexander Gordeev
  2015-05-29  9:53 ` [PATCH v2 6/9] rcu: Limit rcu_capacity[] size " Alexander Gordeev
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Alexander Gordeev @ 2015-05-29  9:53 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexander Gordeev, Paul E. McKenney, Steven Rostedt

Variable rcu_num_lvls is limited by RCU_NUM_LVLS macro.
In turn, rcu_state::levelcnt[] array is never accessed
beyond rcu_num_lvls. Thus, rcu_state::levelcnt[] is safe
to limit to RCU_NUM_LVLS items.

Since rcu_num_lvls could be changed during boot (as result
of rcutree.rcu_fanout_leaf kernel parameter update) one might
assume a new value could overflow the value of RCU_NUM_LVLS.
However, that is not the case, since leaf-level fanout is only
permitted to increase, resulting in rcu_num_lvls possibly to
decrease.

Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 kernel/rcu/tree.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index f1f4784..a6faae5 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -443,7 +443,7 @@ do {									\
 struct rcu_state {
 	struct rcu_node node[NUM_RCU_NODES];	/* Hierarchy. */
 	struct rcu_node *level[RCU_NUM_LVLS];	/* Hierarchy levels. */
-	u32 levelcnt[MAX_RCU_LVLS + 1];		/* # nodes in each level. */
+	u32 levelcnt[RCU_NUM_LVLS];		/* # nodes in each level. */
 	u8 levelspread[RCU_NUM_LVLS];		/* kids/node in each level. */
 	u8 flavor_mask;				/* bit in flavor mask. */
 	struct rcu_data __percpu *rda;		/* pointer of percu rcu_data. */
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 6/9] rcu: Limit rcu_capacity[] size to RCU_NUM_LVLS items
  2015-05-29  9:53 [PATCH v2 0/9] rcu: Cleanup RCU tree initialization Alexander Gordeev
                   ` (4 preceding siblings ...)
  2015-05-29  9:53 ` [PATCH v2 5/9] rcu: Limit rcu_state::levelcnt[] to RCU_NUM_LVLS items Alexander Gordeev
@ 2015-05-29  9:53 ` Alexander Gordeev
  2015-05-29  9:53 ` [PATCH v2 7/9] rcu: Remove unnecessary fields from rcu_state structure Alexander Gordeev
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Alexander Gordeev @ 2015-05-29  9:53 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexander Gordeev, Paul E. McKenney, Steven Rostedt

Number of items in rcu_capacity[] array is defined by macro
MAX_RCU_LVLS. However, that array is never accessed beyond
RCU_NUM_LVLS index. Therefore, we can limit the array to
RCU_NUM_LVLS items and eliminate MAX_RCU_LVLS. As result,
in most cases the memory is conserved.

Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 kernel/rcu/tree.c | 12 ++++++------
 kernel/rcu/tree.h |  2 --
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index b10e48e..f9be748 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3998,19 +3998,19 @@ static void __init rcu_init_one(struct rcu_state *rsp,
 		"rcu_node_0",
 		"rcu_node_1",
 		"rcu_node_2",
-		"rcu_node_3" };  /* Match MAX_RCU_LVLS */
+		"rcu_node_3" };
 	static const char * const fqs[] = {
 		"rcu_node_fqs_0",
 		"rcu_node_fqs_1",
 		"rcu_node_fqs_2",
-		"rcu_node_fqs_3" };  /* Match MAX_RCU_LVLS */
+		"rcu_node_fqs_3" };
 	static u8 fl_mask = 0x1;
 	int cpustride = 1;
 	int i;
 	int j;
 	struct rcu_node *rnp;
 
-	BUILD_BUG_ON(MAX_RCU_LVLS > ARRAY_SIZE(buf));  /* Fix buf[] init! */
+	BUILD_BUG_ON(RCU_NUM_LVLS > ARRAY_SIZE(buf));  /* Fix buf[] init! */
 
 	/* Silence gcc 4.8 false positive about array index out of range. */
 	if (rcu_num_lvls <= 0 || rcu_num_lvls > RCU_NUM_LVLS)
@@ -4082,7 +4082,7 @@ static void __init rcu_init_geometry(void)
 {
 	ulong d;
 	int i;
-	int rcu_capacity[MAX_RCU_LVLS];
+	int rcu_capacity[RCU_NUM_LVLS];
 
 	/*
 	 * Initialize any unspecified boot parameters.
@@ -4109,7 +4109,7 @@ static void __init rcu_init_geometry(void)
 	 * with the given number of levels.
 	 */
 	rcu_capacity[0] = rcu_fanout_leaf;
-	for (i = 1; i < MAX_RCU_LVLS; i++)
+	for (i = 1; i < RCU_NUM_LVLS; i++)
 		rcu_capacity[i] = rcu_capacity[i - 1] * RCU_FANOUT;
 
 	/*
@@ -4122,7 +4122,7 @@ static void __init rcu_init_geometry(void)
 	 * the rcu_node masks.  Complain and fall back to the compile-
 	 * time values if these limits are exceeded.
 	 */
-	if (nr_cpu_ids > rcu_capacity[MAX_RCU_LVLS - 1])
+	if (nr_cpu_ids > rcu_capacity[RCU_NUM_LVLS - 1])
 		panic("rcu_init_geometry: rcu_capacity[] is too small");
 	else if (rcu_fanout_leaf < RCU_FANOUT_LEAF ||
 		 rcu_fanout_leaf > sizeof(unsigned long) * 8) {
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index a6faae5..d625e9f 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -36,8 +36,6 @@
  * Of course, your mileage may vary.
  */
 
-#define MAX_RCU_LVLS 4
-
 #ifdef CONFIG_RCU_FANOUT
 #define RCU_FANOUT CONFIG_RCU_FANOUT
 #else /* #ifdef CONFIG_RCU_FANOUT */
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 7/9] rcu: Remove unnecessary fields from rcu_state structure
  2015-05-29  9:53 [PATCH v2 0/9] rcu: Cleanup RCU tree initialization Alexander Gordeev
                   ` (5 preceding siblings ...)
  2015-05-29  9:53 ` [PATCH v2 6/9] rcu: Limit rcu_capacity[] size " Alexander Gordeev
@ 2015-05-29  9:53 ` Alexander Gordeev
  2015-05-29  9:53 ` [PATCH v2 8/9] rcu: Limit count of static data to the number of RCU levels Alexander Gordeev
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Alexander Gordeev @ 2015-05-29  9:53 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexander Gordeev, Paul E. McKenney, Steven Rostedt

Members rcu_state::levelcnt[] and rcu_state::levelspread[]
are only used at init. There is no reason to keep them
afterwards.

Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 kernel/rcu/tree.c | 27 +++++++++++++++------------
 kernel/rcu/tree.h |  2 --
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index f9be748..ef3c80d 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3967,22 +3967,22 @@ void rcu_scheduler_starting(void)
  * Compute the per-level fanout, either using the exact fanout specified
  * or balancing the tree, depending on the rcu_fanout_exact boot parameter.
  */
-static void __init rcu_init_levelspread(struct rcu_state *rsp)
+static void __init rcu_init_levelspread(int *levelspread, const int *levelcnt)
 {
 	int i;
 
 	if (rcu_fanout_exact) {
-		rsp->levelspread[rcu_num_lvls - 1] = rcu_fanout_leaf;
+		levelspread[rcu_num_lvls - 1] = rcu_fanout_leaf;
 		for (i = rcu_num_lvls - 2; i >= 0; i--)
-			rsp->levelspread[i] = RCU_FANOUT;
+			levelspread[i] = RCU_FANOUT;
 	} else {
 		int ccur;
 		int cprv;
 
 		cprv = nr_cpu_ids;
 		for (i = rcu_num_lvls - 1; i >= 0; i--) {
-			ccur = rsp->levelcnt[i];
-			rsp->levelspread[i] = (cprv + ccur - 1) / ccur;
+			ccur = levelcnt[i];
+			levelspread[i] = (cprv + ccur - 1) / ccur;
 			cprv = ccur;
 		}
 	}
@@ -4005,6 +4005,9 @@ static void __init rcu_init_one(struct rcu_state *rsp,
 		"rcu_node_fqs_2",
 		"rcu_node_fqs_3" };
 	static u8 fl_mask = 0x1;
+
+	int levelcnt[RCU_NUM_LVLS];		/* # nodes in each level. */
+	int levelspread[RCU_NUM_LVLS];		/* kids/node in each level. */
 	int cpustride = 1;
 	int i;
 	int j;
@@ -4019,19 +4022,19 @@ static void __init rcu_init_one(struct rcu_state *rsp,
 	/* Initialize the level-tracking arrays. */
 
 	for (i = 0; i < rcu_num_lvls; i++)
-		rsp->levelcnt[i] = num_rcu_lvl[i];
+		levelcnt[i] = num_rcu_lvl[i];
 	for (i = 1; i < rcu_num_lvls; i++)
-		rsp->level[i] = rsp->level[i - 1] + rsp->levelcnt[i - 1];
-	rcu_init_levelspread(rsp);
+		rsp->level[i] = rsp->level[i - 1] + levelcnt[i - 1];
+	rcu_init_levelspread(levelspread, levelcnt);
 	rsp->flavor_mask = fl_mask;
 	fl_mask <<= 1;
 
 	/* Initialize the elements themselves, starting from the leaves. */
 
 	for (i = rcu_num_lvls - 1; i >= 0; i--) {
-		cpustride *= rsp->levelspread[i];
+		cpustride *= levelspread[i];
 		rnp = rsp->level[i];
-		for (j = 0; j < rsp->levelcnt[i]; j++, rnp++) {
+		for (j = 0; j < levelcnt[i]; j++, rnp++) {
 			raw_spin_lock_init(&rnp->lock);
 			lockdep_set_class_and_name(&rnp->lock,
 						   &rcu_node_class[i], buf[i]);
@@ -4051,10 +4054,10 @@ static void __init rcu_init_one(struct rcu_state *rsp,
 				rnp->grpmask = 0;
 				rnp->parent = NULL;
 			} else {
-				rnp->grpnum = j % rsp->levelspread[i - 1];
+				rnp->grpnum = j % levelspread[i - 1];
 				rnp->grpmask = 1UL << rnp->grpnum;
 				rnp->parent = rsp->level[i - 1] +
-					      j / rsp->levelspread[i - 1];
+					      j / levelspread[i - 1];
 			}
 			rnp->level = i;
 			INIT_LIST_HEAD(&rnp->blkd_tasks);
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index d625e9f..3413f3c 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -441,8 +441,6 @@ do {									\
 struct rcu_state {
 	struct rcu_node node[NUM_RCU_NODES];	/* Hierarchy. */
 	struct rcu_node *level[RCU_NUM_LVLS];	/* Hierarchy levels. */
-	u32 levelcnt[RCU_NUM_LVLS];		/* # nodes in each level. */
-	u8 levelspread[RCU_NUM_LVLS];		/* kids/node in each level. */
 	u8 flavor_mask;				/* bit in flavor mask. */
 	struct rcu_data __percpu *rda;		/* pointer of percu rcu_data. */
 	void (*call)(struct rcu_head *head,	/* call_rcu() flavor. */
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 8/9] rcu: Limit count of static data to the number of RCU levels
  2015-05-29  9:53 [PATCH v2 0/9] rcu: Cleanup RCU tree initialization Alexander Gordeev
                   ` (6 preceding siblings ...)
  2015-05-29  9:53 ` [PATCH v2 7/9] rcu: Remove unnecessary fields from rcu_state structure Alexander Gordeev
@ 2015-05-29  9:53 ` Alexander Gordeev
  2015-05-29  9:53 ` [PATCH v2 9/9] rcu: Simplify arithmetic to calculate number of RCU nodes Alexander Gordeev
  2015-06-01 18:58 ` [PATCH v2 0/9] rcu: Cleanup RCU tree initialization Paul E. McKenney
  9 siblings, 0 replies; 14+ messages in thread
From: Alexander Gordeev @ 2015-05-29  9:53 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexander Gordeev, Paul E. McKenney, Steven Rostedt

Although a number of RCU levels may be less than the current
maximum of four, some static data associated with each level
are allocated for all four levels. As result, the extra data
never get accessed and just wast memory. This update limits
count of allocated items to the number of used RCU levels.

Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 kernel/rcu/tree.c | 21 ++++-----------------
 kernel/rcu/tree.h | 12 ++++++++++++
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index ef3c80d..478a12f 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -124,13 +124,8 @@ module_param(rcu_fanout_exact, bool, 0444);
 static int rcu_fanout_leaf = RCU_FANOUT_LEAF;
 module_param(rcu_fanout_leaf, int, 0444);
 int rcu_num_lvls __read_mostly = RCU_NUM_LVLS;
-static int num_rcu_lvl[] = {  /* Number of rcu_nodes at specified level. */
-	NUM_RCU_LVL_0,
-	NUM_RCU_LVL_1,
-	NUM_RCU_LVL_2,
-	NUM_RCU_LVL_3,
-	NUM_RCU_LVL_4,
-};
+/* Number of rcu_nodes at specified level. */
+static int num_rcu_lvl[] = NUM_RCU_LVL_INIT;
 int rcu_num_nodes __read_mostly = NUM_RCU_NODES; /* Total # rcu_nodes in use. */
 
 /*
@@ -3994,16 +3989,8 @@ static void __init rcu_init_levelspread(int *levelspread, const int *levelcnt)
 static void __init rcu_init_one(struct rcu_state *rsp,
 		struct rcu_data __percpu *rda)
 {
-	static const char * const buf[] = {
-		"rcu_node_0",
-		"rcu_node_1",
-		"rcu_node_2",
-		"rcu_node_3" };
-	static const char * const fqs[] = {
-		"rcu_node_fqs_0",
-		"rcu_node_fqs_1",
-		"rcu_node_fqs_2",
-		"rcu_node_fqs_3" };
+	static const char * const buf[] = RCU_NODE_NAME_INIT;
+	static const char * const fqs[] = RCU_FQS_NAME_INIT;
 	static u8 fl_mask = 0x1;
 
 	int levelcnt[RCU_NUM_LVLS];		/* # nodes in each level. */
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index 3413f3c..d44856b 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -68,6 +68,9 @@
 #  define NUM_RCU_LVL_2	      0
 #  define NUM_RCU_LVL_3	      0
 #  define NUM_RCU_LVL_4	      0
+#  define NUM_RCU_LVL_INIT    { NUM_RCU_LVL_0 }
+#  define RCU_NODE_NAME_INIT  { "rcu_node_0" }
+#  define RCU_FQS_NAME_INIT   { "rcu_node_fqs_0" }
 #elif NR_CPUS <= RCU_FANOUT_2
 #  define RCU_NUM_LVLS	      2
 #  define NUM_RCU_LVL_0	      1
@@ -75,6 +78,9 @@
 #  define NUM_RCU_LVL_2	      (NR_CPUS)
 #  define NUM_RCU_LVL_3	      0
 #  define NUM_RCU_LVL_4	      0
+#  define NUM_RCU_LVL_INIT    { NUM_RCU_LVL_0, NUM_RCU_LVL_1 }
+#  define RCU_NODE_NAME_INIT  { "rcu_node_0", "rcu_node_1" }
+#  define RCU_FQS_NAME_INIT   { "rcu_node_fqs_0", "rcu_node_fqs_1" }
 #elif NR_CPUS <= RCU_FANOUT_3
 #  define RCU_NUM_LVLS	      3
 #  define NUM_RCU_LVL_0	      1
@@ -82,6 +88,9 @@
 #  define NUM_RCU_LVL_2	      DIV_ROUND_UP(NR_CPUS, RCU_FANOUT_1)
 #  define NUM_RCU_LVL_3	      (NR_CPUS)
 #  define NUM_RCU_LVL_4	      0
+#  define NUM_RCU_LVL_INIT    { NUM_RCU_LVL_0, NUM_RCU_LVL_1, NUM_RCU_LVL_2 }
+#  define RCU_NODE_NAME_INIT  { "rcu_node_0", "rcu_node_1", "rcu_node_2" }
+#  define RCU_FQS_NAME_INIT   { "rcu_node_fqs_0", "rcu_node_fqs_1", "rcu_node_fqs_2" }
 #elif NR_CPUS <= RCU_FANOUT_4
 #  define RCU_NUM_LVLS	      4
 #  define NUM_RCU_LVL_0	      1
@@ -89,6 +98,9 @@
 #  define NUM_RCU_LVL_2	      DIV_ROUND_UP(NR_CPUS, RCU_FANOUT_2)
 #  define NUM_RCU_LVL_3	      DIV_ROUND_UP(NR_CPUS, RCU_FANOUT_1)
 #  define NUM_RCU_LVL_4	      (NR_CPUS)
+#  define NUM_RCU_LVL_INIT    { NUM_RCU_LVL_0, NUM_RCU_LVL_1, NUM_RCU_LVL_2, NUM_RCU_LVL_3 }
+#  define RCU_NODE_NAME_INIT  { "rcu_node_0", "rcu_node_1", "rcu_node_2", "rcu_node_3" }
+#  define RCU_FQS_NAME_INIT   { "rcu_node_fqs_0", "rcu_node_fqs_1", "rcu_node_fqs_2", "rcu_node_fqs_3" }
 #else
 # error "CONFIG_RCU_FANOUT insufficient for NR_CPUS"
 #endif /* #if (NR_CPUS) <= RCU_FANOUT_1 */
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 9/9] rcu: Simplify arithmetic to calculate number of RCU nodes
  2015-05-29  9:53 [PATCH v2 0/9] rcu: Cleanup RCU tree initialization Alexander Gordeev
                   ` (7 preceding siblings ...)
  2015-05-29  9:53 ` [PATCH v2 8/9] rcu: Limit count of static data to the number of RCU levels Alexander Gordeev
@ 2015-05-29  9:53 ` Alexander Gordeev
  2015-06-01 18:58 ` [PATCH v2 0/9] rcu: Cleanup RCU tree initialization Paul E. McKenney
  9 siblings, 0 replies; 14+ messages in thread
From: Alexander Gordeev @ 2015-05-29  9:53 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexander Gordeev, Paul E. McKenney, Steven Rostedt

This update makes arithmetic to calculate number of RCU nodes
more straight and easy to read.

Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 kernel/rcu/tree.h        | 17 ++++-------------
 kernel/rcu/tree_plugin.h |  4 ++--
 2 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index d44856b..581f8d3 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -64,10 +64,7 @@
 #if NR_CPUS <= RCU_FANOUT_1
 #  define RCU_NUM_LVLS	      1
 #  define NUM_RCU_LVL_0	      1
-#  define NUM_RCU_LVL_1	      (NR_CPUS)
-#  define NUM_RCU_LVL_2	      0
-#  define NUM_RCU_LVL_3	      0
-#  define NUM_RCU_LVL_4	      0
+#  define NUM_RCU_NODES	      NUM_RCU_LVL_0
 #  define NUM_RCU_LVL_INIT    { NUM_RCU_LVL_0 }
 #  define RCU_NODE_NAME_INIT  { "rcu_node_0" }
 #  define RCU_FQS_NAME_INIT   { "rcu_node_fqs_0" }
@@ -75,9 +72,7 @@
 #  define RCU_NUM_LVLS	      2
 #  define NUM_RCU_LVL_0	      1
 #  define NUM_RCU_LVL_1	      DIV_ROUND_UP(NR_CPUS, RCU_FANOUT_1)
-#  define NUM_RCU_LVL_2	      (NR_CPUS)
-#  define NUM_RCU_LVL_3	      0
-#  define NUM_RCU_LVL_4	      0
+#  define NUM_RCU_NODES	      (NUM_RCU_LVL_0 + NUM_RCU_LVL_1)
 #  define NUM_RCU_LVL_INIT    { NUM_RCU_LVL_0, NUM_RCU_LVL_1 }
 #  define RCU_NODE_NAME_INIT  { "rcu_node_0", "rcu_node_1" }
 #  define RCU_FQS_NAME_INIT   { "rcu_node_fqs_0", "rcu_node_fqs_1" }
@@ -86,8 +81,7 @@
 #  define NUM_RCU_LVL_0	      1
 #  define NUM_RCU_LVL_1	      DIV_ROUND_UP(NR_CPUS, RCU_FANOUT_2)
 #  define NUM_RCU_LVL_2	      DIV_ROUND_UP(NR_CPUS, RCU_FANOUT_1)
-#  define NUM_RCU_LVL_3	      (NR_CPUS)
-#  define NUM_RCU_LVL_4	      0
+#  define NUM_RCU_NODES	      (NUM_RCU_LVL_0 + NUM_RCU_LVL_1 + NUM_RCU_LVL_2)
 #  define NUM_RCU_LVL_INIT    { NUM_RCU_LVL_0, NUM_RCU_LVL_1, NUM_RCU_LVL_2 }
 #  define RCU_NODE_NAME_INIT  { "rcu_node_0", "rcu_node_1", "rcu_node_2" }
 #  define RCU_FQS_NAME_INIT   { "rcu_node_fqs_0", "rcu_node_fqs_1", "rcu_node_fqs_2" }
@@ -97,7 +91,7 @@
 #  define NUM_RCU_LVL_1	      DIV_ROUND_UP(NR_CPUS, RCU_FANOUT_3)
 #  define NUM_RCU_LVL_2	      DIV_ROUND_UP(NR_CPUS, RCU_FANOUT_2)
 #  define NUM_RCU_LVL_3	      DIV_ROUND_UP(NR_CPUS, RCU_FANOUT_1)
-#  define NUM_RCU_LVL_4	      (NR_CPUS)
+#  define NUM_RCU_NODES	      (NUM_RCU_LVL_0 + NUM_RCU_LVL_1 + NUM_RCU_LVL_2 + NUM_RCU_LVL_3)
 #  define NUM_RCU_LVL_INIT    { NUM_RCU_LVL_0, NUM_RCU_LVL_1, NUM_RCU_LVL_2, NUM_RCU_LVL_3 }
 #  define RCU_NODE_NAME_INIT  { "rcu_node_0", "rcu_node_1", "rcu_node_2", "rcu_node_3" }
 #  define RCU_FQS_NAME_INIT   { "rcu_node_fqs_0", "rcu_node_fqs_1", "rcu_node_fqs_2", "rcu_node_fqs_3" }
@@ -105,9 +99,6 @@
 # error "CONFIG_RCU_FANOUT insufficient for NR_CPUS"
 #endif /* #if (NR_CPUS) <= RCU_FANOUT_1 */
 
-#define RCU_SUM (NUM_RCU_LVL_0 + NUM_RCU_LVL_1 + NUM_RCU_LVL_2 + NUM_RCU_LVL_3 + NUM_RCU_LVL_4)
-#define NUM_RCU_NODES (RCU_SUM - NR_CPUS)
-
 extern int rcu_num_lvls;
 extern int rcu_num_nodes;
 
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 3266434..a2f64e4 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -84,8 +84,8 @@ static void __init rcu_bootup_announce_oddness(void)
 		pr_info("\tRCU torture testing starts during boot.\n");
 	if (IS_ENABLED(CONFIG_RCU_CPU_STALL_INFO))
 		pr_info("\tAdditional per-CPU info printed with stalls.\n");
-	if (NUM_RCU_LVL_4 != 0)
-		pr_info("\tFour-level hierarchy is enabled.\n");
+	if (RCU_NUM_LVLS >= 4)
+		pr_info("\tFour(or more)-level hierarchy is enabled.\n");
 	if (RCU_FANOUT_LEAF != 16)
 		pr_info("\tBuild-time adjustment of leaf fanout to %d.\n",
 			RCU_FANOUT_LEAF);
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/9] rcu: Panic if RCU tree can not accommodate all CPUs
  2015-05-29  9:53 ` [PATCH v2 1/9] rcu: Panic if RCU tree can not accommodate all CPUs Alexander Gordeev
@ 2015-06-01 18:37   ` Paul E. McKenney
  2015-06-02  6:45     ` Alexander Gordeev
  0 siblings, 1 reply; 14+ messages in thread
From: Paul E. McKenney @ 2015-06-01 18:37 UTC (permalink / raw)
  To: Alexander Gordeev; +Cc: linux-kernel, Steven Rostedt

On Fri, May 29, 2015 at 11:53:37AM +0200, Alexander Gordeev wrote:
> Currently a condition when RCU tree is unable to accommodate
> the configured number of CPUs is not permitted and causes
> a fall back to compile-time values. However, the code has no
> means to exceed the RCU tree capacity neither at compile-time
> nor in run-time. Therefore, if the condition is met in run-
> time then it indicates a serios problem elsewhere and should
> be handled with a panic.
> 
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  kernel/rcu/tree.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 2fce662..66a4230 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -4117,16 +4117,19 @@ static void __init rcu_init_geometry(void)
>  		rcu_capacity[i] = rcu_capacity[i - 1] * RCU_FANOUT;
> 
>  	/*
> +	 * The tree must be able to accommodate the configured number of CPUs.
> +	 * If this limit is exceeded than we have a serious problem elsewhere.
> +	 *
>  	 * The boot-time rcu_fanout_leaf parameter is only permitted
>  	 * to increase the leaf-level fanout, not decrease it.  Of course,
>  	 * the leaf-level fanout cannot exceed the number of bits in
> -	 * the rcu_node masks.  Finally, the tree must be able to accommodate
> -	 * the configured number of CPUs.  Complain and fall back to the
> -	 * compile-time values if these limits are exceeded.
> +	 * the rcu_node masks.  Complain and fall back to the compile-
> +	 * time values if these limits are exceeded.
>  	 */
> -	if (rcu_fanout_leaf < RCU_FANOUT_LEAF ||
> -	    rcu_fanout_leaf > sizeof(unsigned long) * 8 ||
> -	    n > rcu_capacity[MAX_RCU_LVLS]) {
> +	if (n > rcu_capacity[MAX_RCU_LVLS])
> +		panic("rcu_init_geometry: rcu_capacity[] is too small");

The way this is set up, if the boot parameter (illegally) sets
rcu_fanout_lead smaller than RCU_FANOUT_LEAF, we might panic.  It would
be far better to first check for rcu_fanout_leaf being out of bounds,
and only then have the possibility of panic().  That way, a typo in
the rcu_fanout_leaf boot paremeter is ignored, but with a splat.

Or am I missing something here?

							Thanx, Paul

> +	else if (rcu_fanout_leaf < RCU_FANOUT_LEAF ||
> +		 rcu_fanout_leaf > sizeof(unsigned long) * 8) {
>  		WARN_ON(1);
>  		return;
>  	}
> -- 
> 1.8.3.1
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 0/9] rcu: Cleanup RCU tree initialization
  2015-05-29  9:53 [PATCH v2 0/9] rcu: Cleanup RCU tree initialization Alexander Gordeev
                   ` (8 preceding siblings ...)
  2015-05-29  9:53 ` [PATCH v2 9/9] rcu: Simplify arithmetic to calculate number of RCU nodes Alexander Gordeev
@ 2015-06-01 18:58 ` Paul E. McKenney
  9 siblings, 0 replies; 14+ messages in thread
From: Paul E. McKenney @ 2015-06-01 18:58 UTC (permalink / raw)
  To: Alexander Gordeev; +Cc: linux-kernel, Steven Rostedt

On Fri, May 29, 2015 at 11:53:36AM +0200, Alexander Gordeev wrote:
> Hello Paul,
> 
> This is 2nd attempt to make RCU tree initialization bit more
> clear and optimize memory footprint of data associated with
> the tree.

Thank you for sending these -- please see the comment on patch #1.
I have queued these for testing in the meantime, but please either
explain why I am wrong or resend the set with the fix.

							Thanx, Paul

> Changes since v1:
>   - patch 3 fixed to accomodate nr_cpus=1, otherwise rcu_num_lvls
>     would yield zero, which is wrong;
> 
> The series is against "linux-rcu" tree "rcu/dev" branch
> 
> Thanks!
> 
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> 
> Alexander Gordeev (9):
>   rcu: Panic if RCU tree can not accommodate all CPUs
>   rcu: Remove superfluous local variable in rcu_init_geometry()
>   rcu: Cleanup rcu_init_geometry() code and arithmetics
>   rcu: Simplify rcu_init_geometry() capacity arithmetics
>   rcu: Limit rcu_state::levelcnt[] to RCU_NUM_LVLS items
>   rcu: Limit rcu_capacity[] size to RCU_NUM_LVLS items
>   rcu: Remove unnecessary fields from rcu_state structure
>   rcu: Limit count of static data to the number of RCU levels
>   rcu: Simplify arithmetic to calculate number of RCU nodes
> 
>  kernel/rcu/tree.c        | 99 +++++++++++++++++++++---------------------------
>  kernel/rcu/tree.h        | 33 ++++++++--------
>  kernel/rcu/tree_plugin.h |  4 +-
>  3 files changed, 61 insertions(+), 75 deletions(-)
> 
> -- 
> 1.8.3.1
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/9] rcu: Panic if RCU tree can not accommodate all CPUs
  2015-06-01 18:37   ` Paul E. McKenney
@ 2015-06-02  6:45     ` Alexander Gordeev
  2015-06-02 12:37       ` Paul E. McKenney
  0 siblings, 1 reply; 14+ messages in thread
From: Alexander Gordeev @ 2015-06-02  6:45 UTC (permalink / raw)
  To: Paul E. McKenney; +Cc: linux-kernel, Steven Rostedt

On Mon, Jun 01, 2015 at 11:37:21AM -0700, Paul E. McKenney wrote:
> On Fri, May 29, 2015 at 11:53:37AM +0200, Alexander Gordeev wrote:
> > Currently a condition when RCU tree is unable to accommodate
> > the configured number of CPUs is not permitted and causes
> > a fall back to compile-time values. However, the code has no
> > means to exceed the RCU tree capacity neither at compile-time
> > nor in run-time. Therefore, if the condition is met in run-
> > time then it indicates a serios problem elsewhere and should
> > be handled with a panic.
> > 
> > Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > ---
> >  kernel/rcu/tree.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 2fce662..66a4230 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -4117,16 +4117,19 @@ static void __init rcu_init_geometry(void)
> >  		rcu_capacity[i] = rcu_capacity[i - 1] * RCU_FANOUT;
> > 
> >  	/*
> > +	 * The tree must be able to accommodate the configured number of CPUs.
> > +	 * If this limit is exceeded than we have a serious problem elsewhere.
> > +	 *
> >  	 * The boot-time rcu_fanout_leaf parameter is only permitted
> >  	 * to increase the leaf-level fanout, not decrease it.  Of course,
> >  	 * the leaf-level fanout cannot exceed the number of bits in
> > -	 * the rcu_node masks.  Finally, the tree must be able to accommodate
> > -	 * the configured number of CPUs.  Complain and fall back to the
> > -	 * compile-time values if these limits are exceeded.
> > +	 * the rcu_node masks.  Complain and fall back to the compile-
> > +	 * time values if these limits are exceeded.
> >  	 */
> > -	if (rcu_fanout_leaf < RCU_FANOUT_LEAF ||
> > -	    rcu_fanout_leaf > sizeof(unsigned long) * 8 ||
> > -	    n > rcu_capacity[MAX_RCU_LVLS]) {
> > +	if (n > rcu_capacity[MAX_RCU_LVLS])
> > +		panic("rcu_init_geometry: rcu_capacity[] is too small");
> 
> The way this is set up, if the boot parameter (illegally) sets
> rcu_fanout_lead smaller than RCU_FANOUT_LEAF, we might panic.  It would
> be far better to first check for rcu_fanout_leaf being out of bounds,
> and only then have the possibility of panic().  That way, a typo in
> the rcu_fanout_leaf boot paremeter is ignored, but with a splat.
> 
> Or am I missing something here?

I think you are quite right. But the bounds check is misplaced then.
I would say, it should be placed before rcu_capacity[] seed, as it
only deals with constants and has nothing with rcu_capacity[].

I will send the updated version.

> 							Thanx, Paul
> 
> > +	else if (rcu_fanout_leaf < RCU_FANOUT_LEAF ||
> > +		 rcu_fanout_leaf > sizeof(unsigned long) * 8) {
> >  		WARN_ON(1);
> >  		return;
> >  	}
> > -- 
> > 1.8.3.1
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/9] rcu: Panic if RCU tree can not accommodate all CPUs
  2015-06-02  6:45     ` Alexander Gordeev
@ 2015-06-02 12:37       ` Paul E. McKenney
  0 siblings, 0 replies; 14+ messages in thread
From: Paul E. McKenney @ 2015-06-02 12:37 UTC (permalink / raw)
  To: Alexander Gordeev; +Cc: linux-kernel, Steven Rostedt

On Tue, Jun 02, 2015 at 07:45:27AM +0100, Alexander Gordeev wrote:
> On Mon, Jun 01, 2015 at 11:37:21AM -0700, Paul E. McKenney wrote:
> > On Fri, May 29, 2015 at 11:53:37AM +0200, Alexander Gordeev wrote:
> > > Currently a condition when RCU tree is unable to accommodate
> > > the configured number of CPUs is not permitted and causes
> > > a fall back to compile-time values. However, the code has no
> > > means to exceed the RCU tree capacity neither at compile-time
> > > nor in run-time. Therefore, if the condition is met in run-
> > > time then it indicates a serios problem elsewhere and should
> > > be handled with a panic.
> > > 
> > > Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > > ---
> > >  kernel/rcu/tree.c | 15 +++++++++------
> > >  1 file changed, 9 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 2fce662..66a4230 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -4117,16 +4117,19 @@ static void __init rcu_init_geometry(void)
> > >  		rcu_capacity[i] = rcu_capacity[i - 1] * RCU_FANOUT;
> > > 
> > >  	/*
> > > +	 * The tree must be able to accommodate the configured number of CPUs.
> > > +	 * If this limit is exceeded than we have a serious problem elsewhere.
> > > +	 *
> > >  	 * The boot-time rcu_fanout_leaf parameter is only permitted
> > >  	 * to increase the leaf-level fanout, not decrease it.  Of course,
> > >  	 * the leaf-level fanout cannot exceed the number of bits in
> > > -	 * the rcu_node masks.  Finally, the tree must be able to accommodate
> > > -	 * the configured number of CPUs.  Complain and fall back to the
> > > -	 * compile-time values if these limits are exceeded.
> > > +	 * the rcu_node masks.  Complain and fall back to the compile-
> > > +	 * time values if these limits are exceeded.
> > >  	 */
> > > -	if (rcu_fanout_leaf < RCU_FANOUT_LEAF ||
> > > -	    rcu_fanout_leaf > sizeof(unsigned long) * 8 ||
> > > -	    n > rcu_capacity[MAX_RCU_LVLS]) {
> > > +	if (n > rcu_capacity[MAX_RCU_LVLS])
> > > +		panic("rcu_init_geometry: rcu_capacity[] is too small");
> > 
> > The way this is set up, if the boot parameter (illegally) sets
> > rcu_fanout_lead smaller than RCU_FANOUT_LEAF, we might panic.  It would
> > be far better to first check for rcu_fanout_leaf being out of bounds,
> > and only then have the possibility of panic().  That way, a typo in
> > the rcu_fanout_leaf boot paremeter is ignored, but with a splat.
> > 
> > Or am I missing something here?
> 
> I think you are quite right. But the bounds check is misplaced then.
> I would say, it should be placed before rcu_capacity[] seed, as it
> only deals with constants and has nothing with rcu_capacity[].

That makes sense as well.

> I will send the updated version.

Very good, looking forward to it!

By the way, on the specific configurations that I test, the patch
generates the same topology as previously, which is reassuring.
An exhaustive test is needed, of course.

							Thanx, Paul

> > > +	else if (rcu_fanout_leaf < RCU_FANOUT_LEAF ||
> > > +		 rcu_fanout_leaf > sizeof(unsigned long) * 8) {
> > >  		WARN_ON(1);
> > >  		return;
> > >  	}
> > > -- 
> > > 1.8.3.1
> > > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> -- 
> Regards,
> Alexander Gordeev
> agordeev@redhat.com
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-06-02 12:37 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-29  9:53 [PATCH v2 0/9] rcu: Cleanup RCU tree initialization Alexander Gordeev
2015-05-29  9:53 ` [PATCH v2 1/9] rcu: Panic if RCU tree can not accommodate all CPUs Alexander Gordeev
2015-06-01 18:37   ` Paul E. McKenney
2015-06-02  6:45     ` Alexander Gordeev
2015-06-02 12:37       ` Paul E. McKenney
2015-05-29  9:53 ` [PATCH v2 2/9] rcu: Remove superfluous local variable in rcu_init_geometry() Alexander Gordeev
2015-05-29  9:53 ` [PATCH v2 3/9] rcu: Cleanup rcu_init_geometry() code and arithmetics Alexander Gordeev
2015-05-29  9:53 ` [PATCH v2 4/9] rcu: Simplify rcu_init_geometry() capacity arithmetics Alexander Gordeev
2015-05-29  9:53 ` [PATCH v2 5/9] rcu: Limit rcu_state::levelcnt[] to RCU_NUM_LVLS items Alexander Gordeev
2015-05-29  9:53 ` [PATCH v2 6/9] rcu: Limit rcu_capacity[] size " Alexander Gordeev
2015-05-29  9:53 ` [PATCH v2 7/9] rcu: Remove unnecessary fields from rcu_state structure Alexander Gordeev
2015-05-29  9:53 ` [PATCH v2 8/9] rcu: Limit count of static data to the number of RCU levels Alexander Gordeev
2015-05-29  9:53 ` [PATCH v2 9/9] rcu: Simplify arithmetic to calculate number of RCU nodes Alexander Gordeev
2015-06-01 18:58 ` [PATCH v2 0/9] rcu: Cleanup RCU tree initialization Paul E. McKenney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).