| Message ID | 1536781219-13938-12-git-send-email-leitao@debian.org (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | New TM Model | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | success | Test checkpatch on branch next |
| snowpatch_ozlabs/build-ppc64le | success | Test build-ppc64le on branch next |
| snowpatch_ozlabs/build-ppc64be | success | Test build-ppc64be on branch next |
| snowpatch_ozlabs/build-ppc64e | success | Test build-ppc64e on branch next |
| snowpatch_ozlabs/build-ppc32 | success | Test build-ppc32 on branch next |
On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote: > The Documentation/powerpc/transactional_memory.txt says: > > "Syscalls made from within a suspended transaction are performed as normal > and the transaction is not explicitly doomed by the kernel. However, > what the kernel does to perform the syscall may result in the transaction > being doomed by the hardware." > > With this new TM mechanism, the syscall will continue to be executed if the > syscall happens on a suspended syscall, but, the syscall will *fail* if the > transaction is still active during the syscall invocation. Not sure I get this. This doesn't seem any different to before. An active (not suspended) transaction *will* result in the syscall failing and the transaction being doomed. A syscall in a suspended transaction should succeed and the transaction. You might need to clean up the language. I try to use: Active == transactional but not suspended (ie MSR[TS] = T) Suspended == suspended (ie MSR [TS] = S) Doomed == transaction to be rolled back at next opportinity (ie tcheck returns doomed) (note: the kernel MSR_TM_ACTIVE() macro is not consistent with this since it's MSR[TS] == S or T). > On the syscall path, if the transaction is active and not suspended, it > will call TM_KERNEL_ENTRY which will reclaim and recheckpoint the > transaction, thus, dooming the transaction on userspace return, with > failure code TM_CAUSE_SYSCALL. But the test below is on a suspend transaction? > This new model will break part of this test, but I understand that that the > documentation above didn't guarantee that the syscall would succeed, and it > will never succeed anymore now on. The syscall should pass in suspend (modulo the normal syscall checks). The transaction may fail as a result. > In fact, glibc is calling 'tabort' before every syscalls, thus, any syscall > called through glibc from inside a transaction will be doomed anyhow. > > This patch updates the test case to not assume that a syscall inside a > active transaction will succeed, because it will not anymore. > > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > tools/testing/selftests/powerpc/tm/tm-syscall.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall.c > b/tools/testing/selftests/powerpc/tm/tm-syscall.c > index 454b965a2db3..1439a87eba3a 100644 > --- a/tools/testing/selftests/powerpc/tm/tm-syscall.c > +++ b/tools/testing/selftests/powerpc/tm/tm-syscall.c > @@ -78,12 +78,6 @@ int tm_syscall(void) > timeradd(&end, &now, &end); > > for (count = 0; timercmp(&now, &end, <); count++) { > - /* > - * Test a syscall within a suspended transaction and verify > - * that it succeeds. > - */ > - FAIL_IF(getppid_tm(true) == -1); /* Should succeed. */ > - > /* > * Test a syscall within an active transaction and verify that > * it fails with the correct failure code.
Hi Mikey, On 09/18/2018 03:36 AM, Michael Neuling wrote: > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote: >> The Documentation/powerpc/transactional_memory.txt says: >> >> "Syscalls made from within a suspended transaction are performed as normal >> and the transaction is not explicitly doomed by the kernel. However, >> what the kernel does to perform the syscall may result in the transaction >> being doomed by the hardware." >> >> With this new TM mechanism, the syscall will continue to be executed if the >> syscall happens on a suspended syscall, but, the syscall will *fail* if the >> transaction is still active during the syscall invocation. > > Not sure I get this. This doesn't seem any different to before. > > An active (not suspended) transaction *will* result in the syscall failing and > the transaction being doomed. > > A syscall in a suspended transaction should succeed and the transaction. I understand that a transaction will always be doomed if there is a reclaim/checkpoint, thus, if the you make a system call inside a suspended transaction, it will reclaim and recheckpoint, thus, dooming the transaction, and failing it on the next RFID. So, the syscall executed in suspended mode may succeed, because it will not be skipped (as in active mode), but the transaction will *always* fail, because there was a reclaim and recheckpoint. > You might need to clean up the language. I try to use: > > Active == transactional but not suspended (ie MSR[TS] = T) > Suspended == suspended (ie MSR [TS] = S) > Doomed == transaction to be rolled back at next opportinity (ie tcheck returns doomed) > > (note: the kernel MSR_TM_ACTIVE() macro is not consistent with this since it's > MSR[TS] == S or T). Ok, I agree with this language as well. I might want to improve the code to follow the same language all across the code. >> On the syscall path, if the transaction is active and not suspended, it >> will call TM_KERNEL_ENTRY which will reclaim and recheckpoint the >> transaction, thus, dooming the transaction on userspace return, with >> failure code TM_CAUSE_SYSCALL. > > But the test below is on a suspend transaction? Sorry, I meant "suspended transaction" above instead of "transaction is active and not suspended". > >> This new model will break part of this test, but I understand that that the >> documentation above didn't guarantee that the syscall would succeed, and it >> will never succeed anymore now on. > > The syscall should pass in suspend (modulo the normal syscall checks). The > transaction may fail as a result. Perfect, and if the transaction fail, the CPU will rollback the changes and restore the checkpoint registers (replacing the r3 that contains the pid value), thus, it will be like "getpid" system call didn't execute. For this test specifically, it assumes the syscall didn't execute if the transaction failed. Take a look: FUNC_START(getppid_tm_suspended) tbegin. beq 1f li r0, __NR_getppid tsuspend. sc tresume. tend. blr 1: li r3, -1 blr So, the tests thinks the syscall failed because the transaction aborted. Anyway, I can try to improve this test other than removing this test, but I am not sure how. Thank you
On Thu, 2018-09-27 at 17:57 -0300, Breno Leitao wrote: > Hi Mikey, > > On 09/18/2018 03:36 AM, Michael Neuling wrote: > > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote: > > > The Documentation/powerpc/transactional_memory.txt says: > > > > > > "Syscalls made from within a suspended transaction are performed as > > > normal > > > and the transaction is not explicitly doomed by the kernel. However, > > > what the kernel does to perform the syscall may result in the > > > transaction > > > being doomed by the hardware." > > > > > > With this new TM mechanism, the syscall will continue to be executed if > > > the > > > syscall happens on a suspended syscall, but, the syscall will *fail* if > > > the > > > transaction is still active during the syscall invocation. > > > > Not sure I get this. This doesn't seem any different to before. > > > > An active (not suspended) transaction *will* result in the syscall failing > > and > > the transaction being doomed. > > > > A syscall in a suspended transaction should succeed and the transaction. > > I understand that a transaction will always be doomed if there is a > reclaim/checkpoint, thus, if the you make a system call inside a suspended > transaction, it will reclaim and recheckpoint, thus, dooming the transaction, > and failing it on the next RFID. So currently a syscall won't explicitly treclaim/trecheckpoint so it won't necessarily be doomed. With your new patches it will be. > So, the syscall executed in suspended mode may succeed, because it will not > be skipped (as in active mode), but the transaction will *always* fail, > because there was a reclaim and recheckpoint. > > > You might need to clean up the language. I try to use: > > > > Active == transactional but not suspended (ie MSR[TS] = T) > > Suspended == suspended (ie MSR [TS] = S) > > Doomed == transaction to be rolled back at next opportinity (ie tcheck > > returns doomed) > > > > (note: the kernel MSR_TM_ACTIVE() macro is not consistent with this since > > it's > > MSR[TS] == S or T). > > Ok, I agree with this language as well. I might want to improve the code to > follow the same language all across the code. > > > > On the syscall path, if the transaction is active and not suspended, it > > > will call TM_KERNEL_ENTRY which will reclaim and recheckpoint the > > > transaction, thus, dooming the transaction on userspace return, with > > > failure code TM_CAUSE_SYSCALL. > > > > But the test below is on a suspend transaction? > > Sorry, I meant "suspended transaction" above instead of "transaction is > active and not suspended". > > > > > > This new model will break part of this test, but I understand that that > > > the > > > documentation above didn't guarantee that the syscall would succeed, and > > > it > > > will never succeed anymore now on. > > > > The syscall should pass in suspend (modulo the normal syscall checks). The > > transaction may fail as a result. > > Perfect, and if the transaction fail, the CPU will rollback the changes and > restore the checkpoint registers (replacing the r3 that contains the pid > value), thus, it will be like "getpid" system call didn't execute. No. If we are suspended, then we go back right after the sc. We don't get rolled back till the tresume. Mikey > For this test specifically, it assumes the syscall didn't execute if the > transaction failed. Take a look: > > FUNC_START(getppid_tm_suspended) > tbegin. > beq 1f > li r0, __NR_getppid > tsuspend. > sc > tresume. > tend. > blr > 1: > li r3, -1 > blr > > So, the tests thinks the syscall failed because the transaction aborted. > > Anyway, I can try to improve this test other than removing this test, but I > am not sure how. > > Thank you >
Hi Mikey, On 09/28/2018 02:25 AM, Michael Neuling wrote: >> Perfect, and if the transaction fail, the CPU will rollback the changes and >> restore the checkpoint registers (replacing the r3 that contains the pid >> value), thus, it will be like "getpid" system call didn't execute. > > No. If we are suspended, then we go back right after the sc. We don't get > rolled back till the tresume. Yes, but the test code (below) just run tresume after 'sc'. i.e, the syscall will execute, but there is a tresume just after the syscall, which will cause the transaction to rollback and jump to "1:" label, which will replace r3. with -1. So, the difference now (with this patchset) is that we are calling treclaim/trecheckpoint in kernel space, which will doom the transaction. It was not being done before, thus, the test was passing and it is not anymore. Anyway, the other way to check for it is calling 'blr' just after 'sc' ( before 'tresume.'), and then tresume after checking if pid == getpid(). If you prefer this method, I can implement it. >> For this test specifically, it assumes the syscall didn't execute if the >> transaction failed. Take a look: >> >> FUNC_START(getppid_tm_suspended) >> tbegin. >> beq 1f >> li r0, __NR_getppid >> tsuspend. >> sc >> tresume. >> tend. >> blr >> 1: >> li r3, -1 >> blr >> Thank you!
diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall.c b/tools/testing/selftests/powerpc/tm/tm-syscall.c index 454b965a2db3..1439a87eba3a 100644 --- a/tools/testing/selftests/powerpc/tm/tm-syscall.c +++ b/tools/testing/selftests/powerpc/tm/tm-syscall.c @@ -78,12 +78,6 @@ int tm_syscall(void) timeradd(&end, &now, &end); for (count = 0; timercmp(&now, &end, <); count++) { - /* - * Test a syscall within a suspended transaction and verify - * that it succeeds. - */ - FAIL_IF(getppid_tm(true) == -1); /* Should succeed. */ - /* * Test a syscall within an active transaction and verify that * it fails with the correct failure code.
The Documentation/powerpc/transactional_memory.txt says: "Syscalls made from within a suspended transaction are performed as normal and the transaction is not explicitly doomed by the kernel. However, what the kernel does to perform the syscall may result in the transaction being doomed by the hardware." With this new TM mechanism, the syscall will continue to be executed if the syscall happens on a suspended syscall, but, the syscall will *fail* if the transaction is still active during the syscall invocation. On the syscall path, if the transaction is active and not suspended, it will call TM_KERNEL_ENTRY which will reclaim and recheckpoint the transaction, thus, dooming the transaction on userspace return, with failure code TM_CAUSE_SYSCALL. This new model will break part of this test, but I understand that that the documentation above didn't guarantee that the syscall would succeed, and it will never succeed anymore now on. In fact, glibc is calling 'tabort' before every syscalls, thus, any syscall called through glibc from inside a transaction will be doomed anyhow. This patch updates the test case to not assume that a syscall inside a active transaction will succeed, because it will not anymore. Signed-off-by: Breno Leitao <leitao@debian.org> --- tools/testing/selftests/powerpc/tm/tm-syscall.c | 6 ------ 1 file changed, 6 deletions(-)