server icon indicating copy to clipboard operation
server copied to clipboard

Use aligned memory allocator in pfs-t tests

Open arkamar opened this issue 1 year ago • 10 comments

  • [ ] The Jira issue number for this PR is: MDEV-______

Description

(copy from commit message)

The pfs-t tests can segfault when compiled with the -mavx compiler flag. The segfault is caused by unaligned memory accesses incompatible with AVX instructions.

This patch corrects the alignment issue by adopting aligned memory allocation and deallocation mechanisms, inspired by the approach already in use within stub_pfs_global.h. By ensuring that all dynamically allocated memory in the Performance Schema tests is properly aligned to CPU_LEVEL1_DCACHE_LINESIZE, the patch eliminates the segmentation faults previously observed under compilation with the -mavx flag.

The bug was reported here: https://bugs.gentoo.org/895112 The fix is somewhat related to https://jira.mariadb.org/browse/MDEV-28091

How can this PR be tested?

Compile with -mavx flag and run pfs unittests.

Basing the PR against the correct MariaDB version

  • [ ] This is a new feature and the PR is based against the latest MariaDB development branch.
  • [ ] This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced.

Actually, the bug is in all maintained versions. I have chosen 10.6 because the patch applies cleanly to all more recent versions. But some modification might be needed for 10.4 and 10.5.

PR quality check

  • [x] I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • [x] For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

arkamar avatar Feb 01 '24 15:02 arkamar

The rebase to 10.5 is not straightforward because include/aligned.h was introduced between 10.5 and 10.6 in commit 379467311102 - MDEV-28836: Memory alignment cleanup. It would be necessary to transform the patch to something like this https://github.com/MariaDB/server/blob/78662ddadd5f297116ee1cf5708cbf3e19030152/storage/perfschema/unittest/stub_pfs_global.h#L52-L65 but then the merge back to 10.6 would require modification to currently presented patch. How should I proceed?

arkamar avatar Feb 02 '24 08:02 arkamar

Ack, thank you. I'll think about it, but tempted to keep like you've done and keep it as 10.6

grooverdan avatar Feb 02 '24 21:02 grooverdan

I can prepare the patch for 10.4 in separate PR. Then, it will be necessary to resolve conflict when 10.5 will be merged to 10.6.

I don't know if any other downstream is experiencing this issue. Gentoo users can easily change -m flags and it is common practice to use -march=native, which is why we spot the issue. Anyway, the lowest supported series in Gentoo is 10.6, if I should support your temptation somehow :)

arkamar avatar Feb 02 '24 22:02 arkamar

I imagine it'll start showing up on other distros as they start increasing their -march baseline for amd64. I think Fedora and openSUSE are both considering that (e.g. x86-64-v2/v3).

thesamesam avatar Feb 02 '24 22:02 thesamesam

10.4 is closed to the almost major fixes only with one release to go. I don't recall a current disto with 10.4 now. So a 10.5 hack is welcome, if you have time, and we'll resolve the conflict with this PR.

I hope to have Fedora updated beyond 10.5 before the f40 release.

I imagine it'll start showing up on other distros as they start increasing their -march baseline for amd64. I think Fedora and openSUSE are both considering that (e.g. x86-64-v2/v3).

Any suggestions on where to discover/follow these sort of decisions?

grooverdan avatar Feb 08 '24 04:02 grooverdan

I rebased the branch to the top of 10.6. I will create PR with backport to 10.5 soon.

arkamar avatar Feb 12 '24 16:02 arkamar

The backported fix is in PR #3057. The merge seems to be simple for this change, I followed these steps:

git checkout 10.5
# cherry pick the change from PR #3057
git cherry-pick pfs-t-fix-backport
git checkout 10.6
# cherry pick the change from PR #3038
git cherry-pick pfs-t-needs-aligned-malloc
git merge 10.5
# I don't know about other changes but we want to checkout --ours for pfs-t fix
git checkout --ours -- storage/perfschema/unittest/stub_print_error.h
git add -- storage/perfschema/unittest/stub_print_error.h
# The storage/perfschema/unittest/stub_print_error.h file is no longer in the status list as nothing has changed in 10.6 actually.
git merge --continue

arkamar avatar Feb 12 '24 16:02 arkamar

Can I kindly ask when I can expect any progress with this PR?

arkamar avatar Feb 26 '24 12:02 arkamar

Can I kindly ask when I can expect any progress with this PR?

Sorry, I missed this one. I'll work on getting it a second reviewer in a meeting I have next week.

LinuxJedi avatar May 02 '24 10:05 LinuxJedi

gentle ping

arkamar avatar Jul 03 '24 15:07 arkamar