From: Kevin Hao <hidden> Date: 2015-08-13 11:51:51
Since we moved the "lock" to be the first element of
struct tlb_core_data in commit 82d86de25b9c ("powerpc/e6500: Make TLB
lock recursive), this macro is not used by any code. Just delete it.
Signed-off-by: Kevin Hao <redacted>
---
arch/powerpc/kernel/asm-offsets.c | 1 -
1 file changed, 1 deletion(-)
From: Kevin Hao <hidden> Date: 2015-08-13 11:51:54
It makes no sense to put the instructions for calculating the lock
value (cpu number + 1) and the clearing of eq bit of cr1 in lbarx/stbcx
loop. And when the lock is acquired by the other thread, the current
lock value has no chance to equal with the lock value used by current
cpu. So we can skip the comparing for these two lock values in the
lbz/bne loop.
Signed-off-by: Kevin Hao <redacted>
---
arch/powerpc/mm/tlb_low_64e.S | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
From: Kevin Hao <hidden> Date: 2015-08-13 11:51:55
I didn't find anything unusual. But I think we do need to order the
load/store of esel_next when acquire/release tcd lock. For acquire,
add a data dependency to order the loads of lock and esel_next.
For release, even there already have a "isync" here, but it doesn't
guarantee any memory access order. So we still need "lwsync" for
the two stores for lock and esel_next.
Signed-off-by: Kevin Hao <redacted>
---
arch/powerpc/mm/tlb_low_64e.S | 3 +++
1 file changed, 3 insertions(+)
From: Scott Wood <hidden> Date: 2015-08-13 18:45:00
On Thu, 2015-08-13 at 19:51 +0800, Kevin Hao wrote:
quoted hunk
It makes no sense to put the instructions for calculating the lock
value (cpu number + 1) and the clearing of eq bit of cr1 in lbarx/stbcx
loop. And when the lock is acquired by the other thread, the current
lock value has no chance to equal with the lock value used by current
cpu. So we can skip the comparing for these two lock values in the
lbz/bne loop.
Signed-off-by: Kevin Hao <redacted>
---
arch/powerpc/mm/tlb_low_64e.S | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
You're optimizing the contended case at the expense of introducing stalls in
the uncontended case. Does it really matter if there are more instructions
in the loop? This change just means that you'll spin in the loop for more
iterations (if it even does that -- I think the cycles per loop iteration
might be the same before and after, due to load latency and pairing) while
waiting for the other thread to release the lock.
Do you have any benchmark results for this patch?
-Scott
From: Scott Wood <hidden> Date: 2015-08-14 03:39:34
On Thu, 2015-08-13 at 19:51 +0800, Kevin Hao wrote:
I didn't find anything unusual. But I think we do need to order the
load/store of esel_next when acquire/release tcd lock. For acquire,
add a data dependency to order the loads of lock and esel_next.
For release, even there already have a "isync" here, but it doesn't
guarantee any memory access order. So we still need "lwsync" for
the two stores for lock and esel_next.
I was going to say that esel_next is just a hint and it doesn't really matter
if we occasionally get the wrong value, unless it happens often enough to
cause more performance degradation than the lwsync causes. However, with the
A-008139 workaround we do need to read the same value from esel_next both
times. It might be less costly to save/restore an additional register
instead of lwsync, though.
-Scott
From: Kevin Hao <hidden> Date: 2015-08-14 07:13:21
On Thu, Aug 13, 2015 at 01:44:43PM -0500, Scott Wood wrote:
On Thu, 2015-08-13 at 19:51 +0800, Kevin Hao wrote:
quoted
It makes no sense to put the instructions for calculating the lock
value (cpu number + 1) and the clearing of eq bit of cr1 in lbarx/stbcx
loop. And when the lock is acquired by the other thread, the current
lock value has no chance to equal with the lock value used by current
cpu. So we can skip the comparing for these two lock values in the
lbz/bne loop.
Signed-off-by: Kevin Hao <redacted>
---
arch/powerpc/mm/tlb_low_64e.S | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
You're optimizing the contended case at the expense of introducing stalls in
the uncontended case.
Before the patch, the uncontended case code sequence are:
1: lbarx r15,0,r11
lhz r10,PACAPACAINDEX(r13)
cmpdi r15,0
cmpdi cr1,r15,1 /* set cr1.eq = 0 for non-recursive */
addi r10,r10,1
bne 2f
stbcx. r10,0,r11
bne 1b
After the patch:
lhz r10,PACAPACAINDEX(r13)
addi r10,r10,1
crclr cr1*4+eq /* set cr1.eq = 0 for non-recursive */
1: lbarx r15,0,r11
cmpdi r15,0
bne 2f
stbcx. r10,0,r11
bne 1b
As you know, the lbarx is a Presync instruction and stbcx is a Presync and
Postsync instruction. Putting the unnecessary instructions in the lbarx/stbcx
loop also serialize these instructions execution. The execution latency of
lbarx is only 3 cycles and there are still two instructions after it.
Considering the out of order execution optimization after this patch, do you
really think that new uncontended path will become slower due to this
additional stall?
Does it really matter if there are more instructions
in the loop?
I really think we should minimize the window of lbarx/stbcx, for following two
reasons:
- The bigger of this window, the more possible conflicts between the two
threads run into this loop simultaneously.
- The reservation used by lbarx may be cleared by another thread due to
store to the same reservation granule. The smaller the window of
lbarx/stbcx, the less possibility to be affected by this.
This change just means that you'll spin in the loop for more
iterations (if it even does that -- I think the cycles per loop iteration
might be the same before and after, due to load latency and pairing) while
waiting for the other thread to release the lock.
Besides the optimization for the contended case, it also make the code more
readable with these changes:
- It always seem a bit weird to calculate the lock value for the current
cpu in the lbarx/stbcx loop.
- The "cmpdi cr1,r15,1" seems pretty confusion. It doesn't always do what
the comment said (set cr1.eq = 0). In some cases, it does set the
crq.eq = 1, such as when running on cpu 1 with lock is acquired by cpu0.
All we need to do just clear the cr1.eq unconditionally.
Do you have any benchmark results for this patch?
I doubt it will get any visible difference. Anyway I will gave it a try.
Thanks,
Kevin
From: Kevin Hao <hidden> Date: 2015-08-14 07:14:08
On Thu, Aug 13, 2015 at 10:39:19PM -0500, Scott Wood wrote:
On Thu, 2015-08-13 at 19:51 +0800, Kevin Hao wrote:
quoted
I didn't find anything unusual. But I think we do need to order the
load/store of esel_next when acquire/release tcd lock. For acquire,
add a data dependency to order the loads of lock and esel_next.
For release, even there already have a "isync" here, but it doesn't
guarantee any memory access order. So we still need "lwsync" for
the two stores for lock and esel_next.
I was going to say that esel_next is just a hint and it doesn't really matter
if we occasionally get the wrong value, unless it happens often enough to
cause more performance degradation than the lwsync causes. However, with the
A-008139 workaround we do need to read the same value from esel_next both
times. It might be less costly to save/restore an additional register
instead of lwsync, though.
I will try to get some benchmark number to compare which method is a bit better.
Do you have any recommended benchmark for a case this is?
Thanks,
Kevin
From: Scott Wood <hidden> Date: 2015-08-15 00:44:36
On Fri, 2015-08-14 at 15:13 +0800, Kevin Hao wrote:
On Thu, Aug 13, 2015 at 10:39:19PM -0500, Scott Wood wrote:
quoted
On Thu, 2015-08-13 at 19:51 +0800, Kevin Hao wrote:
quoted
I didn't find anything unusual. But I think we do need to order the
load/store of esel_next when acquire/release tcd lock. For acquire,
add a data dependency to order the loads of lock and esel_next.
For release, even there already have a "isync" here, but it doesn't
guarantee any memory access order. So we still need "lwsync" for
the two stores for lock and esel_next.
I was going to say that esel_next is just a hint and it doesn't really
matter
if we occasionally get the wrong value, unless it happens often enough to
cause more performance degradation than the lwsync causes. However, with
the
A-008139 workaround we do need to read the same value from esel_next both
times. It might be less costly to save/restore an additional register
instead of lwsync, though.
I will try to get some benchmark number to compare which method is a bit
better.
Do you have any recommended benchmark for a case this is?
lmbench lat_mem_rd with a stride chosen to maximize TLB misses. For the
uncontended case, one instance; for the contended case, two instances, one
pinned to each thread of a core.
-Scott
From: Scott Wood <hidden> Date: 2015-08-15 02:44:45
On Fri, 2015-08-14 at 15:13 +0800, Kevin Hao wrote:
On Thu, Aug 13, 2015 at 01:44:43PM -0500, Scott Wood wrote:
quoted
On Thu, 2015-08-13 at 19:51 +0800, Kevin Hao wrote:
quoted
It makes no sense to put the instructions for calculating the lock
value (cpu number + 1) and the clearing of eq bit of cr1 in lbarx/stbcx
loop. And when the lock is acquired by the other thread, the current
lock value has no chance to equal with the lock value used by current
cpu. So we can skip the comparing for these two lock values in the
lbz/bne loop.
Signed-off-by: Kevin Hao <redacted>
---
arch/powerpc/mm/tlb_low_64e.S | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/mm/tlb_low_64e.S
b/arch/powerpc/mm/tlb_low_64e.S
index 765b419883f2..e4185581c5a7 100644
You're optimizing the contended case at the expense of introducing stalls
in
the uncontended case.
Before the patch, the uncontended case code sequence are:
1: lbarx r15,0,r11
lhz r10,PACAPACAINDEX(r13)
cmpdi r15,0
cmpdi cr1,r15,1 /* set cr1.eq = 0 for non-recursive */
addi r10,r10,1
bne 2f
stbcx. r10,0,r11
bne 1b
After the patch:
lhz r10,PACAPACAINDEX(r13)
addi r10,r10,1
crclr cr1*4+eq /* set cr1.eq = 0 for non-recursive */
1: lbarx r15,0,r11
cmpdi r15,0
bne 2f
stbcx. r10,0,r11
bne 1b
As you know, the lbarx is a Presync instruction and stbcx is a Presync and
Postsync instruction.
Yes, so don't we want to move instructions after the lbarx if possible, so
that the presync condition is achieved sooner?
Putting the unnecessary instructions in the lbarx/stbcx
loop also serialize these instructions execution.
Again, the common case should be that the loop executes only once. The two
cmpdi instructions should pair, the addi should pair with the bne, and the
lhz should happen while waiting for the lbarx result. My understanding of
how to model this stuff is certainly imperfect/incomplete, so I generally try
to confirm by testing, but I think both loops take the same number of cycles
per iteration.
The execution latency of
lbarx is only 3 cycles and there are still two instructions after it.
Considering the out of order execution optimization after this patch, do you
really think that new uncontended path will become slower due to this
additional stall?
The (theoretical) additional time is before the loop, not during it.
quoted
Does it really matter if there are more instructions
in the loop?
I really think we should minimize the window of lbarx/stbcx, for following
two
reasons:
- The bigger of this window, the more possible conflicts between the two
threads run into this loop simultaneously.
That's more true of the total time the lock is held, not the lbarx/stbcx
section.
- The reservation used by lbarx may be cleared by another thread due to
store to the same reservation granule. The smaller the window of
lbarx/stbcx, the less possibility to be affected by this.
There's only one other thread that should be touching that reservation
granule, and it's the one we're waiting for.
In any case, if there is a difference in loop iteration execution time, it's
small.
quoted
This change just means that you'll spin in the loop for more
iterations (if it even does that -- I think the cycles per loop iteration
might be the same before and after, due to load latency and pairing)
while
waiting for the other thread to release the lock.
Besides the optimization for the contended case, it also make the code more
readable with these changes:
- It always seem a bit weird to calculate the lock value for the current
cpu in the lbarx/stbcx loop.
- The "cmpdi cr1,r15,1" seems pretty confusion. It doesn't always do
what
the comment said (set cr1.eq = 0). In some cases, it does set the
crq.eq = 1, such as when running on cpu 1 with lock is acquired by cpu0.
All we need to do just clear the cr1.eq unconditionally.
We only care about cr1.eq when we break out of the loop, in which case r15
will have been zero. But yes, crclr is better.
quoted
Do you have any benchmark results for this patch?
I doubt it will get any visible difference. Anyway I will gave it a try.
I tried a couple different benchmarks and didn't find a significant
difference, relative to the variability of the results running on the same
kernel. A patch that claims to "optimize a bit" as its main purpose ought to
show some results. :-)
-Scott
From: Kevin Hao <hidden> Date: 2015-08-17 11:17:07
On Fri, Aug 14, 2015 at 09:44:28PM -0500, Scott Wood wrote:
I tried a couple different benchmarks and didn't find a significant
difference, relative to the variability of the results running on the same
kernel. A patch that claims to "optimize a bit" as its main purpose ought to
show some results. :-)
I tried to compare the execution time of these two code sequences with the
following test module:
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/printk.h>
static void test1(void)
{
int i;
unsigned char lock, c;
unsigned short cpu, s;
for (i = 0; i < 100000; i++) {
lock = 0;
cpu = 1;
asm volatile (
"1: lbarx %0,0,%2\n\
lhz %1,0(%3)\n\
cmpdi %0,0\n\
cmpdi cr1,%1,1\n\
addi %1,%1,1\n\
bne 2f\n\
stbcx. %1,0,%2\n\
bne 1b\n\
2:"
: "=&r" (c), "=&r" (s) : "r" (&lock), "r" (&cpu) : "cr0", "cr1", "memory");
}
}
static void test2(void)
{
int i;
unsigned char lock, c;
unsigned short cpu, s;
for (i = 0; i < 100000; i++) {
lock = 0;
cpu = 1;
asm volatile (
" lhz %1,0(%3)\n\
addi %1,%1,1\n\
crclr cr1*4+eq\n\
1: lbarx %0,0,%2\n\
cmpdi %0,0\n\
bne 2f\n\
stbcx. %1,0,%2\n\
bne 1b\n\
2:"
: "=&r" (c), "=&r" (s) : "r" (&lock), "r" (&cpu) : "cr0", "cr1", "memory");
}
}
static int test_init(void)
{
unsigned long s, e, tm1, tm2;
__hard_irq_disable();
/* Just for prefetch */
test1();
s = mftb();
test1();
e = mftb();
tm1 = e - s;
/* Just for prefetch */
test2();
s = mftb();
test2();
e = mftb();
tm2 = e - s;
__hard_irq_enable();
pr_err("test1: %ld, test2: %ld, %%%ld\n", tm1, tm2, (tm1 - tm2) * 100 / tm1);
return 0;
}
static void test_exit(void)
{
return;
}
module_init(test_init);
module_exit(test_exit);
MODULE_LICENSE("GPL");
The results:
test1: 156568, test2: 151675, %3
test1: 156604, test2: 151670, %3
test1: 156567, test2: 151684, %3
test1: 156567, test2: 151678, %3
test1: 156567, test2: 151688, %3
test1: 156570, test2: 151683, %3
test1: 156565, test2: 151675, %3
test1: 156565, test2: 151673, %3
It seems that there do have a %3 gain in performance by moving the
3 instructions out of lbarx/stbcx loop.
Thanks,
Kevin
From: Kevin Hao <hidden> Date: 2015-08-17 11:19:28
On Fri, Aug 14, 2015 at 07:44:23PM -0500, Scott Wood wrote:
On Fri, 2015-08-14 at 15:13 +0800, Kevin Hao wrote:
quoted
On Thu, Aug 13, 2015 at 10:39:19PM -0500, Scott Wood wrote:
quoted
On Thu, 2015-08-13 at 19:51 +0800, Kevin Hao wrote:
quoted
I didn't find anything unusual. But I think we do need to order the
load/store of esel_next when acquire/release tcd lock. For acquire,
add a data dependency to order the loads of lock and esel_next.
For release, even there already have a "isync" here, but it doesn't
guarantee any memory access order. So we still need "lwsync" for
the two stores for lock and esel_next.
I was going to say that esel_next is just a hint and it doesn't really
matter
if we occasionally get the wrong value, unless it happens often enough to
cause more performance degradation than the lwsync causes. However, with
the
A-008139 workaround we do need to read the same value from esel_next both
times. It might be less costly to save/restore an additional register
instead of lwsync, though.
I will try to get some benchmark number to compare which method is a bit
better.
Do you have any recommended benchmark for a case this is?
lmbench lat_mem_rd with a stride chosen to maximize TLB misses. For the
uncontended case, one instance; for the contended case, two instances, one
pinned to each thread of a core.
I have tried the method to save/restore an additional register for the esel
with the following codes:
From: Scott Wood <hidden> Date: 2015-08-17 21:08:31
On Mon, 2015-08-17 at 19:16 +0800, Kevin Hao wrote:
On Fri, Aug 14, 2015 at 09:44:28PM -0500, Scott Wood wrote:
quoted
I tried a couple different benchmarks and didn't find a significant
difference, relative to the variability of the results running on the
same
kernel. A patch that claims to "optimize a bit" as its main purpose
ought to
show some results. :-)
I tried to compare the execution time of these two code sequences with the
following test module:
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/printk.h>
static void test1(void)
{
int i;
unsigned char lock, c;
unsigned short cpu, s;
for (i = 0; i < 100000; i++) {
lock = 0;
cpu = 1;
asm volatile (
"1: lbarx %0,0,%2\n\
lhz %1,0(%3)\n\
cmpdi %0,0\n\
cmpdi cr1,%1,1\n\
This should be either "cmpdi cr1,%0,1" or crclr, not that it made much
difference. The test seemed to be rather sensitive to additional
instructions inserted at the beginning of the asm statement (especially
isync), so the initial instructions before the loop are probably pairing with
something outside the asm.
That said, it looks like this patch at least doesn't make things worse, and
does convert cmpdi to a more readable crclr, so I guess I'll apply it even
though it doesn't show any measurable benefit when testing entire TLB misses
(much less actual applications).
I suspect the point where I misunderstood the core manual was where it listed
lbarx as having a repeat-rate of 3 cycles. I probably assumed that that was
because of the presync, and thus a subsequent unrelated load could execute
partially in parallel, but it looks like the repeat rate is specifically
talking about how long it is until the execution unit can accept any other
instruction.
-Scott
From: Kevin Hao <hidden> Date: 2015-08-18 07:56:28
In order to workaround Erratum A-008139, we have to invalidate the
tlb entry with tlbilx before overwriting. Due to the performance
consideration, we don't add any memory barrier when acquire/release
the tcd lock. This means the two load instructions for esel_next do
have the possibility to return different value. This is definitely
not acceptable due to the Erratum A-008139. We have two options to
fix this issue:
a) Add memory barrier when acquire/release tcd lock to order the
load/store to esel_next.
b) Just make sure to invalidate and write to the same tlb entry and
tolerate the race that we may get the wrong value and overwrite
the tlb entry just updated by the other thread.
We observe better performance using option b. So reserve an additional
register to save the value of the esel_next.
Signed-off-by: Kevin Hao <redacted>
---
v2: Use an additional register for saving the value of esel_next instead of lwsync.
arch/powerpc/include/asm/exception-64e.h | 11 ++++++-----
arch/powerpc/mm/tlb_low_64e.S | 26 ++++++++++++++++++--------
2 files changed, 24 insertions(+), 13 deletions(-)