[PATCH] hugeltb: Mark hugelb_max_hstate __read_mostly

[PATCH] hugeltb: Mark hugelb_max_hstate __read_mostly

[Aneesh Kumar K.V]

From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 include/linux/hugetlb.h |    2 +-
 mm/hugetlb.c            |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 9650bb1..0f0877e 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -23,7 +23,7 @@ struct hugepage_subpool {
 };
 
 extern spinlock_t hugetlb_lock;
-extern int hugetlb_max_hstate;
+extern int hugetlb_max_hstate __read_mostly;
 #define for_each_hstate(h) \
 	for ((h) = hstates; (h) < &hstates[hugetlb_max_hstate]; (h)++)
 
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index a5a30bf..c57740b 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -37,7 +37,7 @@ const unsigned long hugetlb_zero = 0, hugetlb_infinity = ~0UL;
 static gfp_t htlb_alloc_mask = GFP_HIGHUSER;
 unsigned long hugepages_treat_as_movable;
 
-int hugetlb_max_hstate;
+int hugetlb_max_hstate __read_mostly;
 unsigned int default_hstate_idx;
 struct hstate hstates[HUGE_MAX_HSTATE];
 
-- 
1.7.10

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] hugeltb: Mark hugelb_max_hstate __read_mostly

[Michal Hocko]

On Thu 14-06-12 19:26:18, Aneesh Kumar K.V wrote:
> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
>  include/linux/hugetlb.h |    2 +-
>  mm/hugetlb.c            |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 9650bb1..0f0877e 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -23,7 +23,7 @@ struct hugepage_subpool {
>  };
>  
>  extern spinlock_t hugetlb_lock;
> -extern int hugetlb_max_hstate;
> +extern int hugetlb_max_hstate __read_mostly;

It should be used only for definition

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] hugeltb: Mark hugelb_max_hstate __read_mostly

[Christoph Lameter]

On Thu, 14 Jun 2012, Michal Hocko wrote:

> On Thu 14-06-12 19:26:18, Aneesh Kumar K.V wrote:
> > From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> >
> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> > ---
> >  include/linux/hugetlb.h |    2 +-
> >  mm/hugetlb.c            |    2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> > index 9650bb1..0f0877e 100644
> > --- a/include/linux/hugetlb.h
> > +++ b/include/linux/hugetlb.h
> > @@ -23,7 +23,7 @@ struct hugepage_subpool {
> >  };
> >
> >  extern spinlock_t hugetlb_lock;
> > -extern int hugetlb_max_hstate;
> > +extern int hugetlb_max_hstate __read_mostly;
>
> It should be used only for definition

And a rationale needs to be given. Since this patch had no effect, I would
think that the patch is just the expression of the belief of the patcher
that something would improve performancewise.

But there seems to no need for this patch otherwise someone would have
verified that the patch has the intended beneficial effect on performance.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] hugeltb: Mark hugelb_max_hstate __read_mostly

[Aneesh Kumar K.V]

Christoph Lameter <cl@linux.com> writes:

> On Thu, 14 Jun 2012, Michal Hocko wrote:
>
>> On Thu 14-06-12 19:26:18, Aneesh Kumar K.V wrote:
>> > From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>> >
>> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> > ---
>> >  include/linux/hugetlb.h |    2 +-
>> >  mm/hugetlb.c            |    2 +-
>> >  2 files changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
>> > index 9650bb1..0f0877e 100644
>> > --- a/include/linux/hugetlb.h
>> > +++ b/include/linux/hugetlb.h
>> > @@ -23,7 +23,7 @@ struct hugepage_subpool {
>> >  };
>> >
>> >  extern spinlock_t hugetlb_lock;
>> > -extern int hugetlb_max_hstate;
>> > +extern int hugetlb_max_hstate __read_mostly;
>>
>> It should be used only for definition
>
> And a rationale needs to be given. Since this patch had no effect, I would
> think that the patch is just the expression of the belief of the patcher
> that something would improve performancewise.
>
> But there seems to no need for this patch otherwise someone would have
> verified that the patch has the intended beneficial effect on performance.
>

The variable is never modified after boot.

-aneesh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] hugeltb: Mark hugelb_max_hstate __read_mostly

[Aneesh Kumar K.V]

Michal Hocko <mhocko@suse.cz> writes:

> On Thu 14-06-12 19:26:18, Aneesh Kumar K.V wrote:
>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>> 
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> ---
>>  include/linux/hugetlb.h |    2 +-
>>  mm/hugetlb.c            |    2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
>> index 9650bb1..0f0877e 100644
>> --- a/include/linux/hugetlb.h
>> +++ b/include/linux/hugetlb.h
>> @@ -23,7 +23,7 @@ struct hugepage_subpool {
>>  };
>>  
>>  extern spinlock_t hugetlb_lock;
>> -extern int hugetlb_max_hstate;
>> +extern int hugetlb_max_hstate __read_mostly;
>
> It should be used only for definition
>
I looked at the rest of the source and found multiple place where we
specify __read_mostly in extern.

arch/x86/kernel/cpu/perf_event.h extern struct x86_pmu x86_pmu __read_mostly;
arch/x86/kernel/cpu/perf_event.h extern u64 __read_mostly hw_cache_event_ids
arch/x86/kernel/cpu/perf_event.h extern u64 __read_mostly hw_cache_extra_regs

