Skip to content
This repository was archived by the owner on Nov 12, 2025. It is now read-only.

Comments

Fix data cache flush on ppc64#4844

Merged
pbalcer merged 1 commit intopmem:stable-1.8from
tuliom:fix-flush
Jun 17, 2020
Merged

Fix data cache flush on ppc64#4844
pbalcer merged 1 commit intopmem:stable-1.8from
tuliom:fix-flush

Conversation

@tuliom
Copy link
Contributor

@tuliom tuliom commented Jun 8, 2020

The dcbst instruction doesn't flush a cache block correctly because it
doesn't mark that block as clean on Power 9, see Manual "POWER9
Processor User’s Manual" page 66.
Replace dcbst with dcbf in order to fix this issue. This change requires
to replace lwsync with a hwsync too in order to cover for the usage of dcbf.

Adopt the best practices according to the POWER ISA 3.1.

Fix #4843 .


This change is Reviewable

@pmem-bot
Copy link
Contributor

pmem-bot commented Jun 8, 2020

@janekmi and @lplewa please review this pull request

@pbalcer
Copy link
Member

pbalcer commented Jun 9, 2020

Thanks for your contribution. The patch looks good, but the build is failing because of style checker. You can read our guidelines here: https://github.com/pmem/pmdk/blob/master/CODING_STYLE.md. It's also possible to run the checker locally with make cstyle.
One other thing I noticed is that the commit message doesn't follow our guidelines. In this case the first line should probably read:
pmem: fix data cache flush on ppc64.

@tuliom
Copy link
Contributor Author

tuliom commented Jun 9, 2020

@pbalcer I fixed the issue in the title of the commit, a long line in the commit message and included the bug number in the commit message, following the guidelines.
It's now passing make cstyle.

@tuliom
Copy link
Contributor Author

tuliom commented Jun 9, 2020

This last version should make the appveyor test a little bit less angry.
I'm afraid the last failure from appveyor is a false positive:

C:\projects\pmdk\src\libpmem\ppc64\platform_generic.c:83: missing space around one of operators: % %= / /=

Line 83 is an asm that uses %0.

@lplewa
Copy link
Member

lplewa commented Jun 10, 2020

You can create file .cstyleignore with the name of your file which cstyle should ignore.

here is an example of such file
https://github.com/pmem/pmdk/blob/master/src/common/.cstyleignore

The dcbst instruction doesn't flush a cache block correctly because it
doesn't mark that block as clean on Power 9, see Manual "POWER9
Processor User’s Manual" page 66.
Replace dcbst with dcbf in order to fix this issue. This change requires
to replace lwsync with a hwsync too in order to cover for the usage of
dcbf.

Adopt the best practices according to the POWER ISA 3.1.

Fix bug pmem#4843.

Signed-off-by: Tulio Magno Quites Machado Filho <tuliom@linux.ibm.com>
Copy link
Member

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@tuliom
Copy link
Contributor Author

tuliom commented Jun 10, 2020

The last version of my patch is now ignoring cstyle errors on src/libpmem/ppc64/platform_generic.c in order to avoid a false positive when using asm.

@lucmaga
Copy link
Contributor

lucmaga commented Jun 10, 2020

Thanks @tuliom for the patch.
@pbalcer Do we need a new PR for master or you will merge stable-1.8 fixes on master?

@pbalcer
Copy link
Member

pbalcer commented Jun 10, 2020

I think I can do it myself - should be easy enough.

@lplewa lplewa added this to the 1.9 milestone Jun 15, 2020
@pbalcer pbalcer merged commit 84ac7cd into pmem:stable-1.8 Jun 17, 2020
@pbalcer
Copy link
Member

pbalcer commented Jun 18, 2020

@lucmaga @tuliom there seems to be an issue with this patch on valgrind:


