-
Notifications
You must be signed in to change notification settings - Fork 3
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
Upgrading SPDK to 18.10 and rust to nightly-2018-11-21 #6
Comments
Hey @jkryl, Nice work! I haven't touched the code in a while due to other committments, but hoping to get back to it in the new year. Good investigative work! I have run into the same issue before with librte_mempool_ring missing, but I think at that point using the version of spdk that produced a static library solved it for me. My suggestion (if you want to get yourself ublocked) is to just use in-memory backend for all this (the way it's all setup in the tests here), but I might be wrong? The spdk_nvmf_subsystem_state_change kinda suggests to me you're trying to run on actual hardware, so maybe using the in-memory backend would be ok? Anyway, I don't have a good suggestion off the top of my head right now, in the past I was using cargo config to pass custom linker flags, but it was really hacky (example is here: https://github.com/jkozlowski/starfish/blob/083afb35e5ec870af3d490a0a9b53760947223bf/spdk-sys/.cargo/config) so I'd rather not go back to that. Out of curiosity what's your motivation? The library is really unfinished, so I wouldn't get your hopes up, but I'm hacking on it whenever time permits. If you're interested in contributing, would be great to collaborate. One thing I haven't bothered figuring out is what happens when futures are cancelled (there was an issue on https://github.com/rust-lang-nursery/wg-net about this but I can't find it): basically I'm currently using references to pass buffers to the library but those might dissapear, because futures are polling and spdk is callback/completion based. Anyway, lemme know what you think and please push a PR if you manage to solve this before me! |
Just using the memory backend won't solve the problem I think. These subsystem modules are loaded before main (i.e using the attribute flags) The only way to disable the nvmf target pieces then is simply to not link them in. As for the rustc flags, I've actually tried that and works. However, when running a release build, they still get lost :( |
Hmm, then I'm not sure what's happening. As you can see tests compile and run on CircleCI and I used the same exact setup on my Ubuntu box to run them and I'm not getting those failures :(
Yeah, that's why I'd rather not go back to them, they were so hit and miss :( Another approach I tried in the past was writing a custom Makefile that pulls in the spdk Makefile, requests all the libs and builds the libraries. Getting a single statically linked lib was so sweet, why did they pull the plug on that? In any case, lemme know how it goes, it might just force me to sacrifice my other commitments and play with this myself... |
Hmm, I'm wondering if this is actually worth reporting to spdk folks: Just running the example for the shared libraries doesn't seem to work for me.
|
As a data point, I have followed your instructions and managed to run the tests. I would actually be willing to take a PR with your changes anyway: it's already pretty hard to run these apps that I have a script to run tests locally anyway. So send a PR and add this file https://github.com/jkozlowski/starfish/pull/7/files#diff-40703290474a4270c464349966a9dc2c with suitable LD_* stuff and I'll happily merge. Keeping up with spdk is more important for me now than a bit of a hacky setup; and in the meantime I might raise an issue on the spdk repo (or feel free to do it yourself, I'm going offline in a sec). Also, hi @gila, I originally didn't realise there was yet another person on this issue! |
Nevermind, just forgot sudo... |
@jkozlowski thanks for your prompt response! I submitted a PR with the changes. In a long term, using static libraries would be probably even more useful. But yea, good to be able to run latest spdk version. To answer your question regarding motivation, I just saw the library and thought it was interesting so I decided to give it a try and learn from it. I understand it is not complete and that's fine. I see a great value in combining SPDK with rust async/await feature. Thanks for pioneering this area! :-) |
I would also prefer a statically linked spdk! Any idea how we can get that? I really don't understand (I mean I understand , but I don't buy that it's a problem in general) why you wouldn't want that! |
I know right! I would love to get a bit more time to work on this, the potential for something of the seastar architecture with the expressivity of rust is rather great! Throw in some quic networking and some nice RPC library on top of that and things could get pretty interesting still. |
absolutely! 👍 |
I run
I guess SPDK 18.10 is the way to go? The print out from the program:
Since the repo is in the middle of change to support SPDK 18.10, I'm also curious with the pull request #8 Will the test case pass @jkryl ? Thanks! |
Hi @xxks-kkk! Most likely this is the same problem as I had. The upgrade of spdk would most likely help. The PR#8 you mentioned cannot be used in its current form because it has a problem with linking SPDK libraries. We are considering multiple options of how to fix it but none of them is easy. I don't dare to estimate how and when this could be fixed. |
I have some new interesting ideas around this. @jkozlowski if you are interested to review them please do. thanks! spdk-sys crateProblem statementCreating a crate with bindings for spdk lib is challenging:
The purpose of this doc is to suggest a solution for the last problem mentioned above. As for the first three problems we assume that:
Though there are ways how to address first two problems in rust using “features”, it would require more work for the initial implementation and there is also maintenance burden as spdk libraries and headers get removed or added between SPDK releases. That said it would be a good extension of spdk sys crate later when it gets more mature. GoalsWe wish to have a spdk-sys crate which:
Best practicesBest practices follow from articles listed in the links section and exemplar rust sys crates (libgit2-sys).
Switching between different options is done by using rust “features” or environment variables. The default should be set to the most popular way of using the library which depends on the platform and the library itself. Detecting if the lib is installed on a system is done almost exclusively using pkg-config. Reality of SPDKSPDK can be built static or dynamic. It is a set of following libraries: SPDK:
DPDK:
ISA-L:
There is no monolithic libspdk.so and libdpdk.so containing all libraries above. When building SPDK with Applying the best practices to SPDKThe most preferred solution would be to build SPDK object archives and link them statically to app. We bang a head against a wall when doing so. Object archives are linked with as-needed or no-whole-archive linker flag in rust and all libs which are not explicitly used by the app are omitted. Since there is no way how to change the default linker behaviour in rust, the only viable Shared libraries suffer from the same problem as static libs as rust build system uses
The result is a suboptimal solution because the shared library must be built and installed to the system prior to building the app and deployed to a target system along with the app. Detecting if libspdk fat lib is installed on the system is cumbersome as we can’t use pkg-config. SPDK in general is missing support for pkg-config ( https://trello.com/c/uBM2PR4c/19-generate-pkg-config-pc-file-during-make-install ). In spite of all drawbacks it is kinda standard rust solution for sys crates and works with upstream spdk with no changes required. Using spdk sys crateHigh level steps of how to use spdk sys crate: Phase 1:
Phase 2:
The way how the fat shlib is deployed along with the app to a target system is out of scope. In case of a docker image it can be copied from the system where the image is built to a docker image. On other systems it may be delivered in form of a package as it is usually done for other system libraries. SPDK with patchesSome projects using SPDK in rust will need to use their own version of SPDK which differs from the upstream. There is probably no better way than to clone spdk-sys repository on github and override spdk git submodule in the repository so that it points to their own version of SPDK. In dependencies section of Cargo.toml file must be used a github URL of the cloned spdk-sys repo. TestsThe sys crate should come with tests. It is out of scope to test each function provided by SPDK. At minimum calling a single function from spdk would be sufficient. Later when support for opt-in modules is added, there should be a similar test for each module. Links
|
I like the fat shared library approach. I upgraded the startfish-executor project to latest nightly, I will try to do the same for the spdk-sys package. Then split out the async wrappers into a separate project, and try your suggestion. Can we not build a fat static library? (my knowledge in this domain is not the greatest) |
Hi Jakub, it would be great if we could give it a try. Here is the repository with spdk-sys: https://github.com/jkryl/spdk-sys . Note that it is still very fresh so expect problems when using it 😛 Possible way how to use it in another project is to include it as a submodule, because the fat library needs to be built and installed to the system before running the cargo build so it makes sense for it to be there and use it twice (first time to build and install fat lib and second time to build bindings from headers and serve as a proper sys crate). I'm doing the same work for our private project just now, I suppose it should not be that hard to do the same for starfish for me. I will try to submit a PR .. |
Lovely, will have a look. I suppose we could publish spdk-sys as a separate crate: I planned to do that once it was operational, but it sounds like you fixed things now.
Happy to iterate in your repo.
Jakub
…Sent from my iPhone
On 7 Aug 2019, at 16:07, Jan Kryl ***@***.***> wrote:
Hi Jakub, it would be great if we could give it a try. Here is the repository with spdk-sys: https://github.com/jkryl/spdk-sys . Note that it is still very fresh so expect problems when using it 😛 Possible way how to use it in another project is to include it as a submodule, because the fat library needs to be built and installed to the system before running the cargo build so it makes sense for it to be there and use it twice (first time to build and install fat lib and second time to build bindings from headers and serve as a proper sys crate). I'm doing the same work for our private project just now, I suppose it should not be that hard to do the same for starfish for me. I will try to submit a PR ..
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
@jkozlowski : it would be cool if we could chat on spdk's slack (spdk-team.slack.com) and discuss spdk-sys crate in greater detail there. cheers |
Happy to, but you'd need to invite me, says it's invite only. |
Is it possible for me to join the conversation as well? |
I have created a "rust" channel on spdk's slack. Anyone is welcomed to join. Link to spdk's slack is at https://spdk.io/community/. @jkozlowski : I have sent an invite to your email address that I got from linked in. I hope that will work. |
I decided to try starfish - just building it and running the tests. I tripped over a couple of problems when doing so and will describe each of them in detail. In the end I had to upgrade rust and SPDK versions to make it to work, but here is the longer story to it.
The first problem I encountered was the same as reported in ticket #5:
232 | #[allow(clippy::cast_ptr_alignment)]
I think this problem appears only for certain combination of rust tool chain and feature crate version. Upgrade of the rust toolchain to more recent version solved the problem.
Another problem was a segfault in SPDK when running the test. I hit double linked list corruption issue in spdk_nvmf_subsystem_state_change(). The stack is as follows (shortened version):
I decided to upgrade to latest SPDK released version (v18.10) as there were quite a few bug fixes in this area. And indeed, the newer SPDK version does not crash, however it brings a lot of other problems when building it. The way how shared libraries are built is different.
libspdk.so
is no longer produced as a by-product of building static library archives. Instead a new configure option--with-shared
must be used to build the shared libs. That builds a set of shared libs and a linker scriptlibspdk.so
which combines all of those libs together. For the record I'm attaching the contents of the linker script:Due to "AS_NEEDED" only the libs which are actually used by the application are linked to the executable. So far so good, but there is one more problem. DPDK libs are missing. They are not build nor installed by default. So I had to explicitly call
make CONFIG_RTE_BUILD_SHARED_LIB=y
andmake install
in dpdk subdirectory of spdk. One more thing is that since dpdk is optional part of spdk, one has to modify build.rs script in spdk-sys to include following:With these changes we have a test binary which does not fail in dynamic linker stage when executed. But we are still not done:
Using debugger I found out that the root cause of the problem is missing
librte_mempool_ring.so.1.1
. How is it possible? When building the test binary, rustc records to dependencies only those shared libraries which are actually referenced from the main program or libraries which it depends on. This library is not directly referenced but has this important piece of code which is executed in library constructor (when the lib is loaded) (in drivers/mempool/ring/rte_mempool_ring.c):This problem is normally easy to fix. Linker can be instructed to always include the library in dependencies even if not used by using
--no-as-needed
linker flag. The default in rustc is the opposite and I haven't found a way how to change this in spite of trying many things. So a dirty workaround for this last problem is to manually preload the library:I have a patch which is dirty and incomplete because of a missing solution for this last problem, but might solve as an inspiration for people having the same problem: jkryl@c8f6c7d
The text was updated successfully, but these errors were encountered: