Move x86_cpu_to_apicid to the __read_mostly section

Pls., consider applying this patch series.
It contains the following changes:
- Adds two new macros DEFINE_EARLY_PER_CPU_READ_MOSTLY() and DECLARE_EARLY_PER_CPU_READ_MOSTLY().
- Adds "read-mostly" qualifier to the following variables in smp.h:
- cpu_sibling_map
- cpu_core_map
- cpu_llc_shared_map
- cpu_llc_id
- cpu_number
- x86_cpu_to_apicid
- x86_bios_cpu_apicid
- x86_cpu_to_logical_apicid

As long as all the variables above are only written during the initialization, this change is meant to prevent the false sharing and improve the performance on large multiprocessor systems.

Well, a quick tally of percpu variables on a 'make defconfig' kernel would tell us one way or another?

Here there's almost 200 percpu variables active in the 64-bit x86 defconfig, and a quick random sample suggests that most are read-mostly.

I have no fundamental prefer to either approach, but the direction taken should be justified explicitly, with numbers, arguments, etc. - also a short blurb somewhere in the headers that explains when they should be used, so that others can be aware of vSMP's special needs here.

Quoted 17 lines · Vlad Zolotarov

Pls., consider applying this patch series.

It contains the following changes:
- Adds two new macros DEFINE_EARLY_PER_CPU_READ_MOSTLY() and

DECLARE_EARLY_PER_CPU_READ_MOSTLY().

- Adds "read-mostly" qualifier to the following variables in smp.h:
- cpu_sibling_map
- cpu_core_map
- cpu_llc_shared_map
- cpu_llc_id
- cpu_number
- x86_cpu_to_apicid
- x86_bios_cpu_apicid
- x86_cpu_to_logical_apicid

Quoted 4 lines

As long as all the variables above are only written during the initialization, this change is meant to prevent the false sharing and improve the performance on large multiprocessor systems.

Why have you resent this? The feedback I gave has not been

addressed:

Quoted 7 lines

Well, a quick tally of percpu variables on a 'make defconfig' kernel would tell us one way or another?

Here there's almost 200 percpu variables active in the 64-bit x86 defconfig, and a quick random sample suggests that most are read-mostly.

Quoted 5 lines

I have no fundamental prefer to either approach, but the direction taken should be justified explicitly, with numbers, arguments, etc. - also a short blurb somewhere in the headers that explains when they should be used, so that others can be aware of vSMP's special needs here.

Thanks,

Ingo

Quoted 18 lines · On Monday, May 21, 2012 17:23:48 Ingo Molnar wrote:

Quoted 17 lines · Vlad Zolotarov

Pls., consider applying this patch series.

It contains the following changes:
- Adds two new macros DEFINE_EARLY_PER_CPU_READ_MOSTLY() and

DECLARE_EARLY_PER_CPU_READ_MOSTLY().

- Adds "read-mostly" qualifier to the following variables in smp.h:
- cpu_sibling_map
- cpu_core_map
- cpu_llc_shared_map
- cpu_llc_id
- cpu_number
- x86_cpu_to_apicid
- x86_bios_cpu_apicid
- x86_cpu_to_logical_apicid

Quoted 8 lines

Quoted 4 lines

As long as all the variables above are only written during the initialization, this change is meant to prevent the false sharing and improve the performance on large multiprocessor systems.

Why have you resent this? The feedback I gave has not been

addressed:

Hmmm... I'm a bit confused. There were two feedbacks/threads:
one on "Signed- off-by" format and the other where u asked for a justification on a vSMP side.

The signed-off format sounded to me as a clear blocker for a series so I fixed it and respined. I also mentioned it in patch0.

The second thread seams like getting to submitting a separate patch with a doc under Documents and vSMP testing results explaining and justifying when and were per-CPU and/or __read_mostly variables should be used.

Well, a quick tally of percpu variables on a 'make defconfig' kernel would tell us one way or another?

Here there's almost 200 percpu variables active in the 64-bit x86 defconfig, and a quick random sample suggests that most are read-mostly.

I have no fundamental prefer to either approach, but the direction taken should be justified explicitly, with numbers, arguments, etc. - also a short blurb somewhere in the headers that explains when they should be used, so that others can be aware of vSMP's special needs here.

Quoted 6 lines

I have no fundamental prefer to either approach, but the direction taken should be justified explicitly, with numbers, arguments, etc. - also a short blurb somewhere in the headers that explains when they should be used, so that others can be aware of vSMP's special needs here.

I.e. *numbers* are needed: roughly how many percpu variables in a defconfig of one type versus the other type. This settles the question whether we want to identify read-mostly or write-frequently variables, to address this particular problem
...

I.e. it might make more sense to identify the frequenty modified percpu variables, and move them to a separate section. I think most percpu variables are read mostly, so it would be more maintainable in the long run to figure out the frequently modified ones, not the frequently not modified ones.

Thanks,

Ingo

Quoted 11 lines

Quoted 5 lines

I have no fundamental prefer to either approach, but the direction taken should be justified explicitly, with numbers, arguments, etc. - also a short blurb somewhere in the headers that explains when they should be used, so that others can be aware of vSMP's special needs here.

I.e. *numbers* are needed: roughly how many percpu variables in a defconfig of one type versus the other type. This settles the question whether we want to identify read-mostly or write-frequently variables, to address this particular problem
...

Ok, let's start with *numbers*:
- In the defconfig compilation i've got 219 per_cpu variables: 218 declared as NOT read_mostly and only one (tlb_vector_offset) - as read_mostly. - From those that are not declared as read_mostly the grep on "time|lock|state|stats|last|left|work|owned|reason|list|idle|count|warn|head | rcu|nr_|pvecs|irq", which are a likely candidates to be NOT read_mostly (I verified a few from each pattern) returned 88. I created the above patterns list after walking through about the 1/3 of the per_cpu variables list.

Unfortunately I have no time to analyze all 219 variables in depth - I leave it to the maintainers of the code containing them but it's obvious that there are quite a lot read-write per_cpu variables in the kernel code and u know, I'm not surprised - if u consider the reasoning to declare a per-cpu variable u might notice that in most cases u need it when u actually mean to actively write to it. This is because a main motivation for using per_cpu variables is to hide/prevent from other CPUs seeing the *changes* of the local per_cpu variable: e.g. lists used for lockless stuff, local (per-CPU) locks, counters, etc.

On the other hand declaration of a read_mostly per-cpu variable performance- wise is similar to using a regular read_mostly (*not* per_cpu) array and the main difference is a semantics which I feel like a weaker motivation.

Quoted 5 lines

I.e. it might make more sense to identify the frequenty modified percpu variables, and move them to a separate section. I think most percpu variables are read mostly, so it would be more maintainable in the long run to figure out the frequently modified ones, not the frequently not modified ones.

I guess the numbers above tell us the opposite. So, I think we'd better stick with the read_mostly semantics. ;)

I also would like to draw your attention to the fact that this patch series doesn't introduce the read_mostly semantics either in a per-cpu context or in a non-per-cpu context:
Originally we have discovered that x86_cpu_to_apicid variable has a read_mostly nature and is quite contented and wanted to define it as __read_mostly as it should be in order to prevent false sharing.

More specifically, lapic_events and x86_cpu_to_apicid shared the same cache line and the first one was frequently written.

Since it's defined as an EARLY_PER_CPU variable we had to define the missing XXX_EARLY_PER_CPU_READ_MOSTLY() macros.

Then u asked me to see if other variables from smp.h are also read_mostly, which I did however it wasn't our intention to change any infrastructure, on the contrary we used the existing one. I have a feeling that thought the opposite... ;)

So, pls., tell me what's next? Frankly, I don't think the *numbers* part above is of any interest to the wide public except for u and me... ;) However the "lapic_event" fact above might clarify the motivation a bit more.

Pls., comment.

thanks,
vlad

Quoted 3 lines

Thanks,

Ingo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 6 lines

Quoted 5 lines

I have no fundamental prefer to either approach, but the direction taken should be justified explicitly, with numbers, arguments, etc. - also a short blurb somewhere in the headers that explains when they should be used, so that others can be aware of vSMP's special needs here.

Quoted 5 lines

I.e. *numbers* are needed: roughly how many percpu variables in a defconfig of one type versus the other type. This settles the question whether we want to identify read-mostly or write-frequently variables, to address this particular problem
...