Last 30 lines of drd1.log below (whole file has 32 lines).
out_err_mt/TEST1 drd1.log ==76727== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
out_err_mt/TEST1 drd1.log ==76727== Command: ./out_err_mt /tmp//////test_out_err_mt1__________________PMDK_________/testfile1 /tmp//////test_out_err_mt1__________________PMDK_________/testfile2 /tmp//////test_out_err_mt1__________________PMDK_________/testfile3 /tmp//////test_out_err_mt1__________________PMDK_________/testfile4 /tmp//////test_out_err_mt1__________________PMDK_________
out_err_mt/TEST1 drd1.log ==76727== Parent PID: 76672
out_err_mt/TEST1 drd1.log ==76727== 
out_err_mt/TEST1 drd1.log dis_memsync(ppc)(sync/lwsync,flag_L)
out_err_mt/TEST1 drd1.log disInstr(ppc): unhandled instruction: 0x7C8004AC
out_err_mt/TEST1 drd1.log                  primary 31(0x1F), secondary 1196(0x4AC)
out_err_mt/TEST1 drd1.log ==76727== valgrind: Unrecognised instruction at address 0x4284758.
out_err_mt/TEST1 drd1.log ==76727==    at 0x4284758: ppc_fence (in /pmdk/src/nondebug/libpmem.so.1.0.0)
out_err_mt/TEST1 drd1.log ==76727==    by 0x427EB23: pmem_drain (in /pmdk/src/nondebug/libpmem.so.1.0.0)
out_err_mt/TEST1 drd1.log ==76727==    by 0x421D51F: shutdown_state_init (in /pmdk/src/nondebug/libpmemobj.so.1.0.0)
out_err_mt/TEST1 drd1.log ==76727==    by 0x421820F: util_header_create (in /pmdk/src/nondebug/libpmemobj.so.1.0.0)
out_err_mt/TEST1 drd1.log ==76727==    by 0x4219D17: util_replica_create_local (in /pmdk/src/nondebug/libpmemobj.so.1.0.0)
out_err_mt/TEST1 drd1.log ==76727==    by 0x421937F: util_pool_create_uuids (in /pmdk/src/nondebug/libpmemobj.so.1.0.0)
out_err_mt/TEST1 drd1.log ==76727==    by 0x4219F9F: util_pool_create (in /pmdk/src/nondebug/libpmemobj.so.1.0.0)
out_err_mt/TEST1 drd1.log ==76727==    by 0x4232537: pmemobj_create (in /pmdk/src/nondebug/libpmemobj.so.1.0.0)
out_err_mt/TEST1 drd1.log ==76727==    by 0x10003F8B: main (out_err_mt.c:112)
out_err_mt/TEST1 drd1.log ==76727== Your program just tried to execute an instruction that Valgrind
out_err_mt/TEST1 drd1.log ==76727== did not recognise.  There are two possible reasons for this.
out_err_mt/TEST1 drd1.log ==76727== 1. Your program has a bug and erroneously jumped to a non-code
out_err_mt/TEST1 drd1.log ==76727==    location.  If you are running Memcheck and you just saw a
out_err_mt/TEST1 drd1.log ==76727==    warning about a bad jump, it's probably your program's fault.
out_err_mt/TEST1 drd1.log ==76727== 2. The instruction is legitimate but Valgrind doesn't handle it,
out_err_mt/TEST1 drd1.log ==76727==    i.e. it's Valgrind's fault.  If you think this is the case or
out_err_mt/TEST1 drd1.log ==76727==    you are not sure, please let us know and we'll try to fix it.
out_err_mt/TEST1 drd1.log ==76727== Either way, Valgrind will now raise a SIGILL signal which will
out_err_mt/TEST1 drd1.log ==76727== probably kill your program.
out_err_mt/TEST1 drd1.log ==76727== 
out_err_mt/TEST1 drd1.log ==76727== For lists of detected and suppressed errors, rerun with: -s
out_err_mt/TEST1 drd1.log ==76727== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

(Full log here: https://api.travis-ci.org/v3/job/699647959/log.txt)
The new fence instruction doesn't seem to be recognized by valgrind, this prevents the CI from completing successfully.

Since we are about to tag 1.9 RC1, and we are short on time, we've made the decision to temporarily switch over to using msync when running valgrind tests on PPC. But it would be better to add support for the new instruction to valgrind.

This didn't come up with this PR, because travis is not running on stable-1.8 branch.

@tuliom
Copy link
Contributor Author

tuliom commented Jun 18, 2020

Sorry, I forgot to mention this Valgrind issue: https://bugs.kde.org/show_bug.cgi?id=422677
It has been fixed in Valgrind master (unreleased) and I have to report this issue to distros.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants