Page MenuHomeFreeBSD
Differential D15243

Powerpc64: Enable Hardware Transactional Memory
AbandonedPublic
Actions

Authored by jhibbits on Apr 30 2018, 6:39 PM.
Tags
Referenced Files
Unknown Object (File)
Sun, Apr 14, 8:08 AM
Unknown Object (File)
Sun, Apr 14, 6:41 AM
Unknown Object (File)
Sun, Apr 14, 6:35 AM
Unknown Object (File)
Sun, Apr 14, 5:53 AM
Unknown Object (File)
Sun, Apr 14, 5:31 AM
Unknown Object (File)
Sun, Apr 14, 2:39 AM
Unknown Object (File)
Feb 20 2024, 12:19 PM
Unknown Object (File)
Feb 6 2024, 4:18 PM
View All 43 Files
Subscribers
alfredo
bdragon
jhibbits
kbowling
luporl

Details

Reviewers
nwhitehorn
breno.leitao_gmail.com
Summary

This patch enable Hardware Transactional Memory (HTM) facility.

The facility is only enabled on a process that executes a HTM instruction. It
will raise and facility trap, which will enable flag the process with PCB_HTM,
from there after, the context switch will save and restore the 3 SPRs that are
associated with HTM.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 18375
Build 18091: arc lint + arc unit

Event Timeline

breno.leitao_gmail.com created this revision.Apr 30 2018, 6:39 PM
Herald added a subscriber: jhibbits. · View Herald TranscriptApr 30 2018, 6:39 PM
Harbormaster completed remote builds in B16359: Diff 42012.Apr 30 2018, 6:39 PM
breno.leitao_gmail.com added reviewers: nwhitehorn, jhibbits.Apr 30 2018, 6:45 PM
kbowling added a subscriber: kbowling.Apr 30 2018, 8:51 PM
jhibbits added inline comments.May 9 2018, 2:31 PM
sys/powerpc/include/htm.h
45–51

Why not make these return bool? It looks like they're not used anywhere other than conditional checks.

sys/powerpc/powerpc/trap.c
303

I think SIGTRAP is the wrong signal here. Maybe SIGILL with ucode of ILL_ILLTRP (illegal trap)?

387

Maybe we want to abort the transaction and kill the process in here? SIGILL, or SIGTRAP, or such like that? I don't think it's right to just ignore a trap instruction.

Actually, reading above, the transaction is aborted already anyway at this point so why not follow through? I'm not sure what the right thing to do is. Maybe, like above, it should be SIGILL/ILL_ILLTRP.

Nathan, do you have an opinion?

breno.leitao_gmail.com updated this revision to Diff 42625.May 16 2018, 2:56 PM
breno.leitao_gmail.com marked an inline comment as done.

Changes according to the Justin's review:

  • Changed return type for htm_enabled, htm_transactional and htm_suspended
  • Returning SIGILL if calling systemcall (instead of SIGTRAP) inside a transaction.
  • Returning SIGILL instead of ignoring a trap instruction inside a transaction, thus, avoiding calling kdb_trap() also.
Harbormaster completed remote builds in B16624: Diff 42625.May 16 2018, 2:56 PM
breno.leitao_gmail.com added inline comments.May 16 2018, 2:57 PM
sys/powerpc/powerpc/trap.c
387

Hi Justin,

Right, the tabort just dommed the transaction, but it is still suspended, and it will be aborted when we return the userspace (rfid). In this case, the transaction will try to continue and it will fail, returning to the the next line after tbegin. (usually a bne XX).

I just created another revision with SIGILL/ILL_ILLTRP, but I needed to avoid adding a breakpoint inside the transaction, as showed in my new patch at db_trap_glue.

jhibbits added a comment.Jun 7 2018, 4:22 PM

The code looks fine to me. Can you include a test for it? You showed me the torture test, so that'd be good to include in tools/tools. With a test, I'll approve it.

breno.leitao_gmail.com updated this revision to Diff 45919.Jul 27 2018, 5:57 PM

Add aditional tests. Mainly two things:

  • Instead of testing for transactional states, look for active (Transactional + Suspended)
  • Avoid signal handling inside a transaction, since the code will be rolled back.
Harbormaster completed remote builds in B18375: Diff 45919.Jul 27 2018, 5:57 PM
breno.leitao_gmail.com updated this revision to Diff 47112.Aug 22 2018, 5:11 PM

Adding additional testsuite for the code.

  • Powerpc64: Enable Hardware Transactional Memory
  • Powerpc64: Add HTM example
Harbormaster completed remote builds in B19055: Diff 47112.Aug 22 2018, 5:11 PM
breno.leitao_gmail.com added a comment.Aug 22 2018, 5:21 PM

