Messages in this thread | | | Date | Thu, 14 May 2015 22:15:29 +0100 | From | Alexander Gordeev <> | Subject | Re: [PATCH tip/core/rcu 06/24] rcu: Cleanup rcu_init_geometry() code and arithmetics |
| |
On Tue, May 12, 2015 at 11:22:32PM -0400, Steven Rostedt wrote: > On Tue, 12 May 2015 15:30:36 -0700 > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote: > > > @@ -4103,24 +4102,21 @@ static void __init rcu_init_geometry(void) > > return; > > } > > > > + /* Calculate the number of levels in the tree. */ > > + for (i = 0; nr_cpu_ids > rcu_capacity[i]; i++) { > > Should this start at i = 1 as it use to? Also, should there be a safety > check too: > > for (i = 1; i <= MAX_RCU_LVLS && nr_cpu_ids > rcu_capacity[i]; i++) {
The safety check is not needed as it indirectly tried few lines above:
if (nr_cpu_ids > rcu_capacity[MAX_RCU_LVLS]) panic("rcu_init_geometry: rcu_capacity[] is too small");
Starting at i = 0 appears indeed incorrect. In case NR_CPUS of 1 that might yield rcu_num_lvls = 0, which is wrong. I will check it.
> > > + } > > + 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);
1. In case j == i, num_rcu_lvl[j] = nr_cpu_ids
> > - } > > - 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++) { > > Hmm, up above we have: for (j = 0; j <= i; j++) > > and now we have rcu_num_lvls = i, so shouldn't this be; > > for (i = 0; i <= rcu_num_lvls; i++) > > ?
No, it should not. See [1] above and [2] below.
> -- Steve > > > + 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;
So nr_cpu_ids is added rcu_num_nodes in the cycle and subtracted from rcu_num_nodes afterwards.
The new version of code does not neigher [1] nor [2].
> > } > > > > void __init rcu_init(void) >
-- Regards, Alexander Gordeev agordeev@redhat.com
| |