From the analysis of the first 33 per_cpu variables out of 219 (memories they

Quoted 11 lines · On Tuesday, May 22, 2012 18:55:41 Vlad Zolotarov wrote:

Quoted 11 lines

Quoted 5 lines

I have no fundamental prefer to either approach, but the direction taken should be justified explicitly, with numbers, arguments, etc. - also a short blurb somewhere in the headers that explains when they should be used, so that others can be aware of vSMP's special needs here.

I.e. *numbers* are needed: roughly how many percpu variables in a defconfig of one type versus the other type. This settles the question whether we want to identify read-mostly or write-frequently variables, to address this particular problem
...

Ingo, here is the proposal to the patch (series) description:

----------------------------------------------------------------------------
-------------------- Added "read-mostly" qualifier to the following variables in smp.h: - cpu_sibling_map
- cpu_core_map
- cpu_llc_shared_map
- cpu_llc_id
- cpu_number
- x86_cpu_to_apicid
- x86_bios_cpu_apicid
- x86_cpu_to_logical_apicid

As long as all the variables above are only written during the initialization, this change is meant to prevent the false sharing. More specifically, on vSMP Foundation platform x86_cpu_to_apicid shared the same internode_cache_line with frequently written lapic_events.

From the analysis of the first 33 per_cpu variables out of 219 (memories they describe, to be more specific) the 8 have read_mostly nature (tlb_vector_offset, cpu_loops_per_jiffy, xen_debug_irq, etc.) and 25 are frequently written (irq_stack_union, gdt_page, exception_stacks, idt_desc, etc.). Assuming that the spread of the rest of the per_cpu variables is similar, identifying the read mostly memories will make more sense in terms of long-term code maintenance comparing to identifying frequently written memories.
----------------------------------------------------------------------------
-----------------------

Pls., tell me if the above looks satisfactory to u in light of all your previous remarks.

If yes - I'll respin the series with the description above.

thanks,
vlad
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 42 lines · On Wednesday 23 May 2012 12:16:29 Vlad Zolotarov wrote:

Quoted 11 lines · On Tuesday, May 22, 2012 18:55:41 Vlad Zolotarov wrote:

Quoted 11 lines

Quoted 5 lines

I have no fundamental prefer to either approach, but the direction taken should be justified explicitly, with numbers, arguments, etc. - also a short blurb somewhere in the headers that explains when they should be used, so that others can be aware of vSMP's special needs here.

I.e. *numbers* are needed: roughly how many percpu variables in a defconfig of one type versus the other type. This settles the question whether we want to identify read-mostly or write-frequently variables, to address this particular problem
...

Ingo, here is the proposal to the patch (series) description:

----------------------------------------------------------------------------
-------------------- Added "read-mostly" qualifier to the following variables in smp.h: - cpu_sibling_map
- cpu_core_map
- cpu_llc_shared_map
- cpu_llc_id
- cpu_number
- x86_cpu_to_apicid
- x86_bios_cpu_apicid
- x86_cpu_to_logical_apicid

As long as all the variables above are only written during the initialization, this change is meant to prevent the false sharing. More specifically, on vSMP Foundation platform x86_cpu_to_apicid shared the same internode_cache_line with frequently written lapic_events.

From the analysis of the first 33 per_cpu variables out of 219 (memories they describe, to be more specific) the 8 have read_mostly nature (tlb_vector_offset, cpu_loops_per_jiffy, xen_debug_irq, etc.) and 25 are frequently written (irq_stack_union, gdt_page, exception_stacks, idt_desc, etc.). Assuming that the spread of the rest of the per_cpu variables is similar, identifying the read mostly memories will make more sense in terms of long-term code maintenance comparing to identifying frequently written memories.
----------------------------------------------------------------------------
-----------------------

Quoted 4 lines

Pls., tell me if the above looks satisfactory to u in light of all your previous remarks.

If yes - I'll respin the series with the description above.

Ingo, sorry for bothering. Could u, pls., tell if the above description is ok? We'd like to move on with this patch series.

Quoted 54 lines · Vlad Zolotarov

Quoted 52 lines · On Wednesday 23 May 2012 12:16:29 Vlad Zolotarov wrote:

Quoted 11 lines · On Tuesday, May 22, 2012 18:55:41 Vlad Zolotarov wrote:

Quoted 11 lines

Quoted 5 lines

I have no fundamental prefer to either approach, but the direction taken should be justified explicitly, with numbers, arguments, etc. - also a short blurb somewhere in the headers that explains when they should be used, so that others can be aware of vSMP's special needs here.

I.e. *numbers* are needed: roughly how many percpu variables in a defconfig of one type versus the other type. This settles the question whether we want to identify read-mostly or write-frequently variables, to address this particular problem
...

Ingo, here is the proposal to the patch (series) description:

------------------------------------------------------------------------
---- -------------------- Added "read-mostly" qualifier to the following variables in smp.h: - cpu_sibling_map

- cpu_core_map
- cpu_llc_shared_map
- cpu_llc_id
- cpu_number
- x86_cpu_to_apicid
- x86_bios_cpu_apicid
- x86_cpu_to_logical_apicid

As long as all the variables above are only written during the initialization, this change is meant to prevent the false sharing. More specifically, on vSMP Foundation platform x86_cpu_to_apicid shared the same
internode_cache_line with frequently written lapic_events.

From the analysis of the first 33 per_cpu variables out of 219 (memories
they describe, to be more specific) the 8 have read_mostly nature (tlb_vector_offset, cpu_loops_per_jiffy, xen_debug_irq, etc.) and 25 are frequently written (irq_stack_union, gdt_page, exception_stacks, idt_desc,
etc.). Assuming that the spread of the rest of the per_cpu variables is similar, identifying the read mostly memories will make more sense in terms
of long-term code maintenance comparing to identifying frequently written
memories.
------------------------------------------------------------------------
---- -----------------------

Pls., tell me if the above looks satisfactory to u in light of all your previous remarks.

If yes - I'll respin the series with the description above.

Quoted 3 lines

Ingo, sorry for bothering. Could u, pls., tell if the above description is ok? We'd like to move on with this patch series.

Yeah, that description and analysis looks good and sensible.

Mind resending the updated patches in a new thread?

Thanks,

Ingo