-
Notifications
You must be signed in to change notification settings - Fork 48
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
control/server.py: Enable DSA to offload and accelerate CRC calculation #704
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Submodule spdk
updated
8 files
+1 −2 | configure | |
+0 −41 | doc/jsonrpc.md | |
+2 −5 | include/spdk/nvmf.h | |
+25 −28 | lib/nvmf/nvmf_rpc.c | |
+1 −1 | lib/nvmf/spdk_nvmf.map | |
+7 −22 | lib/nvmf/subsystem.c | |
+3 −7 | python/spdk/rpc/nvmf.py | |
+9 −11 | scripts/rpc.py |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work on older CPU, please detect it and only is supported enable it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we build right now, it might not work on any CPU. To eliminate crashes on some machine we build the SPDK using x86-64-v2 architecture. This instructs the compiler not to use instructions which are not available on v2. We need to verify that the instructions used here are supported on v2 CPUs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CongMinYin @gbregman the question is what happens if one one hand we build to x86-64-v2, but on the other hand we specify this flag (--with-idxd). I assume that the SPDK build will ignore the --with-idxd because it is not supported on x86-64-v2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caroav this can easily verified. We can just disassemble the SPDK binary and see if this instruction was used or not. This is what I did when we had the issue with RDSEED.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caroav I think we shuold check
enable_dsa
inceph-nvmeof.conf
, then configure with--with-idxd
flag. But I don't how to write something like "if else" in.env
orDockerfile
. It seems like you're saying the x86-64-v2 instruction set. But in reality, DSA is a DMA engine, an accelerator only integrated into the new 4th Gen CPU now.idxd
is a kernel driver. I have tried to configureidxd
on server with AMD CPU, it will report an error and exit. So it's best to check theenable_dsa
option in theceph-nvmeof.conf
. Provide different parameters in .env
orDockerfile
. But I couldn't find a similar code block. Do you know what to do?Run configure in a server with AMD CPU:
SPDK configure code to detect CPU:
https://github.com/spdk/spdk/blob/dfb2950fd5b13ba0a2a2b533e3e534202a89de74/configure#L741
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the IDXD code itself is safe for all hardware platforms:
So building the code with --with-idxd will include a new assembly instruction, but it will only be executed on CPUs that support it because of the PCIe device check. Its very hard to prove this other than by code review.
The configure script for SPDK is only permitting the code to be compiled with --with-idxd if it is compiled on an Intel CPU. This means you can't use an AMD server as the build machine. The script is not checking the CPU version - so you can build the code with support for IDXD on an old Intel CPU.
I suggest that the check in the configure script is overly restrictive, and perhaps should be reduce to a warning rather than an error as the code may be built on a system with different capabilities to the system it is run on. It might also be better to check for the presence of the PCIe device rather than the CPU vendor to determine whether the build server supports idxd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, we need support for the code to be compiled for non-Intel CPU
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caroav Is our solution to try submitting a PR to modify the SPDK's config script, or check the
enable_dsa
parameter in thisceph-nvmeof
project?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @caroav @oritwas, I have submit a pr to solve the platforms errors: https://review.spdk.io/gerrit/c/spdk/spdk/+/24172. It merged. SPDK master branch and later versions won't met this platform errors.