This patchset has one caveat at this time that will be fixed soon. If we go into kernel space during the transaction, and the process is rescheduled to another cpu, the transaction state will be on the original CPU, and the new CPU will not have the same CPU state, caused an undefined behaviour.

The right way to solve it is calling 'treclaim' (reclaim the checkpointed values from registers and save them in memory) and, later, calling trechekpoint (put the register back to the checkpointed area, i.e, loading the checkpointed registers, calling trecheckpoint, and restoring the GPRs, once the CPU is rescheduled.

In order to do so, I will need to call 'treclaim' instead of 'tabort' at the beginning of the execption (in file sys/powerpc/powerpc/trap.c). and implement treclaim, which is not very trivial, since it will put all the registers back to userspace while we are in kernel space. Later, call trecheckpoint when a new process, which was aborted in the middle of a transaction, is rescheduled back to the CPU.

breno.leitao_gmail.com updated this revision to Diff 47118.Aug 22 2018, 6:32 PM

Minor fixes

Harbormaster completed remote builds in B19060: Diff 47118.Aug 22 2018, 6:32 PM
jhibbits added a comment.Aug 24 2018, 5:04 PM

@breno.leitao_gmail.com Can you pull out the pcb.h and pcpu.h changes into a separate diff? That way we can get that in before the KBI freeze, and wait until 12.1 to get the actual code in. Once KBI is frozen it's frozen for the full 12.0 life, and any KBI breakage we want to perform will have to wait for 13.0.

sys/powerpc/include/pcpu.h
48

One thing to note is you need to adjust the padding down below, else it should CTASSERT. I'm surprised it doesn't already. This may have changed recently, though. Can you check to confirm?

breno.leitao_gmail.com added inline comments.Aug 24 2018, 9:42 PM
sys/powerpc/include/pcpu.h
48

Hi Justin,

Sure. I also included the struct pcb change, since I am not sure if that will also be considered kABI.

The change is at https://reviews.freebsd.org/D16889

jhibbits commandeered this revision.May 13 2019, 1:22 AM
jhibbits edited reviewers, added: breno.leitao_gmail.com; removed: jhibbits.

Will take this to completion.

Herald added a subscriber: bdragon. · View Herald TranscriptMay 13 2019, 1:22 AM
jhibbits added a project: PowerPC.May 12 2020, 12:12 AM

So, HTM is removed in PowerISA 3.1 and it's broken on POWER9. Is it even worth doing at this point? I'm inclined to abandon this.

Herald added subscribers: alfredo, luporl. · View Herald TranscriptMay 12 2020, 12:12 AM
nwhitehorn added a comment.May 12 2020, 4:03 AM
In D15243#545983, @jhibbits wrote:

So, HTM is removed in PowerISA 3.1 and it's broken on POWER9. Is it even worth doing at this point? I'm inclined to abandon this.

I think the answer is clearly "no", since those facts make the chance that any 3rd-party userland code wants the feature pretty close to zero.

jhibbits abandoned this revision.May 12 2020, 1:42 PM

Since PowerISA v3.1, and therefore POWER10, dropped this feature, maintaining only token support, and it's unusably broken on POWER9, dropping this work. Sorry @breno.leitao_gmail.com .

kbowling added a comment.May 12 2020, 4:01 PM

@jhibbits just curious where are you reading ISA 3.1 info?

jhibbits added a comment.May 12 2020, 4:13 PM

@kbowling it's available from https://www-355.ibm.com/systems/power/openpower/.

Revision Contents
Changeset List

PathSize
sys/
conf/
1 line
2 lines
powerpc/
aim/
5 lines
conf/
1 line
3 lines
include/
61 lines
6 lines
1 line
10 lines
3 lines
1 line
powerpc/
2 lines
216 lines
25 lines
59 lines

Diff 45919

View Options

sys/conf/files.powerpc

Loading...
View Options

sys/conf/options.powerpc

Loading...
View Options

sys/powerpc/aim/aim_machdep.c

Loading...
View Options

sys/powerpc/conf/GENERIC64

Loading...
View Options

sys/powerpc/conf/NOTES

Loading...
View Options

sys/powerpc/include/htm.h

Loading...
View Options

sys/powerpc/include/pcb.h

Loading...
View Options

sys/powerpc/include/pcpu.h

Loading...
View Options

sys/powerpc/include/psl.h

Loading...
View Options

sys/powerpc/include/spr.h

Loading...
View Options

sys/powerpc/include/trap.h

Loading...
View Options

sys/powerpc/powerpc/genassym.c

Loading...
View Options

sys/powerpc/powerpc/htm.c

Loading...
View Options

sys/powerpc/powerpc/swtch64.S

Loading...
View Options

sys/powerpc/powerpc/trap.c

Loading...