Enable Pmemcheck on PPC64 by shivaprasadbhat · Pull Request #90 · pmem/valgrind

shivaprasadbhat · 2022-12-21T08:14:15Z

The patches here attempt add support for pmemcheck on PPC64 machines.

The first three commits
459bce0 Powerpc, add support for the sync instruction with L = 2, 3 and 5.
95e6ae4 Powerpc, dcbf add check for L field and ISA version.
474f8c7 PowerPC: Fix the L field for the sync and dcbf instructions.
are backports of the upstream commits available on tag VALGRIND_3_20_0.

The remaining 6 commits would be private to the pmem/valgrind tree. Out of those 6,
two commits just move around the existing tests to arch specific directories.

I have tested the changes using pmdk's unittests on

amd64: on laptop with qemu, virtual nvdimm
ppc64: POWER10 and POWER9 machines

also valgrind's regtest on amd64 shows no regressions on my setup and travis(internal).

Kindly give your feedback on the changes.

This change is

lukaszstolarczuk

@kilobyte, do you feel like testing this patch on your setup(s)...?

Reviewed 44 of 53 files at r2.
Reviewable status: 44 of 53 files reviewed, all discussions resolved

kilobyte · 2022-12-21T15:23:33Z

pmemcheck/pmc_replace_memcmp.c

+   /* So figure out if [loS .. hiS] overlaps with [loD .. hiD]. */
+   if (loS < loD) {
+      return !(hiS < loD);
+   }
+   else if (loD < loS) {
+      return !(hiD < loS);
+   }
+   else {
+      /* They start at same place.  Since we know neither of them has
+         zero length, they must overlap. */
+      return True;
+   }
+}


This logic can be simplified to hiS<loD || hiD<loS

kilobyte · 2022-12-21T15:28:53Z

I don't have any ppc hardware to test on, nor am I knowledgeable about the architecture.
But as for a mere eyeball review, all changes look plausible.

I've spotted a single possible optimization but that's just a "if you're bored" thing.

Thus: LGTM.

shivaprasadbhat · 2022-12-22T14:51:08Z

I've spotted a single possible optimization but that's just a "if you're bored" thing.

Thanks for the quick review. The optimisation would work for same length source and destination(true with current usage of memset|cmp|cpy), I might have to change is_overlap() to accept single length argument to avoid breaking it with differing lengths if someone uses it in future.

Let me know if you really want me to fix it.

Thus: LGTM.

Thank you. Let me know if you need any more data points/test results etc. to get this merged.

wlemkows · 2023-01-04T13:12:00Z

Please rebase your PR to the new default branch pmem-3.20

Intrumenting the ppc64 floating point form of store instructions need Iop_ReinterpF32asI64 to be defined. Reviewed-By: Carl Love <cel@us.ibm.com> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>

The subsequent patches add ppc64 support for the pmemcheck. Prepare to have independent test results for consistency. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>

For ppc64, insert Imbe_SFence for the 'phwsync' and Ist_Flush for 'dcbf'. The ppc64 uses Ist_Fence for other instructions like eieio, ptesync, lwsync which dont work on persistent storage. So, ignore them on pmemcheck. These instructions(phwsync & dcbf) came with Power ISA 3.1[1], however Power ISA 3.0[2] too support the functionality. The patch carefully instruments those opcode variants for Power ISA 3.0 wherever applicable. References: [1] : https://files.openpower.foundation/s/dAYSdGzTfW4j2r2 [2] : https://files.openpower.foundation/s/XXFoRATEzSFtdG8 Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>

The valgrind infrastructure deduces the cache line size correctly using the aux vectors for PPC64. Use icache line size for now as it is same for dcache line too on the existing POWER processors. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>

The glibc implementation of memset, memcmp and memcpy for POWER10 has optimisations which makes the Valgrind break. So, just like memcheck and dhat, use own simplified versions for these functions to avoid the breakages. The patch selectively replicates the existing definitions from the generic vg_replace_strmem.c for pmemcheck use, and overrides only the POWER10 specific implementations from glibc. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>

The cache line size is 128 bytes on ppc64, and is 64 bytes on amd64. This necessiated moving few of the tests to separate Arch specific directories. The patch adds those specific tests into the ppc64 specific directory. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>

shivaprasadbhat · 2023-01-08T17:08:09Z

Please rebase your PR to the new default branch pmem-3.20

Done.

Thanks!

lukaszstolarczuk · 2023-01-09T08:43:17Z

Hi, thanks for the update.

As you stated in your original description:

are backports of the upstream commits available on tag VALGRIND_3_20_0

perhaps now (on pmem-3.20), these commits are not required in this PR...? Either this or something else, but you have a conflict with the base branch, now...

shivaprasadbhat · 2023-01-09T09:12:17Z

Hi, thanks for the update.

As you stated in your original description:

are backports of the upstream commits available on tag VALGRIND_3_20_0

perhaps now (on pmem-3.20), these commits are not required in this PR...? Either this or something else, but you have a conflict with the base branch, now...

The 3.20 pulled in those 3 into the branch already. My pull request now is for the remaining 6 patches.

I dont see a conflict on the pull request, as the number of commits now is 6, and it was 9 before. Do you see conflict? Am I missing something?

Thanks!

lukaszstolarczuk

huh, sorry, I think it was my imagination 😄 something didn't refresh on my side, I guess.
Apologies!
And thanks again!

I'll just simply verify now if it works correctly with PMDK repository.

Reviewable status: 40 of 53 files reviewed, all discussions resolved

shivaprasadbhat · 2023-01-09T10:38:47Z

I'll just simply verify now if it works correctly with PMDK repository.

If verifying on PPC, some PMDK test specific fixes are required for few of those tests to pass. Let me know if you want me to push them to my PMDK tree for you to pick. They need some more cleanup though, so holding back.

Thanks!

lukaszstolarczuk · 2023-01-09T12:16:01Z

I'll just simply verify now if it works correctly with PMDK repository.

If verifying on PPC, some PMDK test specific fixes are required for few of those tests to pass. Let me know if you want me to push them to my PMDK tree for you to pick. They need some more cleanup though, so holding back.

Thanks!

Great!
If you have fixes ready for PMDK, then, please, go ahead and push them to the PMDK repo.

shivaprasadbhat · 2023-01-11T14:48:11Z

I'll just simply verify now if it works correctly with PMDK repository.

If verifying on PPC, some PMDK test specific fixes are required for few of those tests to pass. Let me know if you want me to push them to my PMDK tree for you to pick. They need some more cleanup though, so holding back.
Thanks!

Great! If you have fixes ready for PMDK, then, please, go ahead and push them to the PMDK repo.

I just sent the pull request on PMDK tree with those fixes here pmem/pmdk#5518

Thanks!

shivaprasadbhat · 2023-02-01T13:42:45Z

Thank you Weronika!

Regards,
Shivaprasad

lukaszstolarczuk reviewed Dec 21, 2022

View reviewed changes

kilobyte reviewed Dec 21, 2022

View reviewed changes

lukaszstolarczuk mentioned this pull request Jan 4, 2023

docker: bump valgrind to release 3.20 pmem/dev-utils-kit#15

Draft

1 task

ShivaprasadGBhat added 5 commits January 4, 2023 09:04

ppc64: Implement Iop_ReinterpF32asI64

95c4e87

Intrumenting the ppc64 floating point form of store instructions need Iop_ReinterpF32asI64 to be defined. Reviewed-By: Carl Love <cel@us.ibm.com> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>

pmemcheck: tests: Move amd64 specific tests to separate directory

0c7ce0e

The subsequent patches add ppc64 support for the pmemcheck. Prepare to have independent test results for consistency. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>

lukaszstolarczuk mentioned this pull request Jan 4, 2023

common: update valgrind ver to 3.20 pmem/pmdk#5514

Merged

shivaprasadbhat force-pushed the pmemcheck-ppc64 branch from cc79f9c to c667cce Compare January 8, 2023 17:06

shivaprasadbhat changed the base branch from pmem-3.19 to pmem-3.20 January 8, 2023 17:06

lukaszstolarczuk reviewed Jan 9, 2023

View reviewed changes

shivaprasadbhat mentioned this pull request Jan 11, 2023

Revisit the tests with pmemcheck support on ppc64le pmem/pmdk#5518

Merged

wlemkows merged commit c0abd81 into pmem:pmem-3.20 Feb 1, 2023

lukaszstolarczuk mentioned this pull request Feb 1, 2023

common: bump valgrind version pmem/pmdk#5530

Merged

pbalcer mentioned this pull request Apr 29, 2024

ppc64el obj_basic_integration/TEST5 crashed on valgrind (debian + ubuntu; ppc64el) pmem/pmdk#6079

Closed

Conversation

shivaprasadbhat commented Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

Uh oh!

kilobyte Dec 21, 2022

Choose a reason for hiding this comment

Uh oh!

kilobyte commented Dec 21, 2022

Uh oh!

shivaprasadbhat commented Dec 22, 2022

Uh oh!

wlemkows commented Jan 4, 2023

Uh oh!

shivaprasadbhat commented Jan 8, 2023

Uh oh!

lukaszstolarczuk commented Jan 9, 2023

Uh oh!

shivaprasadbhat commented Jan 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

Uh oh!

shivaprasadbhat commented Jan 9, 2023

Uh oh!

lukaszstolarczuk commented Jan 9, 2023

Uh oh!

shivaprasadbhat commented Jan 11, 2023

Uh oh!

shivaprasadbhat commented Feb 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shivaprasadbhat commented Dec 21, 2022 •

edited

Loading

shivaprasadbhat commented Jan 9, 2023 •

edited

Loading