This patchset create the infrastructure to handle the CR based
local_* atomic operations. Local atomic operations are fast
and highly reentrant per CPU counters. Used for percpu
variable updates. Local atomic operations only guarantee
variable modification atomicity wrt the CPU which owns the
data and these needs to be executed in a preemption safe way.
Here is the design of the first patch. Since local_* operations
are only need to be atomic to interrupts (IIUC), patch uses
one of the Condition Register (CR) fields as a flag variable. When
entering the local_*, specific bit in the CR5 field is set
and on exit, bit is cleared. CR bit checking is done in the
interrupt return path. If CR5[EQ] bit set and if we return
to kernel, we reset to start of local_* operation.
Reason for this approach is that, currently l[w/d]arx/st[w/d]cx.
instruction pair is used for local_* operations, which are heavy
on cycle count and they dont support a local varient. So to
see whether the new implementation helps, Used a modified
version of Rusty's benchmark code on local_t. Have the
performance numbers in the patch commit message.
Second patch has the rewrite of the local_* functions to use
CR5 based logic. Changes are mostly in asm/local.h and only for
CONFIG_PPC64
Madhavan Srinivasan (2):
powerpc: foundation code to handle CR5 for local_t
powerpc: rewrite local_* to use CR5 flag
Makefile | 6 +
arch/powerpc/include/asm/exception-64s.h | 21 ++-
arch/powerpc/include/asm/local.h | 306 +++++++++++++++++++++++++++++++
arch/powerpc/kernel/entry_64.S | 106 ++++++++++-
arch/powerpc/kernel/exceptions-64s.S | 2 +-
arch/powerpc/kernel/head_64.S | 8 +
6 files changed, 444 insertions(+), 5 deletions(-)
--
1.9.1
This patch create the infrastructure to handle the CR based
local_* atomic operations. Local atomic operations are fast
and highly reentrant per CPU counters. Used for percpu
variable updates. Local atomic operations only guarantee
variable modification atomicity wrt the CPU which owns the
data and these needs to be executed in a preemption safe way.
Here is the design of this patch. Since local_* operations
are only need to be atomic to interrupts (IIUC), patch uses
one of the Condition Register (CR) fields as a flag variable. When
entering the local_*, specific bit in the CR5 field is set
and on exit, bit is cleared. CR bit checking is done in the
interrupt return path. If CR5[EQ] bit set and if we return
to kernel, we reset to start of local_* operation.
Reason for this approach is that, currently l[w/d]arx/st[w/d]cx.
instruction pair is used for local_* operations, which are heavy
on cycle count and they dont support a local variant. So to
see whether the new implementation helps, used a modified
version of Rusty's benchmark code on local_t.
https://lkml.org/lkml/2008/12/16/450
Modifications:
- increated the working set size from 1MB to 8MB,
- removed cpu_local_inc test.
Test ran
- on POWER8 1S Scale out System 2.0GHz
- on OPAL v3 with v3.18-rc4 patch kernel as Host
Here are the values with the patch.
Time in ns per iteration
inc add read add_return
atomic_long 67 67 18 69
irqsave/rest 39 39 23 39
trivalue 39 39 29 49
local_t 26 26 24 26
Since CR5 is used as a flag, have added CFLAGS to avoid CR5
for the kernel compilation and CR5 is zeroed at the kernel
entry.
Tested the patch in a
- pSeries LPAR,
- Host with patched/unmodified guest kernel
To check whether userspace see any CR5 corruption, ran a simple
test which does,
- set CR5 field,
- while(1)
- sleep or gettimeofday
- chk bit set
Signed-off-by: Madhavan Srinivasan <redacted>
---
- I really appreciate feedback on the patchset.
- Kindly comment if I should try with any other benchmark or
workload to check the numbers.
- Also, kindly recommand any know stress test for CR
Makefile | 6 ++
arch/powerpc/include/asm/exception-64s.h | 21 +++++-
arch/powerpc/kernel/entry_64.S | 106 ++++++++++++++++++++++++++++++-
arch/powerpc/kernel/exceptions-64s.S | 2 +-
arch/powerpc/kernel/head_64.S | 8 +++
5 files changed, 138 insertions(+), 5 deletions(-)
@@ -706,6 +706,12 @@ endifKBUILD_CFLAGS+=$(callcc-option,-fno-var-tracking-assignments)+ifdef CONFIG_PPC64+# We need this flag to force compiler not to use CR5, since+# local_t type code is based on this.+KBUILD_CFLAGS+=-ffixed-cr5+endif+ifdef CONFIG_DEBUG_INFOifdef CONFIG_DEBUG_INFO_SPLITKBUILD_CFLAGS+=$(callcc-option,-gsplit-dwarf,-g)
@@ -306,7 +306,26 @@ do_kvm_##n: \stdr10,0(r1);/* make stack chain pointer */\stdr0,GPR0(r1);/* save r0 in stackframe */\stdr10,GPR1(r1);/* save r1 in stackframe */\-beq4f;/* if from kernel mode */\+BEGIN_FTR_SECTION;\+lisr9,4096;/* Create a mask with HV and PR */\+rldicrr9,r9,32,31;/* bits, AND with the MSR */\+mrr10,r9;/* to check for Hyp state */\+orir9,r9,16384;\+andr9,r12,r9;\+cmpdcr3,r10,r9;\+beqcr3,66f;/* Jump if we come from Hyp mode*/\+mtcrf0x04,r10;/* Clear CR5 if coming from usr */\+FTR_SECTION_ELSE;\+beq4f;/* if kernel mode branch */\+lir10,0;/* Clear CR5 incase of coming */\+mtcrf0x04,r10;/* from user. */\+nop;/* This part of code is for */\+nop;/* kernel with MSR[HV]=0, */\+nop;/* MSR[PR]=0, so just chk for */\+nop;/* MSR[PR] */\+nop;\+ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE);\+66:beq4f;/* if from kernel mode */\ACCOUNT_CPU_USER_ENTRY(r9,r10);\SAVE_PPR(area,r9,r10);\4:EXCEPTION_PROLOG_COMMON_2(area)\
This patch re-write the current local_* functions to CR5 based one.
Base flow for each function is
{
set cr5(eq)
load
..
store
clear cr5(eq)
}
Above set of instructions are followed by a fixup section which points
to the entry of the function incase of interrupt in the flow. If the
interrupt happens to be after the store, we just continue to last
instruction in that block.
Currently only asm/local.h has been rewrite, and local64 is TODO.
Also the entire change is only for PPC64.
Signed-off-by: Madhavan Srinivasan <redacted>
---
arch/powerpc/include/asm/local.h | 306 +++++++++++++++++++++++++++++++++++++++
1 file changed, 306 insertions(+)
@@ -162,6 +466,8 @@ static __inline__ long local_dec_if_positive(local_t *l)returnt;}+#endif+/* Use these for per-cpu local_t variables: on some archs they are*muchmoreefficientthanthesenaiveimplementations.Notetheytake*avariable,notanaddress.
On Thu, Nov 27, 2014 at 05:48:40PM +0530, Madhavan Srinivasan wrote:
Here is the design of this patch. Since local_* operations
are only need to be atomic to interrupts (IIUC), patch uses
one of the Condition Register (CR) fields as a flag variable. When
entering the local_*, specific bit in the CR5 field is set
and on exit, bit is cleared. CR bit checking is done in the
interrupt return path. If CR5[EQ] bit set and if we return
to kernel, we reset to start of local_* operation.
Have you tested this with (upcoming) GCC 5.0? GCC now uses CR5,
and it likes to use it very much, it might be more convenient to
use e.g. CR1 (which is allocated almost last, only before CR0).
@@ -306,7 +306,26 @@ do_kvm_##n: \stdr10,0(r1);/* make stack chain pointer */\stdr0,GPR0(r1);/* save r0 in stackframe */\stdr10,GPR1(r1);/* save r1 in stackframe */\-beq4f;/* if from kernel mode */\+BEGIN_FTR_SECTION;\+lisr9,4096;/* Create a mask with HV and PR */\+rldicrr9,r9,32,31;/* bits, AND with the MSR */\+mrr10,r9;/* to check for Hyp state */\+orir9,r9,16384;\+andr9,r12,r9;\+cmpdcr3,r10,r9;\+beqcr3,66f;/* Jump if we come from Hyp mode*/\+mtcrf0x04,r10;/* Clear CR5 if coming from usr */\
Wow, such nastiness, only to avoid using dot insns (since you need to keep
the current CR0 value for the following beq 4f). And CR0 already holds the
PR bit, so you need only to check the HV bit anyway? Some restructuring
would make this a lot simpler and clearer.
quoted hunk
+ /*+ * Now that we are about to exit from interrupt, lets check for+ * cr5 eq bit. If it is set, then we may be in the middle of+ * local_t update. In this case, we should rewind the NIP+ * accordingly.+ */+ mfcr r3+ andi. r4,r3,0x200+ beq 63f
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2014-11-28 00:56:57
On Thu, 2014-11-27 at 17:48 +0530, Madhavan Srinivasan wrote:
quoted hunk
This patch create the infrastructure to handle the CR based
local_* atomic operations. Local atomic operations are fast
and highly reentrant per CPU counters. Used for percpu
variable updates. Local atomic operations only guarantee
variable modification atomicity wrt the CPU which owns the
data and these needs to be executed in a preemption safe way.
Here is the design of this patch. Since local_* operations
are only need to be atomic to interrupts (IIUC), patch uses
one of the Condition Register (CR) fields as a flag variable. When
entering the local_*, specific bit in the CR5 field is set
and on exit, bit is cleared. CR bit checking is done in the
interrupt return path. If CR5[EQ] bit set and if we return
to kernel, we reset to start of local_* operation.
Reason for this approach is that, currently l[w/d]arx/st[w/d]cx.
instruction pair is used for local_* operations, which are heavy
on cycle count and they dont support a local variant. So to
see whether the new implementation helps, used a modified
version of Rusty's benchmark code on local_t.
https://lkml.org/lkml/2008/12/16/450
Modifications:
- increated the working set size from 1MB to 8MB,
- removed cpu_local_inc test.
Test ran
- on POWER8 1S Scale out System 2.0GHz
- on OPAL v3 with v3.18-rc4 patch kernel as Host
Here are the values with the patch.
Time in ns per iteration
inc add read add_return
atomic_long 67 67 18 69
irqsave/rest 39 39 23 39
trivalue 39 39 29 49
local_t 26 26 24 26
Since CR5 is used as a flag, have added CFLAGS to avoid CR5
for the kernel compilation and CR5 is zeroed at the kernel
entry.
Tested the patch in a
- pSeries LPAR,
- Host with patched/unmodified guest kernel
To check whether userspace see any CR5 corruption, ran a simple
test which does,
- set CR5 field,
- while(1)
- sleep or gettimeofday
- chk bit set
Signed-off-by: Madhavan Srinivasan <redacted>
---
- I really appreciate feedback on the patchset.
- Kindly comment if I should try with any other benchmark or
workload to check the numbers.
- Also, kindly recommand any know stress test for CR
Makefile | 6 ++
arch/powerpc/include/asm/exception-64s.h | 21 +++++-
arch/powerpc/kernel/entry_64.S | 106 ++++++++++++++++++++++++++++++-
arch/powerpc/kernel/exceptions-64s.S | 2 +-
arch/powerpc/kernel/head_64.S | 8 +++
5 files changed, 138 insertions(+), 5 deletions(-)
@@ -706,6 +706,12 @@ endifKBUILD_CFLAGS+=$(callcc-option,-fno-var-tracking-assignments)+ifdef CONFIG_PPC64+# We need this flag to force compiler not to use CR5, since+# local_t type code is based on this.+KBUILD_CFLAGS+=-ffixed-cr5+endif+ifdef CONFIG_DEBUG_INFOifdef CONFIG_DEBUG_INFO_SPLITKBUILD_CFLAGS+=$(callcc-option,-gsplit-dwarf,-g)
@@ -306,7 +306,26 @@ do_kvm_##n: \stdr10,0(r1);/* make stack chain pointer */\stdr0,GPR0(r1);/* save r0 in stackframe */\stdr10,GPR1(r1);/* save r1 in stackframe */\-beq4f;/* if from kernel mode */\+BEGIN_FTR_SECTION;\+lisr9,4096;/* Create a mask with HV and PR */\+rldicrr9,r9,32,31;/* bits, AND with the MSR */\+mrr10,r9;/* to check for Hyp state */\+orir9,r9,16384;\+andr9,r12,r9;\+cmpdcr3,r10,r9;\+beqcr3,66f;/* Jump if we come from Hyp mode*/\+mtcrf0x04,r10;/* Clear CR5 if coming from usr */\+FTR_SECTION_ELSE;\
Can't we just unconditionally clear at as long as we do that after we've
saved it ? In that case, it's just a matter for the fixup code to check
the saved version rather than the actual CR..
quoted hunk
+ beq 4f; /* if kernel mode branch */ \+ li r10,0; /* Clear CR5 incase of coming */ \+ mtcrf 0x04,r10; /* from user. */ \+ nop; /* This part of code is for */ \+ nop; /* kernel with MSR[HV]=0, */ \+ nop; /* MSR[PR]=0, so just chk for */ \+ nop; /* MSR[PR] */ \+ nop; \+ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE); \+66: beq 4f; /* if from kernel mode */ \ ACCOUNT_CPU_USER_ENTRY(r9, r10); \ SAVE_PPR(area, r9, r10); \ 4: EXCEPTION_PROLOG_COMMON_2(area) \
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2014-11-28 01:59:26
On Thu, 2014-11-27 at 10:56 -0600, Segher Boessenkool wrote:
On Thu, Nov 27, 2014 at 05:48:40PM +0530, Madhavan Srinivasan wrote:
quoted
Here is the design of this patch. Since local_* operations
are only need to be atomic to interrupts (IIUC), patch uses
one of the Condition Register (CR) fields as a flag variable. When
entering the local_*, specific bit in the CR5 field is set
and on exit, bit is cleared. CR bit checking is done in the
interrupt return path. If CR5[EQ] bit set and if we return
to kernel, we reset to start of local_* operation.
Have you tested this with (upcoming) GCC 5.0? GCC now uses CR5,
and it likes to use it very much, it might be more convenient to
use e.g. CR1 (which is allocated almost last, only before CR0).
We use CR1 all over the place in your asm code. Any other suggestion ?
What's the damage of -ffixed-cr5 on gcc5 ? won't it just use CR4 or 6
instead ?
@@ -306,7 +306,26 @@ do_kvm_##n: \stdr10,0(r1);/* make stack chain pointer */\stdr0,GPR0(r1);/* save r0 in stackframe */\stdr10,GPR1(r1);/* save r1 in stackframe */\-beq4f;/* if from kernel mode */\+BEGIN_FTR_SECTION;\+lisr9,4096;/* Create a mask with HV and PR */\+rldicrr9,r9,32,31;/* bits, AND with the MSR */\+mrr10,r9;/* to check for Hyp state */\+orir9,r9,16384;\+andr9,r12,r9;\+cmpdcr3,r10,r9;\+beqcr3,66f;/* Jump if we come from Hyp mode*/\+mtcrf0x04,r10;/* Clear CR5 if coming from usr */\
Wow, such nastiness, only to avoid using dot insns (since you need to keep
the current CR0 value for the following beq 4f). And CR0 already holds the
PR bit, so you need only to check the HV bit anyway? Some restructuring
would make this a lot simpler and clearer.
quoted
+ /*+ * Now that we are about to exit from interrupt, lets check for+ * cr5 eq bit. If it is set, then we may be in the middle of+ * local_t update. In this case, we should rewind the NIP+ * accordingly.+ */+ mfcr r3+ andi. r4,r3,0x200+ beq 63f
On Thursday 27 November 2014 10:26 PM, Segher Boessenkool wrote:
On Thu, Nov 27, 2014 at 05:48:40PM +0530, Madhavan Srinivasan wrote:
quoted
Here is the design of this patch. Since local_* operations
are only need to be atomic to interrupts (IIUC), patch uses
one of the Condition Register (CR) fields as a flag variable. When
entering the local_*, specific bit in the CR5 field is set
and on exit, bit is cleared. CR bit checking is done in the
interrupt return path. If CR5[EQ] bit set and if we return
to kernel, we reset to start of local_* operation.
Have you tested this with (upcoming) GCC 5.0? GCC now uses CR5,
and it likes to use it very much, it might be more convenient to
use e.g. CR1 (which is allocated almost last, only before CR0).
No. I did not try it with GCC5.0 But I did force kernel compilation with
fixed-cr5 which should make GCC avoid using CR5. But i will try that today.
@@ -306,7 +306,26 @@ do_kvm_##n: \stdr10,0(r1);/* make stack chain pointer */\stdr0,GPR0(r1);/* save r0 in stackframe */\stdr10,GPR1(r1);/* save r1 in stackframe */\-beq4f;/* if from kernel mode */\+BEGIN_FTR_SECTION;\+lisr9,4096;/* Create a mask with HV and PR */\+rldicrr9,r9,32,31;/* bits, AND with the MSR */\+mrr10,r9;/* to check for Hyp state */\+orir9,r9,16384;\+andr9,r12,r9;\+cmpdcr3,r10,r9;\+beqcr3,66f;/* Jump if we come from Hyp mode*/\+mtcrf0x04,r10;/* Clear CR5 if coming from usr */\
Wow, such nastiness, only to avoid using dot insns (since you need to keep
the current CR0 value for the following beq 4f). And CR0 already holds the
PR bit, so you need only to check the HV bit anyway? Some restructuring
would make this a lot simpler and clearer.
Ok I can try that.
quoted
+ /*+ * Now that we are about to exit from interrupt, lets check for+ * cr5 eq bit. If it is set, then we may be in the middle of+ * local_t update. In this case, we should rewind the NIP+ * accordingly.+ */+ mfcr r3+ andi. r4,r3,0x200+ beq 63f
On Friday 28 November 2014 07:28 AM, Benjamin Herrenschmidt wrote:
On Thu, 2014-11-27 at 10:56 -0600, Segher Boessenkool wrote:
quoted
On Thu, Nov 27, 2014 at 05:48:40PM +0530, Madhavan Srinivasan wrote:
quoted
Here is the design of this patch. Since local_* operations
are only need to be atomic to interrupts (IIUC), patch uses
one of the Condition Register (CR) fields as a flag variable. When
entering the local_*, specific bit in the CR5 field is set
and on exit, bit is cleared. CR bit checking is done in the
interrupt return path. If CR5[EQ] bit set and if we return
to kernel, we reset to start of local_* operation.
Have you tested this with (upcoming) GCC 5.0? GCC now uses CR5,
and it likes to use it very much, it might be more convenient to
use e.g. CR1 (which is allocated almost last, only before CR0).
We use CR1 all over the place in your asm code. Any other suggestion ?
Yes. CR1 is used in many places and so do CR7. And CR0 are alway used
for dot instn. And I guess Vector instructions use CR6.
What's the damage of -ffixed-cr5 on gcc5 ? won't it just use CR4 or 6
instead ?
@@ -306,7 +306,26 @@ do_kvm_##n: \stdr10,0(r1);/* make stack chain pointer */\stdr0,GPR0(r1);/* save r0 in stackframe */\stdr10,GPR1(r1);/* save r1 in stackframe */\-beq4f;/* if from kernel mode */\+BEGIN_FTR_SECTION;\+lisr9,4096;/* Create a mask with HV and PR */\+rldicrr9,r9,32,31;/* bits, AND with the MSR */\+mrr10,r9;/* to check for Hyp state */\+orir9,r9,16384;\+andr9,r12,r9;\+cmpdcr3,r10,r9;\+beqcr3,66f;/* Jump if we come from Hyp mode*/\+mtcrf0x04,r10;/* Clear CR5 if coming from usr */\
Wow, such nastiness, only to avoid using dot insns (since you need to keep
the current CR0 value for the following beq 4f). And CR0 already holds the
PR bit, so you need only to check the HV bit anyway? Some restructuring
would make this a lot simpler and clearer.
quoted
+ /*+ * Now that we are about to exit from interrupt, lets check for+ * cr5 eq bit. If it is set, then we may be in the middle of+ * local_t update. In this case, we should rewind the NIP+ * accordingly.+ */+ mfcr r3+ andi. r4,r3,0x200+ beq 63f
On Friday 28 November 2014 06:26 AM, Benjamin Herrenschmidt wrote:
On Thu, 2014-11-27 at 17:48 +0530, Madhavan Srinivasan wrote:
quoted
This patch create the infrastructure to handle the CR based
local_* atomic operations. Local atomic operations are fast
and highly reentrant per CPU counters. Used for percpu
variable updates. Local atomic operations only guarantee
variable modification atomicity wrt the CPU which owns the
data and these needs to be executed in a preemption safe way.
Here is the design of this patch. Since local_* operations
are only need to be atomic to interrupts (IIUC), patch uses
one of the Condition Register (CR) fields as a flag variable. When
entering the local_*, specific bit in the CR5 field is set
and on exit, bit is cleared. CR bit checking is done in the
interrupt return path. If CR5[EQ] bit set and if we return
to kernel, we reset to start of local_* operation.
Reason for this approach is that, currently l[w/d]arx/st[w/d]cx.
instruction pair is used for local_* operations, which are heavy
on cycle count and they dont support a local variant. So to
see whether the new implementation helps, used a modified
version of Rusty's benchmark code on local_t.
https://lkml.org/lkml/2008/12/16/450
Modifications:
- increated the working set size from 1MB to 8MB,
- removed cpu_local_inc test.
Test ran
- on POWER8 1S Scale out System 2.0GHz
- on OPAL v3 with v3.18-rc4 patch kernel as Host
Here are the values with the patch.
Time in ns per iteration
inc add read add_return
atomic_long 67 67 18 69
irqsave/rest 39 39 23 39
trivalue 39 39 29 49
local_t 26 26 24 26
Since CR5 is used as a flag, have added CFLAGS to avoid CR5
for the kernel compilation and CR5 is zeroed at the kernel
entry.
Tested the patch in a
- pSeries LPAR,
- Host with patched/unmodified guest kernel
To check whether userspace see any CR5 corruption, ran a simple
test which does,
- set CR5 field,
- while(1)
- sleep or gettimeofday
- chk bit set
Signed-off-by: Madhavan Srinivasan <redacted>
---
- I really appreciate feedback on the patchset.
- Kindly comment if I should try with any other benchmark or
workload to check the numbers.
- Also, kindly recommand any know stress test for CR
Makefile | 6 ++
arch/powerpc/include/asm/exception-64s.h | 21 +++++-
arch/powerpc/kernel/entry_64.S | 106 ++++++++++++++++++++++++++++++-
arch/powerpc/kernel/exceptions-64s.S | 2 +-
arch/powerpc/kernel/head_64.S | 8 +++
5 files changed, 138 insertions(+), 5 deletions(-)
@@ -706,6 +706,12 @@ endifKBUILD_CFLAGS+=$(callcc-option,-fno-var-tracking-assignments)+ifdef CONFIG_PPC64+# We need this flag to force compiler not to use CR5, since+# local_t type code is based on this.+KBUILD_CFLAGS+=-ffixed-cr5+endif+ifdef CONFIG_DEBUG_INFOifdef CONFIG_DEBUG_INFO_SPLITKBUILD_CFLAGS+=$(callcc-option,-gsplit-dwarf,-g)
@@ -306,7 +306,26 @@ do_kvm_##n: \stdr10,0(r1);/* make stack chain pointer */\stdr0,GPR0(r1);/* save r0 in stackframe */\stdr10,GPR1(r1);/* save r1 in stackframe */\-beq4f;/* if from kernel mode */\+BEGIN_FTR_SECTION;\+lisr9,4096;/* Create a mask with HV and PR */\+rldicrr9,r9,32,31;/* bits, AND with the MSR */\+mrr10,r9;/* to check for Hyp state */\+orir9,r9,16384;\+andr9,r12,r9;\+cmpdcr3,r10,r9;\+beqcr3,66f;/* Jump if we come from Hyp mode*/\+mtcrf0x04,r10;/* Clear CR5 if coming from usr */\+FTR_SECTION_ELSE;\
Can't we just unconditionally clear at as long as we do that after we've
saved it ? In that case, it's just a matter for the fixup code to check
the saved version rather than the actual CR..
I use CR bit setting in the interrupt return path to enter the fixup
section search. If we unconditionally clear it, we will have to enter
the fixup section for every kernel return nip right?
Regards
Maddy
quoted
+ beq 4f; /* if kernel mode branch */ \+ li r10,0; /* Clear CR5 incase of coming */ \+ mtcrf 0x04,r10; /* from user. */ \+ nop; /* This part of code is for */ \+ nop; /* kernel with MSR[HV]=0, */ \+ nop; /* MSR[PR]=0, so just chk for */ \+ nop; /* MSR[PR] */ \+ nop; \+ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE); \+66: beq 4f; /* if from kernel mode */ \ ACCOUNT_CPU_USER_ENTRY(r9, r10); \ SAVE_PPR(area, r9, r10); \ 4: EXCEPTION_PROLOG_COMMON_2(area) \
From: Benjamin Herrenschmidt <hidden> Date: 2014-11-28 03:22:16
On Fri, 2014-11-28 at 08:45 +0530, Madhavan Srinivasan wrote:
quoted
Can't we just unconditionally clear at as long as we do that after we've
saved it ? In that case, it's just a matter for the fixup code to check
the saved version rather than the actual CR..
I use CR bit setting in the interrupt return path to enter the fixup
section search. If we unconditionally clear it, we will have to enter
the fixup section for every kernel return nip right?
As I said above. Can't we look at the saved version ?
IE.
- On interrupt entry:
* Save CR to CCR(r1)
* clear CR5
- On exit
* Check CCR(r1)'s CR5 field
* restore CR
Cheers,
Ben.
On Thursday 27 November 2014 07:35 PM, David Laight wrote:
From: Madhavan Srinivasan
quoted
This patchset create the infrastructure to handle the CR based
local_* atomic operations. Local atomic operations are fast
and highly reentrant per CPU counters. Used for percpu
variable updates. Local atomic operations only guarantee
variable modification atomicity wrt the CPU which owns the
data and these needs to be executed in a preemption safe way.
These are usually called 'restartable atomic sequences (RAS)'.
quoted
Here is the design of the first patch. Since local_* operations
are only need to be atomic to interrupts (IIUC), patch uses
one of the Condition Register (CR) fields as a flag variable. When
entering the local_*, specific bit in the CR5 field is set
and on exit, bit is cleared. CR bit checking is done in the
interrupt return path. If CR5[EQ] bit set and if we return
to kernel, we reset to start of local_* operation.
I don't claim to be able to read ppc assembler.
But I can't see the code that clears CR5[EQ] for the duration
of the ISR.
I use crclr instruction at the end of the code block to clear the bit.
Without it a nested interrupt will go through unwanted paths.
There are also a lot of 'magic' constants in that assembly code.
All these constants are define in asm/ppc-opcode.h
I also wonder if it is possible to inspect the interrupted
code to determine the start/end of the RAS block.
(Easiest if you assume that there is a single 'write' instruction
as the last entry in the block.)
So each local_* function also have code in the __ex_table section. IIUC,
__ex_table contains two address. So if the return address found in the
first column of the _ex_table, use the corresponding address in the
second column to continue from.
Also, how expensive is it to disable all interrupts?
In the patch 1/2, posted the numbers for that too.
On Fri, Nov 28, 2014 at 12:58:55PM +1100, Benjamin Herrenschmidt wrote:
quoted
Have you tested this with (upcoming) GCC 5.0? GCC now uses CR5,
and it likes to use it very much, it might be more convenient to
use e.g. CR1 (which is allocated almost last, only before CR0).
We use CR1 all over the place in your asm code. Any other suggestion ?
What's the damage of -ffixed-cr5 on gcc5 ? won't it just use CR4 or 6
instead ?
Oh, it will work fine. Not using CR5 would be more convenient so that
the register allocation for most code would not change when you use or
not use -ffixed-cr5, making code easier to read. But your point about
asm code already using the other CR fields makes CR5 a better choice
actually, because people avoided it (because the compiler did) :-)
Segher
On Fri, Nov 28, 2014 at 08:27:22AM +0530, Madhavan Srinivasan wrote:
quoted
Have you tested this with (upcoming) GCC 5.0? GCC now uses CR5,
and it likes to use it very much, it might be more convenient to
use e.g. CR1 (which is allocated almost last, only before CR0).
No. I did not try it with GCC5.0 But I did force kernel compilation with
fixed-cr5 which should make GCC avoid using CR5. But i will try that today.
It should work just fine, but testing is always useful, because, you know,
"should". Thanks.
Segher
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2014-11-30 09:02:17
On Fri, 2014-11-28 at 10:53 +0000, David Laight wrote:
From: Benjamin Herrenschmidt
quoted
On Fri, 2014-11-28 at 08:45 +0530, Madhavan Srinivasan wrote:
quoted
quoted
Can't we just unconditionally clear at as long as we do that after we've
saved it ? In that case, it's just a matter for the fixup code to check
the saved version rather than the actual CR..
I use CR bit setting in the interrupt return path to enter the fixup
section search. If we unconditionally clear it, we will have to enter
the fixup section for every kernel return nip right?
As I said above. Can't we look at the saved version ?
IE.
- On interrupt entry:
* Save CR to CCR(r1)
* clear CR5
- On exit
* Check CCR(r1)'s CR5 field
* restore CR
Actually there is no real reason why the 'fixup' can't be done
during interrupt entry.
Other than if we crash, we get the wrong PC in the log etc... unlikely
but I tend to prefer this. Also if we ever allow something like a local
atomic on a faulting (uesrspace) address, we want a precise PC on entry.
Generally, we have a lot more entry path than exit path, it's easier to
keep the entry path simpler.
Cheers,
Ben.
On Friday 28 November 2014 03:39 PM, David Laight wrote:
From: Madhavan Srinivasan
quoted
On Thursday 27 November 2014 07:35 PM, David Laight wrote:
quoted
From: Madhavan Srinivasan
quoted
This patchset create the infrastructure to handle the CR based
local_* atomic operations. Local atomic operations are fast
and highly reentrant per CPU counters. Used for percpu
variable updates. Local atomic operations only guarantee
variable modification atomicity wrt the CPU which owns the
data and these needs to be executed in a preemption safe way.
These are usually called 'restartable atomic sequences (RAS)'.
quoted
Here is the design of the first patch. Since local_* operations
are only need to be atomic to interrupts (IIUC), patch uses
one of the Condition Register (CR) fields as a flag variable. When
entering the local_*, specific bit in the CR5 field is set
and on exit, bit is cleared. CR bit checking is done in the
interrupt return path. If CR5[EQ] bit set and if we return
to kernel, we reset to start of local_* operation.
I don't claim to be able to read ppc assembler.
But I can't see the code that clears CR5[EQ] for the duration
of the ISR.
I use crclr instruction at the end of the code block to clear the bit.
quoted
Without it a nested interrupt will go through unwanted paths.
That crclr looks to be in the ISR exit path, you need one in the
isr entry path.
In case of entry path, i clear the field using mtcr instruction
quoted
quoted
There are also a lot of 'magic' constants in that assembly code.
All these constants are define in asm/ppc-opcode.h
I was thinking of the lines like:
+ ori r3,r3,16384
ok sure. Will comment
This one probably deserves a comment - or something
+"3:" PPC405_ERR77(0,%2)
quoted
quoted
I also wonder if it is possible to inspect the interrupted
code to determine the start/end of the RAS block.
(Easiest if you assume that there is a single 'write' instruction
as the last entry in the block.)
So each local_* function also have code in the __ex_table section. IIUC,
__ex_table contains two address. So if the return address found in the
first column of the _ex_table, use the corresponding address in the
second column to continue from.
That really doesn't scale.
I don't know how many 1000 address pairs you table will have (and the
ones in each loadable module), but the search isn't going to be cheap.
If these sequences are restartable then they can only have one write
to memory.
May be, but i see these issues incase of insts decode path,
1) Decoding instruction may also cause a fault (in case of module) and
handling a fault at this stage toward the exit path of interrupt exit
makes me nervous
2) resulting code with lot of condition and branch (for opcode decode)
will be lot messy and may be an issue incase of maintenance,
but let me think it over again on this.
Regards
Maddy
Given your:
quoted
This patch re-write the current local_* functions to CR5 based one.
Base flow for each function is
{
set cr5(eq)
load
..
store
clear cr5(eq)
}
On ISR entry:
If an ISR detects cr5(eq) set then look at the returned to instruction.
If it is 'clear cr5(eq)' do nothing.
Otherwise read backwards through the code (for a max of (say) 16 instructions)
searching for the 'set cr5(eq)' and change the return address to be that
of the instruction following the 'set cr5(eq)'.
In all cases clear cr5(eq) for the ISR itself (leave the saved value unchanged).
The you don't need a table of fault locations.
David
From: Gabriel Paubert <hidden> Date: 2014-12-01 18:35:13
On Thu, Nov 27, 2014 at 05:48:41PM +0530, Madhavan Srinivasan wrote:
quoted hunk
This patch re-write the current local_* functions to CR5 based one.
Base flow for each function is
{
set cr5(eq)
load
..
store
clear cr5(eq)
}
Above set of instructions are followed by a fixup section which points
to the entry of the function incase of interrupt in the flow. If the
interrupt happens to be after the store, we just continue to last
instruction in that block.
Currently only asm/local.h has been rewrite, and local64 is TODO.
Also the entire change is only for PPC64.
Signed-off-by: Madhavan Srinivasan <redacted>
---
arch/powerpc/include/asm/local.h | 306 +++++++++++++++++++++++++++++++++++++++
1 file changed, 306 insertions(+)
Apart from the other comments on bloat which can very likely
be removed by tracing backwards for a few instructions, removing
the exception table entries which are 2 or 4 (64 bit?) times as
large as the instruction sequence, I don't understand at all why
you need these sequences for the local_read and local_set functions.
After all these are single instructions (why do you perform a read before
the write in set when the result of the read is never used?).
I believe read and set are better mapped to access_once (or assign_once
or whatever it's called after the recent discussion on linux-kernel).
You don't even need a memory barrier if it's for a single thread,
so you could get away with a single volatile access to the variables.
For the other ones, I think that what you do is correct, except that
the workaround for PPC405 erratum 77 is not needed since this erratum
only affects the stwcx. instruction and the whole point of the patch
is to avoid the use of an l?arx/st?cx. pair.
Regards,
Gabriel
quoted hunk
+static __inline__ void local_add(long i, local_t *l)+{+ long t;++ __asm__ __volatile__(+"1: crset 22\n"+"2:" PPC_LL" %0,0(%2)\n"+"3: add %0,%1,%0\n"+"4:" PPC405_ERR77(0,%2)+"5:" PPC_STL" %0,0(%2)\n"+"6: crclr 22\n"+"7:\n"+" .section __ex_table,\"a\"\n"+ PPC_LONG_ALIGN "\n"+ PPC_LONG "2b,1b\n"+ PPC_LONG "3b,1b\n"+ PPC_LONG "4b,1b\n"+ PPC_LONG "5b,1b\n"+ PPC_LONG "6b,6b\n"+" .previous\n"+ : "=&r" (t)+ : "r" (i), "r" (&(l->a.counter)));+}++static __inline__ void local_sub(long i, local_t *l)+{+ long t;++ __asm__ __volatile__(+"1: crset 22\n"+"2:" PPC_LL" %0,0(%2)\n"+"3: subf %0,%1,%0\n"+"4:" PPC405_ERR77(0,%2)+"5:" PPC_STL" %0,0(%2)\n"+"6: crclr 22\n"+"7:\n"+" .section __ex_table,\"a\"\n"+ PPC_LONG_ALIGN "\n"+ PPC_LONG "2b,1b\n"+ PPC_LONG "3b,1b\n"+ PPC_LONG "4b,1b\n"+ PPC_LONG "5b,1b\n"+ PPC_LONG "6b,6b\n"+" .previous\n"+ : "=&r" (t)+ : "r" (i), "r" (&(l->a.counter)));+}++static __inline__ long local_add_return(long a, local_t *l)+{+ long t;++ __asm__ __volatile__(+"1: crset 22\n"+"2:" PPC_LL" %0,0(%2)\n"+"3: add %0,%1,%0\n"+"4:" PPC405_ERR77(0,%2)+"5:" PPC_STL "%0,0(%2)\n"+"6: crclr 22\n"+"7:\n"+" .section __ex_table,\"a\"\n"+ PPC_LONG_ALIGN "\n"+ PPC_LONG "2b,1b\n"+ PPC_LONG "3b,1b\n"+ PPC_LONG "4b,1b\n"+ PPC_LONG "5b,1b\n"+ PPC_LONG "6b,6b\n"+" .previous\n"+ : "=&r" (t)+ : "r" (a), "r" (&(l->a.counter))+ : "cc", "memory");++ return t;+}+++#define local_add_negative(a, l) (local_add_return((a), (l)) < 0)++static __inline__ long local_sub_return(long a, local_t *l)+{+ long t;++ __asm__ __volatile__(+"1: crset 22\n"+"2:" PPC_LL" %0,0(%2)\n"+"3: subf %0,%1,%0\n"+"4:" PPC405_ERR77(0,%2)+"5:" PPC_STL "%0,0(%2)\n"+"6: crclr 22\n"+"7:\n"+" .section __ex_table,\"a\"\n"+ PPC_LONG_ALIGN "\n"+ PPC_LONG "2b,1b\n"+ PPC_LONG "3b,1b\n"+ PPC_LONG "4b,1b\n"+ PPC_LONG "5b,1b\n"+ PPC_LONG "6b,6b\n"+" .previous\n"+ : "=&r" (t)+ : "r" (a), "r" (&(l->a.counter))+ : "cc", "memory");++ return t;+}++static __inline__ long local_inc_return(local_t *l)+{+ long t;++ __asm__ __volatile__(+"1: crset 22\n"+"2:" PPC_LL" %0,0(%1)\n"+"3: addic %0,%0,1\n"+"4:" PPC405_ERR77(0,%1)+"5:" PPC_STL "%0,0(%1)\n"+"6: crclr 22\n"+"7:\n"+" .section __ex_table,\"a\"\n"+ PPC_LONG_ALIGN "\n"+ PPC_LONG "2b,1b\n"+ PPC_LONG "3b,1b\n"+ PPC_LONG "4b,1b\n"+ PPC_LONG "5b,1b\n"+ PPC_LONG "6b,6b\n"+" .previous"+ : "=&r" (t)+ : "r" (&(l->a.counter))+ : "cc", "xer", "memory");++ return t;+}++/*+ * local_inc_and_test - increment and test+ * @l: pointer of type local_t+ *+ * Atomically increments @l by 1+ * and returns true if the result is zero, or false for all+ * other cases.+ */+#define local_inc_and_test(l) (local_inc_return(l) == 0)++static __inline__ long local_dec_return(local_t *l)+{+ long t;++ __asm__ __volatile__(+"1: crset 22\n"+"2:" PPC_LL" %0,0(%1)\n"+"3: addic %0,%0,-1\n"+"4:" PPC405_ERR77(0,%1)+"5:" PPC_STL "%0,0(%1)\n"+"6: crclr 22\n"+"7:\n"+" .section __ex_table,\"a\"\n"+ PPC_LONG_ALIGN "\n"+ PPC_LONG "2b,1b\n"+ PPC_LONG "3b,1b\n"+ PPC_LONG "4b,1b\n"+ PPC_LONG "5b,1b\n"+ PPC_LONG "6b,6b\n"+" .previous\n"+ : "=&r" (t)+ : "r" (&(l->a.counter))+ : "cc", "xer", "memory");++ return t;+}++#define local_inc(l) local_inc_return(l)+#define local_dec(l) local_dec_return(l)++#define local_cmpxchg(l, o, n) \+ (cmpxchg_local(&((l)->a.counter), (o), (n)))+#define local_xchg(l, n) (xchg_local(&((l)->a.counter), (n)))++/**+ * local_add_unless - add unless the number is a given value+ * @l: pointer of type local_t+ * @a: the amount to add to v...+ * @u: ...unless v is equal to u.+ *+ * Atomically adds @a to @l, so long as it was not @u.+ * Returns non-zero if @l was not @u, and zero otherwise.+ */+static __inline__ int local_add_unless(local_t *l, long a, long u)+{+ long t;++ __asm__ __volatile__ (+"1: crset 22\n"+"2:" PPC_LL" %0,0(%1)\n"+"3: cmpw 0,%0,%3 \n"+"4: beq- 9f \n"+"5: add %0,%2,%0 \n"+"6:" PPC405_ERR77(0,%1)+"7:" PPC_STL" %0,0(%1) \n"+"8: subf %0,%2,%0 \n"+"9: crclr 22\n"+"10:\n"+" .section __ex_table,\"a\"\n"+ PPC_LONG_ALIGN "\n"+ PPC_LONG "2b,1b\n"+ PPC_LONG "3b,1b\n"+ PPC_LONG "4b,1b\n"+ PPC_LONG "5b,1b\n"+ PPC_LONG "6b,1b\n"+ PPC_LONG "7b,1b\n"+ PPC_LONG "8b,8b\n"+ PPC_LONG "9b,9b\n"+" .previous\n"+ : "=&r" (t)+ : "r" (&(l->a.counter)), "r" (a), "r" (u)+ : "cc", "memory");++ return t != u;+}++#define local_inc_not_zero(l) local_add_unless((l), 1, 0)++#define local_sub_and_test(a, l) (local_sub_return((a), (l)) == 0)+#define local_dec_and_test(l) (local_dec_return((l)) == 0)++/*+ * Atomically test *l and decrement if it is greater than 0.+ * The function returns the old value of *l minus 1.+ */+static __inline__ long local_dec_if_positive(local_t *l)+{+ long t;++ __asm__ __volatile__(+"1: crset 22\n"+"2:" PPC_LL" %0,0(%1)\n"+"3: cmpwi %0,1\n"+"4: addi %0,%0,-1\n"+"5: blt- 8f\n"+"6:" PPC405_ERR77(0,%1)+"7:" PPC_STL "%0,0(%1)\n"+"8: crclr 22\n"+"9:\n"+" .section__ex_table,\"a\"\n"+ PPC_LONG_ALIGN "\n"+ PPC_LONG "2b,1b\n"+ PPC_LONG "3b,1b\n"+ PPC_LONG "4b,1b\n"+ PPC_LONG "5b,1b\n"+ PPC_LONG "6b,1b\n"+ PPC_LONG "7b,1b\n"+ PPC_LONG "8b,8b\n"+" .previous\n"+ : "=&b" (t)+ : "r" (&(l->a.counter))+ : "cc", "memory");++ return t;+}++#else+ #define local_read(l) atomic_long_read(&(l)->a) #define local_set(l,i) atomic_long_set(&(l)->a, (i))
@@ -162,6 +466,8 @@ static __inline__ long local_dec_if_positive(local_t *l) return t; }+#endif+ /* Use these for per-cpu local_t variables: on some archs they are * much more efficient than these naive implementations. Note they take * a variable, not an address.
--
1.9.1
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
From: Gabriel Paubert <hidden> Date: 2014-12-01 21:35:49
On Thu, Nov 27, 2014 at 05:48:40PM +0530, Madhavan Srinivasan wrote:
quoted hunk
This patch create the infrastructure to handle the CR based
local_* atomic operations. Local atomic operations are fast
and highly reentrant per CPU counters. Used for percpu
variable updates. Local atomic operations only guarantee
variable modification atomicity wrt the CPU which owns the
data and these needs to be executed in a preemption safe way.
Here is the design of this patch. Since local_* operations
are only need to be atomic to interrupts (IIUC), patch uses
one of the Condition Register (CR) fields as a flag variable. When
entering the local_*, specific bit in the CR5 field is set
and on exit, bit is cleared. CR bit checking is done in the
interrupt return path. If CR5[EQ] bit set and if we return
to kernel, we reset to start of local_* operation.
Reason for this approach is that, currently l[w/d]arx/st[w/d]cx.
instruction pair is used for local_* operations, which are heavy
on cycle count and they dont support a local variant. So to
see whether the new implementation helps, used a modified
version of Rusty's benchmark code on local_t.
https://lkml.org/lkml/2008/12/16/450
Modifications:
- increated the working set size from 1MB to 8MB,
- removed cpu_local_inc test.
Test ran
- on POWER8 1S Scale out System 2.0GHz
- on OPAL v3 with v3.18-rc4 patch kernel as Host
Here are the values with the patch.
Time in ns per iteration
inc add read add_return
atomic_long 67 67 18 69
irqsave/rest 39 39 23 39
trivalue 39 39 29 49
local_t 26 26 24 26
Since CR5 is used as a flag, have added CFLAGS to avoid CR5
for the kernel compilation and CR5 is zeroed at the kernel
entry.
Tested the patch in a
- pSeries LPAR,
- Host with patched/unmodified guest kernel
To check whether userspace see any CR5 corruption, ran a simple
test which does,
- set CR5 field,
- while(1)
- sleep or gettimeofday
- chk bit set
Signed-off-by: Madhavan Srinivasan <redacted>
---
- I really appreciate feedback on the patchset.
- Kindly comment if I should try with any other benchmark or
workload to check the numbers.
- Also, kindly recommand any know stress test for CR
Makefile | 6 ++
arch/powerpc/include/asm/exception-64s.h | 21 +++++-
arch/powerpc/kernel/entry_64.S | 106 ++++++++++++++++++++++++++++++-
arch/powerpc/kernel/exceptions-64s.S | 2 +-
arch/powerpc/kernel/head_64.S | 8 +++
5 files changed, 138 insertions(+), 5 deletions(-)
@@ -706,6 +706,12 @@ endifKBUILD_CFLAGS+=$(callcc-option,-fno-var-tracking-assignments)+ifdef CONFIG_PPC64+# We need this flag to force compiler not to use CR5, since+# local_t type code is based on this.+KBUILD_CFLAGS+=-ffixed-cr5+endif+ifdef CONFIG_DEBUG_INFOifdef CONFIG_DEBUG_INFO_SPLITKBUILD_CFLAGS+=$(callcc-option,-gsplit-dwarf,-g)
@@ -306,7 +306,26 @@ do_kvm_##n: \stdr10,0(r1);/* make stack chain pointer */\stdr0,GPR0(r1);/* save r0 in stackframe */\stdr10,GPR1(r1);/* save r1 in stackframe */\-beq4f;/* if from kernel mode */\+BEGIN_FTR_SECTION;\+lisr9,4096;/* Create a mask with HV and PR */\+rldicrr9,r9,32,31;/* bits, AND with the MSR */\+mrr10,r9;/* to check for Hyp state */\+orir9,r9,16384;\+andr9,r12,r9;\+cmpdcr3,r10,r9;\+beqcr3,66f;/* Jump if we come from Hyp mode*/\+mtcrf0x04,r10;/* Clear CR5 if coming from usr */\
I think you can do better than this, powerpc has a fantastic set
of rotate and mask instructions. If I understand correctly your
code you can replace it with the following:
rldicl r10,r12,4,63 /* Extract HV bit to LSB of r10*/
rlwinm r9,r12,19,0x02 /* Extract PR bit to 2nd to last bit of r9 */
or r9,r9,10
cmplwi cr3,r9,1 /* Check for HV=1 and PR=0 */
beq cr3,66f
mtcrf 0x04,r10 /* Bits going to cr5 bits are 0 in r10 */
and obviously similarly for the other instances of this code sequence.
This said, mtcrf is not necessarily the fastest way of setting cr5 if
you only want to clear the EQ bit. For example comparing the stack pointer
with 0 should have the same effect.
quoted hunk
+FTR_SECTION_ELSE; \+ beq 4f; /* if kernel mode branch */ \+ li r10,0; /* Clear CR5 incase of coming */ \+ mtcrf 0x04,r10; /* from user. */ \+ nop; /* This part of code is for */ \+ nop; /* kernel with MSR[HV]=0, */ \+ nop; /* MSR[PR]=0, so just chk for */ \+ nop; /* MSR[PR] */ \+ nop; \+ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE); \+66: beq 4f; /* if from kernel mode */ \ ACCOUNT_CPU_USER_ENTRY(r9, r10); \ SAVE_PPR(area, r9, r10); \ 4: EXCEPTION_PROLOG_COMMON_2(area) \
I believe that someonw has already mentioned that this is a very
convoluted way of testing a cr bit. This can be optimized to bne cr5,63f
quoted hunk
++ /*+ * Now that the bit is set, lets check for return to User+ */+ ld r4,_MSR(r1)+BEGIN_FTR_SECTION+ li r3,4096+ rldicr r3,r3,32,31+ mr r5,r3+ ori r3,r3,16384+ and r3,r4,r3+ cmpd r5,r3+ bne 63f+FTR_SECTION_ELSE+ andi. r3,r4,MSR_PR+ bne 63f+ nop+ nop+ nop+ nop+ nop+ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)++ /*+ * Looks like we are returning to Kernel, so+ * lets get the NIP and search the ex_table.+ * Change the NIP based on the return value+ */+lookup_ex_table:+ ld r3,_NIP(r1)+ bl search_exception_tables + cmpli 0,1,r3,0+ bne 62f++ /*+ * This is a panic case. Reason is that, we+ * have the CR5 bit set, but we are not in+ * local_* code and we are returning to Kernel.+ */+ ld r3,_NIP(r1)+ mfcr r4+ EMIT_BUG_ENTRY lookup_ex_table, __FILE__,__LINE__,BUGFLAG_WARNING++ /*+ * Now save the return fixup address as NIP+ */+62: ld r4,8(r3)+ std r4,_NIP(r1)+ crclr 22+63: ld r3,_MSR(r1) ld r4,_CTR(r1) ld r0,_LINK(r1) mtctr r4
From: Scott Wood <hidden> Date: 2014-12-02 02:05:06
On Thu, 2014-11-27 at 17:48 +0530, Madhavan Srinivasan wrote:
quoted hunk
- I really appreciate feedback on the patchset.- Kindly comment if I should try with any other benchmark or workload to check the numbers.- Also, kindly recommand any know stress test for CR Makefile | 6 ++ arch/powerpc/include/asm/exception-64s.h | 21 +++++- arch/powerpc/kernel/entry_64.S | 106 ++++++++++++++++++++++++++++++- arch/powerpc/kernel/exceptions-64s.S | 2 +- arch/powerpc/kernel/head_64.S | 8 +++ 5 files changed, 138 insertions(+), 5 deletions(-)
Patch 2/2 enables this for all PPC64, not just book3s -- so please don't
forget about the book3e exception paths (also MSR[GS] for KVM, but
aren't most if not all the places you're checking for HV mode after KVM
would have taken control? Or am I missing something about how book3s
KVM works?).
Or, if you don't want to do that, change patch 2/2 to be book3s only and
ifdef-protect the changes to common exception code.
mtcrf with more than one CRn being updated is expensive on Freescale
chips (and this isn't a book3s-only code path). Why do you need to do
it twice? I don't see where either r5 or cr5 are messed with between
the two places...
-Scott
On Tuesday 02 December 2014 07:34 AM, Scott Wood wrote:
On Thu, 2014-11-27 at 17:48 +0530, Madhavan Srinivasan wrote:
quoted
- I really appreciate feedback on the patchset.- Kindly comment if I should try with any other benchmark or workload to check the numbers.- Also, kindly recommand any know stress test for CR Makefile | 6 ++ arch/powerpc/include/asm/exception-64s.h | 21 +++++- arch/powerpc/kernel/entry_64.S | 106 ++++++++++++++++++++++++++++++- arch/powerpc/kernel/exceptions-64s.S | 2 +- arch/powerpc/kernel/head_64.S | 8 +++ 5 files changed, 138 insertions(+), 5 deletions(-)
Patch 2/2 enables this for all PPC64, not just book3s -- so please don't
forget about the book3e exception paths (also MSR[GS] for KVM, but
aren't most if not all the places you're checking for HV mode after KVM
would have taken control? Or am I missing something about how book3s
KVM works?).
Or, if you don't want to do that, change patch 2/2 to be book3s only and
ifdef-protect the changes to common exception code.
Still learning the interrupt path and various configs. Was assuming
PPC64 is for server side. My bad. Will change the it to book3s and also
to common code.
mtcrf with more than one CRn being updated is expensive on Freescale
chips (and this isn't a book3s-only code path). Why do you need to do
it twice? I don't see where either r5 or cr5 are messed with between
the two places...
Incase of returning to userspace, will be updating it twice. Which can
be avoid. Will redo this.
Regards
Maddy
On Tuesday 02 December 2014 03:05 AM, Gabriel Paubert wrote:
On Thu, Nov 27, 2014 at 05:48:40PM +0530, Madhavan Srinivasan wrote:
quoted
This patch create the infrastructure to handle the CR based
local_* atomic operations. Local atomic operations are fast
and highly reentrant per CPU counters. Used for percpu
variable updates. Local atomic operations only guarantee
variable modification atomicity wrt the CPU which owns the
data and these needs to be executed in a preemption safe way.
Here is the design of this patch. Since local_* operations
are only need to be atomic to interrupts (IIUC), patch uses
one of the Condition Register (CR) fields as a flag variable. When
entering the local_*, specific bit in the CR5 field is set
and on exit, bit is cleared. CR bit checking is done in the
interrupt return path. If CR5[EQ] bit set and if we return
to kernel, we reset to start of local_* operation.
Reason for this approach is that, currently l[w/d]arx/st[w/d]cx.
instruction pair is used for local_* operations, which are heavy
on cycle count and they dont support a local variant. So to
see whether the new implementation helps, used a modified
version of Rusty's benchmark code on local_t.
https://lkml.org/lkml/2008/12/16/450
Modifications:
- increated the working set size from 1MB to 8MB,
- removed cpu_local_inc test.
Test ran
- on POWER8 1S Scale out System 2.0GHz
- on OPAL v3 with v3.18-rc4 patch kernel as Host
Here are the values with the patch.
Time in ns per iteration
inc add read add_return
atomic_long 67 67 18 69
irqsave/rest 39 39 23 39
trivalue 39 39 29 49
local_t 26 26 24 26
Since CR5 is used as a flag, have added CFLAGS to avoid CR5
for the kernel compilation and CR5 is zeroed at the kernel
entry.
Tested the patch in a
- pSeries LPAR,
- Host with patched/unmodified guest kernel
To check whether userspace see any CR5 corruption, ran a simple
test which does,
- set CR5 field,
- while(1)
- sleep or gettimeofday
- chk bit set
Signed-off-by: Madhavan Srinivasan <redacted>
---
- I really appreciate feedback on the patchset.
- Kindly comment if I should try with any other benchmark or
workload to check the numbers.
- Also, kindly recommand any know stress test for CR
Makefile | 6 ++
arch/powerpc/include/asm/exception-64s.h | 21 +++++-
arch/powerpc/kernel/entry_64.S | 106 ++++++++++++++++++++++++++++++-
arch/powerpc/kernel/exceptions-64s.S | 2 +-
arch/powerpc/kernel/head_64.S | 8 +++
5 files changed, 138 insertions(+), 5 deletions(-)
@@ -706,6 +706,12 @@ endifKBUILD_CFLAGS+=$(callcc-option,-fno-var-tracking-assignments)+ifdef CONFIG_PPC64+# We need this flag to force compiler not to use CR5, since+# local_t type code is based on this.+KBUILD_CFLAGS+=-ffixed-cr5+endif+ifdef CONFIG_DEBUG_INFOifdef CONFIG_DEBUG_INFO_SPLITKBUILD_CFLAGS+=$(callcc-option,-gsplit-dwarf,-g)
@@ -306,7 +306,26 @@ do_kvm_##n: \stdr10,0(r1);/* make stack chain pointer */\stdr0,GPR0(r1);/* save r0 in stackframe */\stdr10,GPR1(r1);/* save r1 in stackframe */\-beq4f;/* if from kernel mode */\+BEGIN_FTR_SECTION;\+lisr9,4096;/* Create a mask with HV and PR */\+rldicrr9,r9,32,31;/* bits, AND with the MSR */\+mrr10,r9;/* to check for Hyp state */\+orir9,r9,16384;\+andr9,r12,r9;\+cmpdcr3,r10,r9;\+beqcr3,66f;/* Jump if we come from Hyp mode*/\+mtcrf0x04,r10;/* Clear CR5 if coming from usr */\
I think you can do better than this, powerpc has a fantastic set
of rotate and mask instructions. If I understand correctly your
code you can replace it with the following:
rldicl r10,r12,4,63 /* Extract HV bit to LSB of r10*/
rlwinm r9,r12,19,0x02 /* Extract PR bit to 2nd to last bit of r9 */
or r9,r9,10
cmplwi cr3,r9,1 /* Check for HV=1 and PR=0 */
beq cr3,66f
mtcrf 0x04,r10 /* Bits going to cr5 bits are 0 in r10 */
From previous comment, at this point, CR0 already has MSR[PR], and only
HV need to be checked. So I guess there still room for optimization.
But good to learn this seq.
and obviously similarly for the other instances of this code sequence.
This said, mtcrf is not necessarily the fastest way of setting cr5 if
you only want to clear the EQ bit. For example comparing the stack pointer
with 0 should have the same effect.
Yes. Will try this out.
quoted
+FTR_SECTION_ELSE; \+ beq 4f; /* if kernel mode branch */ \+ li r10,0; /* Clear CR5 incase of coming */ \+ mtcrf 0x04,r10; /* from user. */ \+ nop; /* This part of code is for */ \+ nop; /* kernel with MSR[HV]=0, */ \+ nop; /* MSR[PR]=0, so just chk for */ \+ nop; /* MSR[PR] */ \+ nop; \+ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE); \+66: beq 4f; /* if from kernel mode */ \ ACCOUNT_CPU_USER_ENTRY(r9, r10); \ SAVE_PPR(area, r9, r10); \ 4: EXCEPTION_PROLOG_COMMON_2(area) \
I believe that someonw has already mentioned that this is a very
convoluted way of testing a cr bit. This can be optimized to bne cr5,63f
This. Will change it.
Regards
Maddy
quoted
++ /*+ * Now that the bit is set, lets check for return to User+ */+ ld r4,_MSR(r1)+BEGIN_FTR_SECTION+ li r3,4096+ rldicr r3,r3,32,31+ mr r5,r3+ ori r3,r3,16384+ and r3,r4,r3+ cmpd r5,r3+ bne 63f+FTR_SECTION_ELSE+ andi. r3,r4,MSR_PR+ bne 63f+ nop+ nop+ nop+ nop+ nop+ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)++ /*+ * Looks like we are returning to Kernel, so+ * lets get the NIP and search the ex_table.+ * Change the NIP based on the return value+ */+lookup_ex_table:+ ld r3,_NIP(r1)+ bl search_exception_tables + cmpli 0,1,r3,0+ bne 62f++ /*+ * This is a panic case. Reason is that, we+ * have the CR5 bit set, but we are not in+ * local_* code and we are returning to Kernel.+ */+ ld r3,_NIP(r1)+ mfcr r4+ EMIT_BUG_ENTRY lookup_ex_table, __FILE__,__LINE__,BUGFLAG_WARNING++ /*+ * Now save the return fixup address as NIP+ */+62: ld r4,8(r3)+ std r4,_NIP(r1)+ crclr 22+63: ld r3,_MSR(r1) ld r4,_CTR(r1) ld r0,_LINK(r1) mtctr r4
On Monday 01 December 2014 11:31 PM, Gabriel Paubert wrote:
On Thu, Nov 27, 2014 at 05:48:41PM +0530, Madhavan Srinivasan wrote:
quoted
This patch re-write the current local_* functions to CR5 based one.
Base flow for each function is
{
set cr5(eq)
load
..
store
clear cr5(eq)
}
Above set of instructions are followed by a fixup section which points
to the entry of the function incase of interrupt in the flow. If the
interrupt happens to be after the store, we just continue to last
instruction in that block.
Currently only asm/local.h has been rewrite, and local64 is TODO.
Also the entire change is only for PPC64.
Signed-off-by: Madhavan Srinivasan <redacted>
---
arch/powerpc/include/asm/local.h | 306 +++++++++++++++++++++++++++++++++++++++
1 file changed, 306 insertions(+)
Apart from the other comments on bloat which can very likely
be removed by tracing backwards for a few instructions, removing
the exception table entries which are 2 or 4 (64 bit?) times as
large as the instruction sequence, I don't understand at all why
you need these sequences for the local_read and local_set functions.
After all these are single instructions (why do you perform a read before
the write in set when the result of the read is never used?).
Agreed. But the benchmark I used, also did check for local_read timing.
So for consistency I re-wrote the these functions. But will drop the
changes for these function in the next version.
I believe read and set are better mapped to access_once (or assign_once
or whatever it's called after the recent discussion on linux-kernel).
You don't even need a memory barrier if it's for a single thread,
so you could get away with a single volatile access to the variables.
OK.
For the other ones, I think that what you do is correct, except that
the workaround for PPC405 erratum 77 is not needed since this erratum
only affects the stwcx. instruction and the whole point of the patch
is to avoid the use of an l?arx/st?cx. pair.
Yes. Will remove it.
With regards
Maddy
Regards,
Gabriel
quoted
+static __inline__ void local_add(long i, local_t *l)+{+ long t;++ __asm__ __volatile__(+"1: crset 22\n"+"2:" PPC_LL" %0,0(%2)\n"+"3: add %0,%1,%0\n"+"4:" PPC405_ERR77(0,%2)+"5:" PPC_STL" %0,0(%2)\n"+"6: crclr 22\n"+"7:\n"+" .section __ex_table,\"a\"\n"+ PPC_LONG_ALIGN "\n"+ PPC_LONG "2b,1b\n"+ PPC_LONG "3b,1b\n"+ PPC_LONG "4b,1b\n"+ PPC_LONG "5b,1b\n"+ PPC_LONG "6b,6b\n"+" .previous\n"+ : "=&r" (t)+ : "r" (i), "r" (&(l->a.counter)));+}++static __inline__ void local_sub(long i, local_t *l)+{+ long t;++ __asm__ __volatile__(+"1: crset 22\n"+"2:" PPC_LL" %0,0(%2)\n"+"3: subf %0,%1,%0\n"+"4:" PPC405_ERR77(0,%2)+"5:" PPC_STL" %0,0(%2)\n"+"6: crclr 22\n"+"7:\n"+" .section __ex_table,\"a\"\n"+ PPC_LONG_ALIGN "\n"+ PPC_LONG "2b,1b\n"+ PPC_LONG "3b,1b\n"+ PPC_LONG "4b,1b\n"+ PPC_LONG "5b,1b\n"+ PPC_LONG "6b,6b\n"+" .previous\n"+ : "=&r" (t)+ : "r" (i), "r" (&(l->a.counter)));+}++static __inline__ long local_add_return(long a, local_t *l)+{+ long t;++ __asm__ __volatile__(+"1: crset 22\n"+"2:" PPC_LL" %0,0(%2)\n"+"3: add %0,%1,%0\n"+"4:" PPC405_ERR77(0,%2)+"5:" PPC_STL "%0,0(%2)\n"+"6: crclr 22\n"+"7:\n"+" .section __ex_table,\"a\"\n"+ PPC_LONG_ALIGN "\n"+ PPC_LONG "2b,1b\n"+ PPC_LONG "3b,1b\n"+ PPC_LONG "4b,1b\n"+ PPC_LONG "5b,1b\n"+ PPC_LONG "6b,6b\n"+" .previous\n"+ : "=&r" (t)+ : "r" (a), "r" (&(l->a.counter))+ : "cc", "memory");++ return t;+}+++#define local_add_negative(a, l) (local_add_return((a), (l)) < 0)++static __inline__ long local_sub_return(long a, local_t *l)+{+ long t;++ __asm__ __volatile__(+"1: crset 22\n"+"2:" PPC_LL" %0,0(%2)\n"+"3: subf %0,%1,%0\n"+"4:" PPC405_ERR77(0,%2)+"5:" PPC_STL "%0,0(%2)\n"+"6: crclr 22\n"+"7:\n"+" .section __ex_table,\"a\"\n"+ PPC_LONG_ALIGN "\n"+ PPC_LONG "2b,1b\n"+ PPC_LONG "3b,1b\n"+ PPC_LONG "4b,1b\n"+ PPC_LONG "5b,1b\n"+ PPC_LONG "6b,6b\n"+" .previous\n"+ : "=&r" (t)+ : "r" (a), "r" (&(l->a.counter))+ : "cc", "memory");++ return t;+}++static __inline__ long local_inc_return(local_t *l)+{+ long t;++ __asm__ __volatile__(+"1: crset 22\n"+"2:" PPC_LL" %0,0(%1)\n"+"3: addic %0,%0,1\n"+"4:" PPC405_ERR77(0,%1)+"5:" PPC_STL "%0,0(%1)\n"+"6: crclr 22\n"+"7:\n"+" .section __ex_table,\"a\"\n"+ PPC_LONG_ALIGN "\n"+ PPC_LONG "2b,1b\n"+ PPC_LONG "3b,1b\n"+ PPC_LONG "4b,1b\n"+ PPC_LONG "5b,1b\n"+ PPC_LONG "6b,6b\n"+" .previous"+ : "=&r" (t)+ : "r" (&(l->a.counter))+ : "cc", "xer", "memory");++ return t;+}++/*+ * local_inc_and_test - increment and test+ * @l: pointer of type local_t+ *+ * Atomically increments @l by 1+ * and returns true if the result is zero, or false for all+ * other cases.+ */+#define local_inc_and_test(l) (local_inc_return(l) == 0)++static __inline__ long local_dec_return(local_t *l)+{+ long t;++ __asm__ __volatile__(+"1: crset 22\n"+"2:" PPC_LL" %0,0(%1)\n"+"3: addic %0,%0,-1\n"+"4:" PPC405_ERR77(0,%1)+"5:" PPC_STL "%0,0(%1)\n"+"6: crclr 22\n"+"7:\n"+" .section __ex_table,\"a\"\n"+ PPC_LONG_ALIGN "\n"+ PPC_LONG "2b,1b\n"+ PPC_LONG "3b,1b\n"+ PPC_LONG "4b,1b\n"+ PPC_LONG "5b,1b\n"+ PPC_LONG "6b,6b\n"+" .previous\n"+ : "=&r" (t)+ : "r" (&(l->a.counter))+ : "cc", "xer", "memory");++ return t;+}++#define local_inc(l) local_inc_return(l)+#define local_dec(l) local_dec_return(l)++#define local_cmpxchg(l, o, n) \+ (cmpxchg_local(&((l)->a.counter), (o), (n)))+#define local_xchg(l, n) (xchg_local(&((l)->a.counter), (n)))++/**+ * local_add_unless - add unless the number is a given value+ * @l: pointer of type local_t+ * @a: the amount to add to v...+ * @u: ...unless v is equal to u.+ *+ * Atomically adds @a to @l, so long as it was not @u.+ * Returns non-zero if @l was not @u, and zero otherwise.+ */+static __inline__ int local_add_unless(local_t *l, long a, long u)+{+ long t;++ __asm__ __volatile__ (+"1: crset 22\n"+"2:" PPC_LL" %0,0(%1)\n"+"3: cmpw 0,%0,%3 \n"+"4: beq- 9f \n"+"5: add %0,%2,%0 \n"+"6:" PPC405_ERR77(0,%1)+"7:" PPC_STL" %0,0(%1) \n"+"8: subf %0,%2,%0 \n"+"9: crclr 22\n"+"10:\n"+" .section __ex_table,\"a\"\n"+ PPC_LONG_ALIGN "\n"+ PPC_LONG "2b,1b\n"+ PPC_LONG "3b,1b\n"+ PPC_LONG "4b,1b\n"+ PPC_LONG "5b,1b\n"+ PPC_LONG "6b,1b\n"+ PPC_LONG "7b,1b\n"+ PPC_LONG "8b,8b\n"+ PPC_LONG "9b,9b\n"+" .previous\n"+ : "=&r" (t)+ : "r" (&(l->a.counter)), "r" (a), "r" (u)+ : "cc", "memory");++ return t != u;+}++#define local_inc_not_zero(l) local_add_unless((l), 1, 0)++#define local_sub_and_test(a, l) (local_sub_return((a), (l)) == 0)+#define local_dec_and_test(l) (local_dec_return((l)) == 0)++/*+ * Atomically test *l and decrement if it is greater than 0.+ * The function returns the old value of *l minus 1.+ */+static __inline__ long local_dec_if_positive(local_t *l)+{+ long t;++ __asm__ __volatile__(+"1: crset 22\n"+"2:" PPC_LL" %0,0(%1)\n"+"3: cmpwi %0,1\n"+"4: addi %0,%0,-1\n"+"5: blt- 8f\n"+"6:" PPC405_ERR77(0,%1)+"7:" PPC_STL "%0,0(%1)\n"+"8: crclr 22\n"+"9:\n"+" .section__ex_table,\"a\"\n"+ PPC_LONG_ALIGN "\n"+ PPC_LONG "2b,1b\n"+ PPC_LONG "3b,1b\n"+ PPC_LONG "4b,1b\n"+ PPC_LONG "5b,1b\n"+ PPC_LONG "6b,1b\n"+ PPC_LONG "7b,1b\n"+ PPC_LONG "8b,8b\n"+" .previous\n"+ : "=&b" (t)+ : "r" (&(l->a.counter))+ : "cc", "memory");++ return t;+}++#else+ #define local_read(l) atomic_long_read(&(l)->a) #define local_set(l,i) atomic_long_set(&(l)->a, (i))
@@ -162,6 +466,8 @@ static __inline__ long local_dec_if_positive(local_t *l) return t; }+#endif+ /* Use these for per-cpu local_t variables: on some archs they are * much more efficient than these naive implementations. Note they take * a variable, not an address.
--
1.9.1
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
@@ -306,7 +306,26 @@ do_kvm_##n: \stdr10,0(r1);/* make stack chain pointer */\stdr0,GPR0(r1);/* save r0 in stackframe */\stdr10,GPR1(r1);/* save r1 in stackframe */\-beq4f;/* if from kernel mode */\+BEGIN_FTR_SECTION;\+lisr9,4096;/* Create a mask with HV and PR */\+rldicrr9,r9,32,31;/* bits, AND with the MSR */\+mrr10,r9;/* to check for Hyp state */\+orir9,r9,16384;\+andr9,r12,r9;\+cmpdcr3,r10,r9;\+beqcr3,66f;/* Jump if we come from Hyp mode*/\+mtcrf0x04,r10;/* Clear CR5 if coming from usr */\
I think you can do better than this, powerpc has a fantastic set
of rotate and mask instructions. If I understand correctly your
code you can replace it with the following:
rldicl r10,r12,4,63 /* Extract HV bit to LSB of r10*/
rlwinm r9,r12,19,0x02 /* Extract PR bit to 2nd to last bit of r9 */
or r9,r9,10
cmplwi cr3,r9,1 /* Check for HV=1 and PR=0 */
beq cr3,66f
mtcrf 0x04,r10 /* Bits going to cr5 bits are 0 in r10 */
quoted
From previous comment, at this point, CR0 already has MSR[PR], and only
HV need to be checked. So I guess there still room for optimization.
But good to learn this seq.
Actually the sequence I suggested can even be shortened again since the or
is not necessary and you can use an rlwimi instead.
rldicl r9,r12,5,62 /* r9 = 62 zeroes | HV | ? */
rlwimi r9,r12,18,0x01 /* r9 = 62 zeroes | HV | PR */
cmplwi cr3,r9,2 /* Check for HV=1 and PR=0 */
For 32 bit operands, rlwimi is a generic bit field to bit field move, but
GCC is (was, maybe GCC5 is better?) quite bad at recognizing opportunities
to use it.
I'm not sure that it is better to have two separate branches, one for
testing PR and the other for testing HV. Through how many branches can
the hardware speculate?
Unless I'm mistaken, this is code which is likely not to be in the
cache so I would optimize it first towards minimal code size.
Regards,
Gabriel
I also wonder if it is possible to inspect the interrupted
code to determine the start/end of the RAS block.
(Easiest if you assume that there is a single 'write' instruction
as the last entry in the block.)
So each local_* function also have code in the __ex_table section. IIUC,
__ex_table contains two address. So if the return address found in the
first column of the _ex_table, use the corresponding address in the
second column to continue from.
That really doesn't scale.
I don't know how many 1000 address pairs you table will have (and the
ones in each loadable module), but the search isn't going to be cheap.
If these sequences are restartable then they can only have one write
to memory.
May be, but i see these issues incase of insts decode path,
1) Decoding instruction may also cause a fault (in case of module) and
handling a fault at this stage toward the exit path of interrupt exit
makes me nervous
It shouldn't be possible to unload a module that is interrupted by
a hardware interrupt.
An 'invalid' loadable module can cause an oops/panic anyway.
Yes, the module won't fault (vmalloc memory can be lazily mapped, but
we've already copied the module into there, so it won't happen).
quoted
2) resulting code with lot of condition and branch (for opcode decode)
will be lot messy and may be an issue incase of maintenance,
You don't need to decode the instructions.
Just look for the two specific instructions used as markers.
This is only really possible with fixed-size instructions.
It might also be that the 'interrupt entry' path is easier to
modify than the 'interrupt exit' one (fewer code paths) and
you just need to modify the 'pc' in the stack frame.
You are only interested in interrupts from kernel space.
It's an overoptimization for case that statistically never happens.
You won't even be able to measure the difference.
The question of bloat remains, but that's also easily measured. In
practice, I'd guess less than 1k.
Cheers,
Rusty.
I also wonder if it is possible to inspect the interrupted
code to determine the start/end of the RAS block.
(Easiest if you assume that there is a single 'write' instruction
as the last entry in the block.)
So each local_* function also have code in the __ex_table section. IIUC,
__ex_table contains two address. So if the return address found in the
first column of the _ex_table, use the corresponding address in the
second column to continue from.
That really doesn't scale.
I don't know how many 1000 address pairs you table will have (and the
ones in each loadable module), but the search isn't going to be cheap.
If these sequences are restartable then they can only have one write
to memory.
...
quoted
quoted
quoted
2) resulting code with lot of condition and branch (for opcode decode)
will be lot messy and may be an issue incase of maintenance,
You don't need to decode the instructions.
Just look for the two specific instructions used as markers.
This is only really possible with fixed-size instructions.
It might also be that the 'interrupt entry' path is easier to
modify than the 'interrupt exit' one (fewer code paths) and
you just need to modify the 'pc' in the stack frame.
You are only interested in interrupts from kernel space.
It's an overoptimization for case that statistically never happens.
You won't even be able to measure the difference.
The question of bloat remains, but that's also easily measured. In
practice, I'd guess less than 1k.
IIRC they were 'static inline' so the table of addresses is generated
for every use site.
(copyin/out generates a similarly enormous table of addresses on amd64)
There are about 20 callers in the entire kernel.
Cheers,
Rusty.