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

Jump to bottom

simd: use alloc_pages_node to force alignment #9674

Merged
merged 1 commit into from Dec 10, 2019
Merged

simd: use alloc_pages_node to force alignment #9674

merged 1 commit into from Dec 10, 2019

Conversation

Fabian-Gruenbichler
Copy link
Contributor

Motivation and Context / Description

See #9608 - the SIMD/FPU code is currently broken because it relies on (wrong) assumptions about kernel memory allocation.

fxsave and xsave require the target address to be 16-/64-byte aligned.

kmalloc(_node) does not (yet) offer such fine-grained control over
alignement[0,1], even though it does "the right thing" most of the time
for power-of-2 sizes. unfortunately, alignment is completely off when
using certain debugging or hardening features/configs, such as KASAN,
slub_debug=Z or the not-yet-upstream SLAB_CANARY.

resort to (spl_)kmem_cache_alloc, which enables us to specify an
explicit alignment. CPU/NUMA-node locality is lost, since spl_kmem_cache
does not have kmem_cache_alloc_node support.

Fixes: #9608

0: https://lwn.net/Articles/787740/
1: https://lore.kernel.org/linux-block/20190826111627.7505-1-vbabka@suse.cz/

How Has This Been Tested?

smoke-tested in VMs with regular, hardened and slub_debug kernels

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

@Fabian-Gruenbichler
Copy link
Contributor Author

@bugcommenter @zachariasmaladroit maybe you could keep this a spin once another dev has taken a look?

@behlendorf maybe we do want to add a kmem_cache_alloc_node wrapper as well, and force the cache to use SLAB?

@Fabian-Gruenbichler
Copy link
Contributor Author

forgot to mention - I dumped the addresses, and the alignment was indeed off for KASAN/slub_debug=Z/SLAB_CANARY, and correct for regular kernel configs. with this patch, it was correct across the board - so I am fairly certain this is the culprit..

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Dec 4, 2019
@zachariasmaladroit
Copy link

@Fabian-Gruenbichler definitely would give it a test once it is deemed safe ! Thanks for the work and quick track-down of the cause

I'm running slub via

slub_nomerge slub_debug=FZP slab_common.usercopy_fallback=y

behlendorf
behlendorf requested changes Dec 4, 2019
View reviewed changes
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

This nicely explains things! I agree switching this to the kernel's slab allocator is going to be the best way to handle this. I also don't think we need to give up the numa locality if we directly use the kernel's slab allocator instead of the spl. Here's what I was thinking, 6ced01f, which tweaks your patch a little bit. Additional comments inline.

include/os/linux/kernel/linux/simd_x86.h Outdated Show resolved Hide resolved
include/os/linux/kernel/linux/simd_x86.h Outdated Show resolved Hide resolved
include/os/linux/kernel/linux/simd_x86.h Outdated Show resolved Hide resolved
include/os/linux/kernel/linux/simd_x86.h Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 5, 2019 •
edited

Codecov Report

Merging #9674 into master will increase coverage by <1%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #9674    +/-   ##
========================================
+ Coverage      79%      79%   +<1%     
========================================
  Files         418      418            
  Lines      123577   123562    -15     
========================================
+ Hits        97951    98048    +97     
+ Misses      25626    25514   -112
Flag Coverage Δ
#kernel 80% <ø> (ø) ⬆️
#user 67% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c502d3...e943c8e. Read the comment docs.

@Fabian-Gruenbichler
Copy link
Contributor Author

I took your v2 and just slightly reworded the commit message, and added '16-/' to the comment, hence left your authorship+final S-O-B. feel free to re-order the S-O-Bs/reset the author, if you think it is more appropriate the other way round. either is fine for me!

@Fabian-Gruenbichler Fabian-Gruenbichler changed the title [WIP] simd: use kmem_cache to force alignment simd: use kmem_cache to force alignment Dec 9, 2019
@Fabian-Gruenbichler
Copy link
Contributor Author

also, dropped [WIP] prefix in PR :)

@behlendorf behlendorf changed the title simd: use kmem_cache to force alignment simd: use alloc_pages_node to force alignment Dec 9, 2019
behlendorf
behlendorf approved these changes Dec 9, 2019
View reviewed changes
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Looks good, aside from the missing header. Thanks for updating it.

include/os/linux/kernel/linux/simd_x86.h Show resolved Hide resolved
@behlendorf @Fabian-Gruenbichler
simd: use alloc_pages_node to force alignment
e943c8e 
fxsave and xsave require the target address to be 16-/64-byte aligned.