drivers/gpu/drm/i915/i915_drv.h extern int i915_panel_ignore_lid __read_mostly;
drivers/gpu/drm/i915/i915_drv.h extern unsigned int i915_powersave __read_mostly;
drivers/gpu/drm/i915/i915_drv.h extern int i915_semaphores __read_mostly;
drivers/gpu/drm/i915/i915_drv.h extern unsigned int i915_lvds_downclock __read_mostly;
drivers/gpu/drm/i915/i915_drv.h extern int i915_lvds_channel_mode __read_mostly;
drivers/gpu/drm/i915/i915_drv.h extern int i915_panel_use_ssc __read_mostly;
drivers/gpu/drm/i915/i915_drv.h extern int i915_vbt_sdvo_panel_type __read_mostly;
drivers/gpu/drm/i915/i915_drv.h extern int i915_enable_rc6 __read_mostly;
drivers/gpu/drm/i915/i915_drv.h extern int i915_enable_fbc __read_mostly;
drivers/gpu/drm/i915/i915_drv.h extern bool i915_enable_hangcheck __read_mostly;
drivers/gpu/drm/i915/i915_drv.h extern int i915_enable_ppgtt __read_mostly;

-aneesh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] hugeltb: Mark hugelb_max_hstate __read_mostly

[Christoph Lameter]

On Fri, 15 Jun 2012, Aneesh Kumar K.V wrote:

> > But there seems to no need for this patch otherwise someone would have
> > verified that the patch has the intended beneficial effect on performance.
> >
>
> The variable is never modified after boot.

Thats all? There is no performance gain from this change?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] hugeltb: Mark hugelb_max_hstate __read_mostly

[Michal Hocko]

On Fri 15-06-12 08:57:59, Christoph Lameter wrote:
> On Fri, 15 Jun 2012, Aneesh Kumar K.V wrote:
> 
> > > But there seems to no need for this patch otherwise someone would have
> > > verified that the patch has the intended beneficial effect on performance.
> > >
> >
> > The variable is never modified after boot.
> 
> Thats all? There is no performance gain from this change?
 
Is that required in order to put data in the read mostly section?

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] hugeltb: Mark hugelb_max_hstate __read_mostly

[Christoph Lameter]

On Fri, 15 Jun 2012, Michal Hocko wrote:

> > Thats all? There is no performance gain from this change?
>
> Is that required in order to put data in the read mostly section?

I thought so. The read_mostly section is specially designed for data that
causes excessive cacheline bounces and has to be grouped with rarely
accessed other data. That was at least the intend when we created it.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] hugeltb: Mark hugelb_max_hstate __read_mostly

[Andrew Morton]

On Fri, 15 Jun 2012 11:40:24 +0530
"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> wrote:

> Michal Hocko <mhocko@suse.cz> writes:
> 
> > On Thu 14-06-12 19:26:18, Aneesh Kumar K.V wrote:
> >> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> >> 
> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> >> ---
> >>  include/linux/hugetlb.h |    2 +-
> >>  mm/hugetlb.c            |    2 +-
> >>  2 files changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> >> index 9650bb1..0f0877e 100644
> >> --- a/include/linux/hugetlb.h
> >> +++ b/include/linux/hugetlb.h
> >> @@ -23,7 +23,7 @@ struct hugepage_subpool {
> >>  };
> >>  
> >>  extern spinlock_t hugetlb_lock;
> >> -extern int hugetlb_max_hstate;
> >> +extern int hugetlb_max_hstate __read_mostly;
> >
> > It should be used only for definition
> >
> I looked at the rest of the source and found multiple place where we
> specify __read_mostly in extern.
> 
> arch/x86/kernel/cpu/perf_event.h extern struct x86_pmu x86_pmu __read_mostly;

We have had one situation in the past where the lack of a section
annotation on a declaration caused an architecture (arm?) to fail to
build.  iirc the compiler emitted some short-mode relative-addressed
opcode to reference the variable, but when the linker came along to
resolve the offset it discovered that it exceeded the short-mode
addressing range, because that variable was in a section which landed
far away from .data.

That's only happened once, and that architecture might have changed,
and we're missing the section annotation on many variables anyway, so
I'd be inclined to just leave it off - if we ever hit significant
problems with this, we have a lot of work to do.

Also, we currently have no automated way of keeping the annotation on
the declaration and definition in sync.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] hugeltb: Mark hugelb_max_hstate __read_mostly

[Andrew Morton]

On Fri, 15 Jun 2012 09:50:00 -0500 (CDT)
Christoph Lameter <cl@linux.com> wrote:

> On Fri, 15 Jun 2012, Michal Hocko wrote:
> 
> > > Thats all? There is no performance gain from this change?
> >
> > Is that required in order to put data in the read mostly section?
> 
> I thought so. The read_mostly section is specially designed for data that
> causes excessive cacheline bounces and has to be grouped with rarely
> accessed other data. That was at least the intend when we created it.
> 

The __read_mostly thing really is a bit of a crapshoot.  The runtime
effects are extremely dependent upon Kconfig settings and toolchain
behaviour.  I do recall one or two cases where people did fix
real-world observed performance issues by adding __read_mostly.

Literally "one or two".  We have more than one or two __read_mostly
annotations in there!

As that hugelb_max_hstate is write-once, it's a good candidate.  I'll
apply the patch and hope that it improves someone's kernel somewhere
someday.  Shrug.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>