Re: [RFC PATCH 11/11] selftests/powerpc: Adapt the test

[Michael Neuling <mikey@neuling.org>]

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
>