kmalloc(_node) does not (yet) offer such fine-grained control over
alignement[0,1], even though it does "the right thing" most of the time
for power-of-2 sizes. unfortunately, alignment is completely off when
using certain debugging or hardening features/configs, such as KASAN,
slub_debug=Z or the not-yet-upstream SLAB_CANARY.

Use alloc_pages_node() instead which allows us to allocate page-aligned
memory. Since fpregs_state is padded to a full page anyway, and this
code is only relevant for x86 which has 4k pages, this approach should
not allocate any unnecessary memory but still guarantuee the neeeded
alignment.

Fixes: openzfs#9608

0: https://lwn.net/Articles/787740/
1: https://lore.kernel.org/linux-block/20190826111627.7505-1-vbabka@suse.cz/

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>Foo
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 10, 2019
@behlendorf behlendorf merged commit b119e2c into openzfs:master Dec 10, 2019
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Dec 10, 2019
@Fabian-Gruenbichler @behlendorf
SIMD: Use alloc_pages_node to force alignment
942f7d7 
fxsave and xsave require the target address to be 16-/64-byte aligned.

kmalloc(_node) does not (yet) offer such fine-grained control over
alignment[0,1], even though it does "the right thing" most of the time
for power-of-2 sizes. unfortunately, alignment is completely off when
using certain debugging or hardening features/configs, such as KASAN,
slub_debug=Z or the not-yet-upstream SLAB_CANARY.

Use alloc_pages_node() instead which allows us to allocate page-aligned
memory. Since fpregs_state is padded to a full page anyway, and this
code is only relevant for x86 which has 4k pages, this approach should
not allocate any unnecessary memory but still guarantee the needed
alignment.

0: https://lwn.net/Articles/787740/
1: https://lore.kernel.org/linux-block/20190826111627.7505-1-vbabka@suse.cz/

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#9608
Closes openzfs#9674
gamanakis pushed a commit to gamanakis/zfs that referenced this pull request Dec 26, 2019
@Fabian-Gruenbichler @gamanakis
SIMD: Use alloc_pages_node to force alignment
95bde46 
fxsave and xsave require the target address to be 16-/64-byte aligned.

kmalloc(_node) does not (yet) offer such fine-grained control over
alignment[0,1], even though it does "the right thing" most of the time
for power-of-2 sizes. unfortunately, alignment is completely off when
using certain debugging or hardening features/configs, such as KASAN,
slub_debug=Z or the not-yet-upstream SLAB_CANARY.

Use alloc_pages_node() instead which allows us to allocate page-aligned
memory. Since fpregs_state is padded to a full page anyway, and this
code is only relevant for x86 which has 4k pages, this approach should
not allocate any unnecessary memory but still guarantee the needed
alignment.

0: https://lwn.net/Articles/787740/
1: https://lore.kernel.org/linux-block/20190826111627.7505-1-vbabka@suse.cz/

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#9608
Closes openzfs#9674
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 26, 2019
@Fabian-Gruenbichler @tonyhutter
SIMD: Use alloc_pages_node to force alignment
91d6a71 
fxsave and xsave require the target address to be 16-/64-byte aligned.

kmalloc(_node) does not (yet) offer such fine-grained control over
alignment[0,1], even though it does "the right thing" most of the time
for power-of-2 sizes. unfortunately, alignment is completely off when
using certain debugging or hardening features/configs, such as KASAN,
slub_debug=Z or the not-yet-upstream SLAB_CANARY.

Use alloc_pages_node() instead which allows us to allocate page-aligned
memory. Since fpregs_state is padded to a full page anyway, and this
code is only relevant for x86 which has 4k pages, this approach should
not allocate any unnecessary memory but still guarantee the needed
alignment.

0: https://lwn.net/Articles/787740/
1: https://lore.kernel.org/linux-block/20190826111627.7505-1-vbabka@suse.cz/

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#9608
Closes openzfs#9674
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 27, 2019
@Fabian-Gruenbichler @tonyhutter
SIMD: Use alloc_pages_node to force alignment
be71dd1 
fxsave and xsave require the target address to be 16-/64-byte aligned.

kmalloc(_node) does not (yet) offer such fine-grained control over
alignment[0,1], even though it does "the right thing" most of the time
for power-of-2 sizes. unfortunately, alignment is completely off when
using certain debugging or hardening features/configs, such as KASAN,
slub_debug=Z or the not-yet-upstream SLAB_CANARY.

Use alloc_pages_node() instead which allows us to allocate page-aligned
memory. Since fpregs_state is padded to a full page anyway, and this
code is only relevant for x86 which has 4k pages, this approach should
not allocate any unnecessary memory but still guarantee the needed
alignment.

