Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ppc.h: use correct cacheline size for macOS #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

barracuda156
Copy link
Contributor

Commit b4e52e3 unconditionally switched to 256 for PowerPC, which is wrong, AFAIK.
Fallback to 128 for Apple case at least.

@mgood7123
Copy link

anyone gonna merge ?

@compudj
Copy link
Contributor

compudj commented Mar 23, 2024

This commit lacks supporting material:

  • references to the Apple PowerPC hardware specifications supporting this change,
  • discussion about whether Apple software can run (or not) on POWER5+, which has L3 caches with 256 bytes cache line size,
  • discussion of which use-cases benefit from a smaller cache line size value, justifying the tweak specifically for Apple.

@barracuda156
Copy link
Contributor Author

@compudj

  1. From G5 specs is looks that the right value is 128: https://www.7-cpu.com/cpu/PowerPC970.html
    https://www.evl.uic.edu/julian/cs466/rep_g5.pdf
    This should not be something really controversial, since the code here also used 128 prior to a commit I referred b4e52e3

  2. No, it is not possible. You could emulate G5 on a modern POWER, but then the values for G5 will still hold.

  3. If we can detect G5 instead of conditioning on the OS, it is even better, however a) it is not really trivial, AFAIK, though possible, b) conditioning on Apple is a single line safe fix (we are sure that macOS is running on G5 or earlier), essentially just restoring the earlier code which existing for a reason, I believe.

@compudj
Copy link
Contributor

compudj commented Mar 28, 2024

I have prepared a commit that documents further the choice of 256 bytes, see https://review.lttng.org/c/userspace-rcu/+/12210 .

Given that this value is exposed in a public header, and used in public data structures, it is part of the liburcu ABI. We cannot change this value without a soname bump. Also, having different values between toolchains would make the executables generated by one toolchain ABI-incompatible with those generated with another. This is not an approach I want to take.

I would be open to change the powerpc cache line size value to 128 at the next liburcu soname bump, because POWER5 hardware has been EOL for 15 years now, and POWER 6, 7, 8, 9 CPUs went back to 128 bytes cache lines. So we can keep this in mind for the next soname bump, but I cannot guarantee how soon it will happen.

@compudj
Copy link
Contributor

compudj commented Mar 28, 2024

Note that I've asked around on the linuxppc mailing list and got relevant context: https://lists.ozlabs.org/pipermail/linuxppc-dev/2024-March/thread.html#269930

@barracuda156
Copy link
Contributor Author

Note that I've asked around on the linuxppc mailing list and got relevant context: https://lists.ozlabs.org/pipermail/linuxppc-dev/2024-March/thread.html#269930

@compudj Thank you for raising the issue. So the consensus appears to be that 128 is the appropriate value?

I have found a reference in IBM docs for G5 (PowerPC 970, which is the last one where macOS can run): https://www.redbooks.ibm.com/redpapers/pdfs/redp3890.pdf (page 4).
I think G5 does not have L3, while L1 and L2 use 128 bytes.

@compudj
Copy link
Contributor

compudj commented Mar 28, 2024

Yes, I think 128 bytes is appropriate nowadays, considering that only specific hardware had 256 bytes cache lines (POWER5) and has been EOL for a long time.

I arrived at the same conclusion for the G5 PowerPC 970: no L3, 128 bytes cache line size for L1 (data+insn) and L2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants