* [PATCH] ARM: cpuidle: declare cpuidle_ops __read_mostly @ 2016-08-10 8:49 Jisheng Zhang 2016-08-10 8:57 ` Arnd Bergmann 2016-08-10 9:54 ` Jisheng Zhang 0 siblings, 2 replies; 7+ messages in thread From: Jisheng Zhang @ 2016-08-10 8:49 UTC (permalink / raw) To: linux-arm-kernel cpuidle_ops is initialized once by arm_cpuidle_read_ops() during initialization, and thereafter is mostly read in arm_cpuidle_suspend() The fact that it is mostly read and not written to makes it candidates for __read_mostly declarations. Signed-off-by: Jisheng Zhang <jszhang@marvell.com> --- arch/arm/kernel/cpuidle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c index 7dccc96..762e0929 100644 --- a/arch/arm/kernel/cpuidle.c +++ b/arch/arm/kernel/cpuidle.c @@ -19,7 +19,7 @@ extern struct of_cpuidle_method __cpuidle_method_of_table[]; static const struct of_cpuidle_method __cpuidle_method_of_table_sentinel __used __section(__cpuidle_method_of_table_end); -static struct cpuidle_ops cpuidle_ops[NR_CPUS]; +static struct cpuidle_ops cpuidle_ops[NR_CPUS] __read_mostly; /** * arm_cpuidle_simple_enter() - a wrapper to cpu_do_idle() -- 2.8.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] ARM: cpuidle: declare cpuidle_ops __read_mostly 2016-08-10 8:49 [PATCH] ARM: cpuidle: declare cpuidle_ops __read_mostly Jisheng Zhang @ 2016-08-10 8:57 ` Arnd Bergmann 2016-08-10 9:19 ` Jisheng Zhang 2016-08-10 9:54 ` Jisheng Zhang 1 sibling, 1 reply; 7+ messages in thread From: Arnd Bergmann @ 2016-08-10 8:57 UTC (permalink / raw) To: linux-arm-kernel On Wednesday, August 10, 2016 4:49:57 PM CEST Jisheng Zhang wrote: > diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c > index 7dccc96..762e0929 100644 > --- a/arch/arm/kernel/cpuidle.c > +++ b/arch/arm/kernel/cpuidle.c > @@ -19,7 +19,7 @@ extern struct of_cpuidle_method __cpuidle_method_of_table[]; > static const struct of_cpuidle_method __cpuidle_method_of_table_sentinel > __used __section(__cpuidle_method_of_table_end); > > -static struct cpuidle_ops cpuidle_ops[NR_CPUS]; > +static struct cpuidle_ops cpuidle_ops[NR_CPUS] __read_mostly; Should this perhaps be percpu data instead? Arnd ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] ARM: cpuidle: declare cpuidle_ops __read_mostly 2016-08-10 8:57 ` Arnd Bergmann @ 2016-08-10 9:19 ` Jisheng Zhang 2016-08-10 10:47 ` Arnd Bergmann 0 siblings, 1 reply; 7+ messages in thread From: Jisheng Zhang @ 2016-08-10 9:19 UTC (permalink / raw) To: linux-arm-kernel Dear Arnd, On Wed, 10 Aug 2016 10:57:57 +0200 Arnd Bergmann wrote: > On Wednesday, August 10, 2016 4:49:57 PM CEST Jisheng Zhang wrote: > > diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c > > index 7dccc96..762e0929 100644 > > --- a/arch/arm/kernel/cpuidle.c > > +++ b/arch/arm/kernel/cpuidle.c > > @@ -19,7 +19,7 @@ extern struct of_cpuidle_method __cpuidle_method_of_table[]; > > static const struct of_cpuidle_method __cpuidle_method_of_table_sentinel > > __used __section(__cpuidle_method_of_table_end); > > > > -static struct cpuidle_ops cpuidle_ops[NR_CPUS]; > > +static struct cpuidle_ops cpuidle_ops[NR_CPUS] __read_mostly; > > Should this perhaps be percpu data instead? > Per my understanding, percpu is used for those vars with normal read/write frequency, while the cpuidle_ops is read mostly, so IMHO, __read_mostly is suitable, what do you think? Thanks, Jisheng ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] ARM: cpuidle: declare cpuidle_ops __read_mostly 2016-08-10 9:19 ` Jisheng Zhang @ 2016-08-10 10:47 ` Arnd Bergmann 2016-08-10 11:06 ` Jisheng Zhang 0 siblings, 1 reply; 7+ messages in thread From: Arnd Bergmann @ 2016-08-10 10:47 UTC (permalink / raw) To: linux-arm-kernel On Wednesday, August 10, 2016 5:19:26 PM CEST Jisheng Zhang wrote: > Dear Arnd, > > On Wed, 10 Aug 2016 10:57:57 +0200 Arnd Bergmann wrote: > > > On Wednesday, August 10, 2016 4:49:57 PM CEST Jisheng Zhang wrote: > > > diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c > > > index 7dccc96..762e0929 100644 > > > --- a/arch/arm/kernel/cpuidle.c > > > +++ b/arch/arm/kernel/cpuidle.c > > > @@ -19,7 +19,7 @@ extern struct of_cpuidle_method __cpuidle_method_of_table[]; > > > static const struct of_cpuidle_method __cpuidle_method_of_table_sentinel > > > __used __section(__cpuidle_method_of_table_end); > > > > > > -static struct cpuidle_ops cpuidle_ops[NR_CPUS]; > > > +static struct cpuidle_ops cpuidle_ops[NR_CPUS] __read_mostly; > > > > Should this perhaps be percpu data instead? > > > > Per my understanding, percpu is used for those vars with normal read/write > frequency, while the cpuidle_ops is read mostly, so IMHO, __read_mostly > is suitable, what do you think? You are right, __read_mostly is better than the normal .data section here, but percpu is also better than .data because it saves a little memory on machines that have few present CPUs than CONFIG_NR_CPUS. So both have their advantages, we just need to pick a preference. Actually __ro_after_init would be even better than __read_mostly here I think, as this is only updated in an __init function. I guess using that would have the added security advantage of preventing an attacker from writing to the function pointers when they find a way to overflow an access in the percpu data section. Arnd ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] ARM: cpuidle: declare cpuidle_ops __read_mostly 2016-08-10 10:47 ` Arnd Bergmann @ 2016-08-10 11:06 ` Jisheng Zhang 0 siblings, 0 replies; 7+ messages in thread From: Jisheng Zhang @ 2016-08-10 11:06 UTC (permalink / raw) To: linux-arm-kernel Dear Arnd, On Wed, 10 Aug 2016 12:47:21 +0200 Arnd Bergmann wrote: > On Wednesday, August 10, 2016 5:19:26 PM CEST Jisheng Zhang wrote: > > Dear Arnd, > > > > On Wed, 10 Aug 2016 10:57:57 +0200 Arnd Bergmann wrote: > > > > > On Wednesday, August 10, 2016 4:49:57 PM CEST Jisheng Zhang wrote: > > > > diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c > > > > index 7dccc96..762e0929 100644 > > > > --- a/arch/arm/kernel/cpuidle.c > > > > +++ b/arch/arm/kernel/cpuidle.c > > > > @@ -19,7 +19,7 @@ extern struct of_cpuidle_method __cpuidle_method_of_table[]; > > > > static const struct of_cpuidle_method __cpuidle_method_of_table_sentinel > > > > __used __section(__cpuidle_method_of_table_end); > > > > > > > > -static struct cpuidle_ops cpuidle_ops[NR_CPUS]; > > > > +static struct cpuidle_ops cpuidle_ops[NR_CPUS] __read_mostly; > > > > > > Should this perhaps be percpu data instead? > > > > > > > Per my understanding, percpu is used for those vars with normal read/write > > frequency, while the cpuidle_ops is read mostly, so IMHO, __read_mostly > > is suitable, what do you think? > > You are right, __read_mostly is better than the normal .data section here, > but percpu is also better than .data because it saves a little memory > on machines that have few present CPUs than CONFIG_NR_CPUS. > > So both have their advantages, we just need to pick a preference. > > Actually __ro_after_init would be even better than __read_mostly here > I think, as this is only updated in an __init function. I guess > using that would have the added security advantage of preventing > an attacker from writing to the function pointers when they > find a way to overflow an access in the percpu data section. > Got it, thanks for the detailed explanations. And I think the answer to questions: http://lists.infradead.org/pipermail/linux-arm-kernel/2016-August/448057.html and http://lists.infradead.org/pipermail/linux-arm-kernel/2016-August/448059.html are both "yes" Thanks, Jisheng ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] ARM: cpuidle: declare cpuidle_ops __read_mostly 2016-08-10 8:49 [PATCH] ARM: cpuidle: declare cpuidle_ops __read_mostly Jisheng Zhang 2016-08-10 8:57 ` Arnd Bergmann @ 2016-08-10 9:54 ` Jisheng Zhang 2016-08-10 10:49 ` Arnd Bergmann 1 sibling, 1 reply; 7+ messages in thread From: Jisheng Zhang @ 2016-08-10 9:54 UTC (permalink / raw) To: linux-arm-kernel On Wed, 10 Aug 2016 16:49:57 +0800 Jisheng Zhang wrote: > cpuidle_ops is initialized once by arm_cpuidle_read_ops() during > initialization, and thereafter is mostly read in arm_cpuidle_suspend() > > The fact that it is mostly read and not written to makes it candidates > for __read_mostly declarations. > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > --- > arch/arm/kernel/cpuidle.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c > index 7dccc96..762e0929 100644 > --- a/arch/arm/kernel/cpuidle.c > +++ b/arch/arm/kernel/cpuidle.c > @@ -19,7 +19,7 @@ extern struct of_cpuidle_method __cpuidle_method_of_table[]; > static const struct of_cpuidle_method __cpuidle_method_of_table_sentinel > __used __section(__cpuidle_method_of_table_end); > > -static struct cpuidle_ops cpuidle_ops[NR_CPUS]; > +static struct cpuidle_ops cpuidle_ops[NR_CPUS] __read_mostly; oops, I just found that "arm: apply more __ro_after_init" from Kees marked cpuidle_ops as __ro_after_init, that also meet my need, so let's drop my patch. Thanks, Jisheng ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] ARM: cpuidle: declare cpuidle_ops __read_mostly 2016-08-10 9:54 ` Jisheng Zhang @ 2016-08-10 10:49 ` Arnd Bergmann 0 siblings, 0 replies; 7+ messages in thread From: Arnd Bergmann @ 2016-08-10 10:49 UTC (permalink / raw) To: linux-arm-kernel On Wednesday, August 10, 2016 5:54:03 PM CEST Jisheng Zhang wrote: > On Wed, 10 Aug 2016 16:49:57 +0800 Jisheng Zhang wrote: > > > cpuidle_ops is initialized once by arm_cpuidle_read_ops() during > > initialization, and thereafter is mostly read in arm_cpuidle_suspend() > > > > The fact that it is mostly read and not written to makes it candidates > > for __read_mostly declarations. > > > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > > --- > > arch/arm/kernel/cpuidle.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c > > index 7dccc96..762e0929 100644 > > --- a/arch/arm/kernel/cpuidle.c > > +++ b/arch/arm/kernel/cpuidle.c > > @@ -19,7 +19,7 @@ extern struct of_cpuidle_method __cpuidle_method_of_table[]; > > static const struct of_cpuidle_method __cpuidle_method_of_table_sentinel > > __used __section(__cpuidle_method_of_table_end); > > > > -static struct cpuidle_ops cpuidle_ops[NR_CPUS]; > > +static struct cpuidle_ops cpuidle_ops[NR_CPUS] __read_mostly; > > > oops, I just found that "arm: apply more __ro_after_init" from Kees > marked cpuidle_ops as __ro_after_init, that also meet my need, so > let's drop my patch. > > Ok, just saw this reply and that matches up with what I just wrote as well. Thanks, Arnd ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-08-10 11:06 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-08-10 8:49 [PATCH] ARM: cpuidle: declare cpuidle_ops __read_mostly Jisheng Zhang 2016-08-10 8:57 ` Arnd Bergmann 2016-08-10 9:19 ` Jisheng Zhang 2016-08-10 10:47 ` Arnd Bergmann 2016-08-10 11:06 ` Jisheng Zhang 2016-08-10 9:54 ` Jisheng Zhang 2016-08-10 10:49 ` Arnd Bergmann
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).