0: https://lwn.net/Articles/787740/
1: https://lore.kernel.org/linux-block/20190826111627.7505-1-vbabka@suse.cz/

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#9608
Closes openzfs#9674
tonyhutter pushed a commit that referenced this pull request Jan 23, 2020
@Fabian-Gruenbichler @tonyhutter
SIMD: Use alloc_pages_node to force alignment
35155c0 
fxsave and xsave require the target address to be 16-/64-byte aligned.

kmalloc(_node) does not (yet) offer such fine-grained control over
alignment[0,1], even though it does "the right thing" most of the time
for power-of-2 sizes. unfortunately, alignment is completely off when
using certain debugging or hardening features/configs, such as KASAN,
slub_debug=Z or the not-yet-upstream SLAB_CANARY.

Use alloc_pages_node() instead which allows us to allocate page-aligned
memory. Since fpregs_state is padded to a full page anyway, and this
code is only relevant for x86 which has 4k pages, this approach should
not allocate any unnecessary memory but still guarantee the needed
alignment.

0: https://lwn.net/Articles/787740/
1: https://lore.kernel.org/linux-block/20190826111627.7505-1-vbabka@suse.cz/

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes #9608
Closes #9674
ahrens pushed a commit to ahrens/zfs that referenced this pull request Mar 20, 2020
@Fabian-Gruenbichler @ahrens
SIMD: Use alloc_pages_node to force alignment
b2a09cb 
fxsave and xsave require the target address to be 16-/64-byte aligned.

kmalloc(_node) does not (yet) offer such fine-grained control over
alignment[0,1], even though it does "the right thing" most of the time
for power-of-2 sizes. unfortunately, alignment is completely off when
using certain debugging or hardening features/configs, such as KASAN,
slub_debug=Z or the not-yet-upstream SLAB_CANARY.

Use alloc_pages_node() instead which allows us to allocate page-aligned
memory. Since fpregs_state is padded to a full page anyway, and this
code is only relevant for x86 which has 4k pages, this approach should
not allocate any unnecessary memory but still guarantee the needed
alignment.

0: https://lwn.net/Articles/787740/
1: https://lore.kernel.org/linux-block/20190826111627.7505-1-vbabka@suse.cz/

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#9608 
Closes openzfs#9674
ahrens pushed a commit to ahrens/zfs that referenced this pull request Mar 23, 2020
@Fabian-Gruenbichler @ahrens
DLPX-69007 [Backport of DLPX-69006 to 6.0.2.0] SIMD: Use alloc_pages_…
c58392c 
…node to force alignment

fxsave and xsave require the target address to be 16-/64-byte aligned.

kmalloc(_node) does not (yet) offer such fine-grained control over
alignment[0,1], even though it does "the right thing" most of the time
for power-of-2 sizes. unfortunately, alignment is completely off when
using certain debugging or hardening features/configs, such as KASAN,
slub_debug=Z or the not-yet-upstream SLAB_CANARY.

Use alloc_pages_node() instead which allows us to allocate page-aligned
memory. Since fpregs_state is padded to a full page anyway, and this
code is only relevant for x86 which has 4k pages, this approach should
not allocate any unnecessary memory but still guarantee the needed
alignment.

0: https://lwn.net/Articles/787740/
1: https://lore.kernel.org/linux-block/20190826111627.7505-1-vbabka@suse.cz/

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#9608
Closes openzfs#9674
allanjude pushed a commit to KlaraSystems/zfs that referenced this pull request Apr 28, 2020
@Fabian-Gruenbichler
SIMD: Use alloc_pages_node to force alignment
b8d1abf 
fxsave and xsave require the target address to be 16-/64-byte aligned.

kmalloc(_node) does not (yet) offer such fine-grained control over
alignment[0,1], even though it does "the right thing" most of the time
for power-of-2 sizes. unfortunately, alignment is completely off when
using certain debugging or hardening features/configs, such as KASAN,
slub_debug=Z or the not-yet-upstream SLAB_CANARY.

Use alloc_pages_node() instead which allows us to allocate page-aligned
memory. Since fpregs_state is padded to a full page anyway, and this
code is only relevant for x86 which has 4k pages, this approach should
not allocate any unnecessary memory but still guarantee the needed
alignment.

0: https://lwn.net/Articles/787740/
1: https://lore.kernel.org/linux-block/20190826111627.7505-1-vbabka@suse.cz/

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#9608
Closes openzfs#9674
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behlendorf behlendorf behlendorf approved these changes

@tonyhutter tonyhutter tonyhutter approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[5.3.11] WARNING: CPU: include/os/linux/kernel/linux/simd_x86.h:204 zvol_fini+0x2463e/0x5ef0b [zfs]
4 participants
@Fabian-Gruenbichler @zachariasmaladroit @behlendorf @tonyhutter