[06:59:05] peluse: why bdev_crypto require static IPsec_MB libs? [06:59:20] is there some rease it can't use '-lISsec_MB'? [07:31:05] pwodkowx - you may already know this, but we have to exclude bdev_crypto from any of our packages - it will not work with a packaged DPDK [07:31:18] it requires a patch that is in our dpdk submodule [09:08:35] sethhowe: ping [09:12:57] lhodev: hi [09:14:11] Hey Seth. After checking out your latest shared libs, I ran objdump -t on the combined shared lib (libspdk.so). [09:14:36] I did that to look for symbols. I failed to see some symbols that I expected. [09:14:55] By comparison, when run on a build without the shared lib patches, the symbols *did* appear. [09:15:19] I did a "make V=1" so that I could compare the exact compilation flags building libspdk.so. [09:15:53] They appeared to be the same, so I'm bewildered why I seem to be missing symbols in the library in one version vs. the other. [09:16:32] Can you run "objdump -t libspdk.so | grep nvmf" on your build that has your patches? [09:18:27] That is odd. I did the same thing, comparing the make commands. I figured that would be enough to ensure they are the same. I will try the objdump thing now. [09:25:56] sethhowe: To sanity check, I just did another build with a branch I created/check-out on the commit just before the top two which were yours. When I build at that point, dumping the sumbols from libspdk.so *does* show the symbols. [09:26:12] lhodev: I just verified your findings. . . I am a little confused. I will have to look through the changes for a bit to wrap my head around what's going wrong [09:26:38] sethhowe: So, you're seeing the same thing? That is, missing symbols? [09:34:42] lhodev: yes, it doesn't show any of the nvmf stuff when I do objdump. [09:38:07] sethhowe: before your patches, the size of the libspdk.so.1 on my build is 6007232. With your patches applied, the size of the libspdk.so.1 for my build is much smaller: 1126824 [09:42:32] sethhowe: I did also note that the resulting compile line has "extraneous" --whole-archive directives; i.e. ones without a matching --no-whole-archive to close it. [09:43:10] However, even before your patches, the same thing existed so that's likely a red-herring with respect to what we're observing with the missing symbols. [09:44:32] Nonetheless, should address those unbalanced --whole-archive's. [09:51:00] lhodev: One thing that is definitely different between the two builds is that libspdk.so is the first shared object file we create on master, whereas it's the last one we create after applying my patches. Not sure what that means yet, but I am going to start looking down the order of operations route. [09:52:31] sethhowe: oh, oh. I think I might know what it is then. That's a vital clue. Let me check something quickly.... [09:53:47] sethhowe: Yup, I found the "issue", and I think we're ok after all. [09:55:48] On master -- i.e. before your patches -- the combined shared lib was built based on the static libs (and hence the value of the --whole-archive, directive). With your patches applied, the build of libspdk.so.1 results in *dependencies* to the independent shared libs instead of their contents. [09:57:09] darsto: are you still on by chance? [09:59:15] lhodev: just coming to the same conclusion. Running ldd on the shared lib on my patch reveals dependencies on all of the other spdk_ shared libraries while on master this is not the case. I assume that this would be desired behavior, we don't want to duplicate all of the symbols in the common so and the other so files right? [09:59:24] jimharris: y [09:59:48] was looking at your vhost multiple initiator patch series - and noticed you marked the first patch -1 [09:59:54] sethhowe: correct [10:00:00] are the rest of those patches ready for review or should I hold off? [10:00:19] sethhowe: One caveat, though: given that the libspdk.so now has dependencies on other spdk shared libs instead of incorporating their actual contents, I am left to wonder if we could possibly incur a situation where a symbol might not resolve at run-time leading to a segfault as we previously saw in that issue with nvmf_tgt. [10:00:36] they're ready for review. I only intend to do small cleanup in vhost-nvme [10:01:06] so should i ignore the first patch then but review the rest? [10:01:25] i'd say please review the first patch as well [10:02:57] lhodev: That's a good point. I know that we conditionally exclude some libraries from the spdk_lib_list variable, (e.g. vhost), but I noticed that we don't do that with, for example, the nvmf lib. [10:04:35] sethhowe: And nvmf is just an example that I specifically searched owing to issue #434 regarding nvmf_tgt. I need to review the root cause for that issue. [10:04:49] jimharris: the first patch in series is a one big refactor - it prepares all the function and structure names, shows the general outline of what direction the series is heading, but doesn't introduce any changes to vhost logic [10:05:14] I remember it involved libibverbs, but cannot recall off the top of my head if that was static or shared. [10:05:36] so while it is important, you might want to give it a just quick look [10:06:10] and then review the subsequent patches [10:06:30] ok - i'll take some time to give those patches some love [10:06:32] libibverbs is shared. The root cause there was that the nvmf_tgt binary was linking to libibverbs, but the spdk_nvmf.so did not have a linker dependency to libibverbs. [10:06:45] :) [10:06:52] hooray [10:10:42] sethhowe: Trying to recall why that still didn't resolve. Was it because libibverbs appeared before spdk_nvmf.so ? My instinct is that if libibverbs appeared after, then it should've resolved any missing syms that resulted in spdk_nvmf.so, but maybe that was part of the flawed thinking. [10:13:59] lhodev: As long as the main shared lib properly links to all of the top level spdk shared lib files (which can be confirmed by running ldd on the libspdk.so file), and those shared libs in turn link to the proper external .so files (which is what my patch set is aimed at fixing with the local_shared_lib variable) I don't think there is a way we can segfault on a symbol. [10:14:14] Perhaps as long as we have dependencies described in the libspdk.so.1.0 then all will get resolved? As an experiment, I'd like to see if I can hand-invoke a final link of nvmf_tgt, but instead of using the linker line currently employed during the build, use a modification where only the single libspdk.so.1.0 shared lib is specified. May have to add a couple other system libs too, but no independent spdk libs. [10:16:11] lhodev: I'm sorry, I don't follow your first comment. What do you mean by Trying to recall why that still didn't resolve? [10:19:40] lhodev: Oh wait, just got it. As far as I can tell, if there is a runtime linked symbol reference inside of a shared object, that dependency has to be resolved internally. For whatever reason, the runtime linker will not look back to the original calling executables dependencies to resolve references in the .so file. [10:19:58] sethhowe: Was trying to understand the case that if you had something like "CC -o foo foo.o -lshared1 -lshared2", then if shared1 had symbols that needed to be resolved by shared2, then shouldn't that get resolved since shared2 physically follows shared1 on the link line. [10:20:17] sethhowe: Right [10:24:37] sethhowe: I need to head out for lunch and let my dogs out at home. Will sync up with you again later. [10:25:16] lhodev: OK, thank you for pointing all of this stuff out. [10:27:10] exit [10:27:21] *wrong window [10:56:39] *** Quits: bwalker (bwalker@nat/intel/x-ltztyiskcxybmjyq) (ZNC - http://znc.in) [11:03:23] *** Joins: bwalker (~bwalker@134.134.139.72) [11:03:24] *** ChanServ sets mode: +o bwalker [11:03:24] *** Server sets mode: +cnrt [11:03:24] *** Server sets mode: +cnrt [11:12:16] *** Joins: jimharris (jimharris@nat/intel/x-xtfubgfdsttaelwd) [11:12:16] *** ChanServ sets mode: +o jimharris [11:33:38] *** Joins: travis-ci (~travis-ci@ec2-54-196-28-243.compute-1.amazonaws.com) [11:33:39] (spdk/master) test/common: parallelize vm_setup.sh (Seth Howell) [11:33:39] Diff URL: https://github.com/spdk/spdk/compare/cafd537c7d5f...a0a92be2dfe7 [11:33:39] *** Parts: travis-ci (~travis-ci@ec2-54-196-28-243.compute-1.amazonaws.com) () [11:34:38] *** Joins: travis-ci (~travis-ci@ec2-54-167-161-33.compute-1.amazonaws.com) [11:34:39] (spdk/master) crypto: Misc cleanup from final review (paul luse) [11:34:39] Diff URL: https://github.com/spdk/spdk/compare/a0a92be2dfe7...e42bb743f979 [11:34:39] *** Parts: travis-ci (~travis-ci@ec2-54-167-161-33.compute-1.amazonaws.com) () [11:38:13] *** Joins: travis-ci (~travis-ci@ec2-54-90-191-211.compute-1.amazonaws.com) [11:38:14] (spdk/master) env/dpdk: do not clean up DPDK 18.05 shared files (Darek Stojaczyk) [11:38:14] Diff URL: https://github.com/spdk/spdk/compare/e42bb743f979...036d92397246 [11:38:14] *** Parts: travis-ci (~travis-ci@ec2-54-90-191-211.compute-1.amazonaws.com) () [11:38:45] *** Joins: travis-ci (~travis-ci@ec2-54-205-2-234.compute-1.amazonaws.com) [11:38:46] (spdk/master) conf: add an example of defining RAID bdev (GangCao) [11:38:47] Diff URL: https://github.com/spdk/spdk/compare/036d92397246...2e2aba39275d [11:38:47] *** Parts: travis-ci (~travis-ci@ec2-54-205-2-234.compute-1.amazonaws.com) () [11:41:05] *** Joins: travis-ci (~travis-ci@ec2-54-211-227-170.compute-1.amazonaws.com) [11:41:06] (spdk/master) lib/nvme: add a check for valid namespace id (GangCao) [11:41:06] Diff URL: https://github.com/spdk/spdk/compare/2e2aba39275d...61e741efca85 [11:41:06] *** Parts: travis-ci (~travis-ci@ec2-54-211-227-170.compute-1.amazonaws.com) () [11:43:58] *** Joins: sethhowe (~sethhowe@134.134.139.72) [11:43:59] *** Joins: travis-ci (~travis-ci@ec2-54-205-2-234.compute-1.amazonaws.com) [11:44:00] (spdk/master) fio_plugin: add more information and fix a minor error for usage guide (Yanbo Zhou) [11:44:00] Diff URL: https://github.com/spdk/spdk/compare/61e741efca85...3bb7438e0509 [11:44:00] *** Parts: travis-ci (~travis-ci@ec2-54-205-2-234.compute-1.amazonaws.com) () [11:45:04] *** Joins: travis-ci (~travis-ci@ec2-54-81-189-81.compute-1.amazonaws.com) [11:45:05] (spdk/master) test/nvmf: No longer use construct_nvmf_subsystem method (ChenWang01) [11:45:05] Diff URL: https://github.com/spdk/spdk/compare/3bb7438e0509...4db5688e32e3 [11:45:05] *** Parts: travis-ci (~travis-ci@ec2-54-81-189-81.compute-1.amazonaws.com) () [11:47:20] *** Joins: travis-ci (~travis-ci@ec2-54-227-61-0.compute-1.amazonaws.com) [11:47:21] (spdk/master) test/vhost: reduce size of created raw disk file (Karol Latecki) [11:47:21] Diff URL: https://github.com/spdk/spdk/compare/369719c26bf3...0f25ee975021 [11:47:21] *** Parts: travis-ci (~travis-ci@ec2-54-227-61-0.compute-1.amazonaws.com) () [11:47:34] *** Joins: travis-ci (~travis-ci@ec2-54-90-191-211.compute-1.amazonaws.com) [11:47:35] (spdk/master) crypto: move PMD driver input validation to common function (paul luse) [11:47:35] Diff URL: https://github.com/spdk/spdk/compare/4db5688e32e3...369719c26bf3 [11:47:35] *** Parts: travis-ci (~travis-ci@ec2-54-90-191-211.compute-1.amazonaws.com) () [11:50:09] *** Joins: travis-ci (~travis-ci@ec2-54-81-189-81.compute-1.amazonaws.com) [11:50:10] (spdk/master) vhost: don't lock global vhost mutex when waiting for device start/stop (Darek Stojaczyk) [11:50:10] Diff URL: https://github.com/spdk/spdk/compare/0f25ee975021...f08a6eebd3dc [11:50:10] *** Parts: travis-ci (~travis-ci@ec2-54-81-189-81.compute-1.amazonaws.com) () [11:54:18] *** Joins: travis-ci (~travis-ci@ec2-54-159-156-233.compute-1.amazonaws.com) [11:54:19] (spdk/master) crypto: fix file permissions (paul luse) [11:54:19] Diff URL: https://github.com/spdk/spdk/compare/f08a6eebd3dc...18aeb14fe0c8 [11:54:19] *** Parts: travis-ci (~travis-ci@ec2-54-159-156-233.compute-1.amazonaws.com) () [13:18:28] sethhowe: ping [13:24:54] *** Joins: travis-ci (~travis-ci@ec2-54-159-117-127.compute-1.amazonaws.com) [13:24:55] (spdk/master) crypto: add error handling to claim function (paul luse) [13:24:55] Diff URL: https://github.com/spdk/spdk/compare/6de9698dec7d...b5c5d1450683 [13:24:55] *** Parts: travis-ci (~travis-ci@ec2-54-159-117-127.compute-1.amazonaws.com) () [13:25:28] *** Joins: travis-ci (~travis-ci@ec2-54-204-142-96.compute-1.amazonaws.com) [13:25:29] (spdk/master) crypto: Build QAT module when crypto is enabled (paul luse) [13:25:29] Diff URL: https://github.com/spdk/spdk/compare/b5c5d1450683...ff852667f84e [13:25:29] *** Parts: travis-ci (~travis-ci@ec2-54-204-142-96.compute-1.amazonaws.com) () [13:25:33] lhodev: could you look at https://review.gerrithub.io/#/c/spdk/spdk/+/426129/ again? [13:25:41] you gave this a -1 earlier [13:26:16] jimharris: Yeah, I went to add another comment which resulted in removal of the '-1'. I'm actively working on this very issue currently. [13:26:17] oh - i guess then you removed the -1 - anyways, if you wouldn't mind taking a look we can get both of these patches from seth merged [13:26:30] would you like me to hold off on merging then? [13:26:51] Yes, Jim, please hold off. [13:27:18] lhodev: ping [13:27:43] I'm trying to test the use of the combined libspdk.so and it's not working for me (yet). At this stage, dunno if it's something I'm doing wrong vs something in the lib itself. [13:28:27] What is the linker command you are passing to the nvmf_tgt application? [13:29:24] sethhowe: after I did a build, I then performed a "make DESTDIR=$HOME install" [13:29:51] sethowe: Then, I cd'd into the app/nvmf_tgt dir to try and manually build it as follows: [13:30:09] export LD_LIBRARY_PATH=$HOME/usr/local/lib [13:30:19] gcc -o nvmf_tgt_lance \ [13:30:29] -Wl,-z,relro,-z,now -Wl,-z,noexecstack \ [13:30:40] -fprofile-arcs -ftest-coverage -pthread nvmf_main.o \ [13:30:47] -L$HOME/usr/local/lib \ [13:30:56] -lspdk [13:31:08] sethhowe: This results in complaints: [13:31:41] ld: nvmf_main.o: undefined reference to symbol 'spdk_app_start' [13:31:57] $HOME/usr/local/lib/libspdk_event.so.1: error adding symbols: DSO missing from command line [13:31:57] collect2: error: ld returned 1 exit status [13:33:53] sethhowe: spdk_app_start() lives in libspdk_event.so.1, and a "ldd libspdk.so" does list libspdk_event.so.1 complete with the resolved path to my $HOME/usr/local/lib [13:35:48] sethhowe: Additional note: After doing the "make install" above, I also ran "ldconfig -v -n $HOME/usr/local/lib" so the symlinks were built correctly. [13:35:49] lhodev: I am looking up that DSO error right now to see what it means. [13:36:35] sethhowe: I've seen that DSO complaint before, and it's almost always been due to an ordering of the libraries on the link line. [13:39:15] sethhhowe: For grins, if I go ahead and append a '-lspdk_event', then it no longer complains with the DSO error, but instead I get a list of undefined references from libnspdk_rte_vhost.so.1 and libspdk_rte_vhost.so.1 [13:41:01] sethhowe: Also, FYI, my config is pointing to the DPDK submodule -- not a DPDK package install. Hence, the linking as far as the DPDK is concerned, I think, should be depending on *static* DPDK libs. [13:47:12] lhodev: I guess that it makes sense that since the spdk shared library doesn't include those symbols, then at compile time the linker doesn't think they exist anywhere. But I thought it would be able to resolve down through the dependencies. [13:52:07] sethhowe: So, I amended my compile line to specify the DPDK static libraries. I had mistakenly thought they were linked into libspdk_env_dpdk. [13:52:19] sethhowe: However, that still doesn't resolve everything. [13:53:29] I enclosed the DPDK static libs within a --start-group and --whole-archive as well. [13:54:11] Alas, that got me further, but I got a complaint about numa in spite of the fact that libnuma appears as a dependency in libspdk.so [13:54:20] So, I added a -lnuma as well. [13:54:50] question for both of you - do these issues only happen with seth's patches? or is this a problem on current master as well? [13:55:26] jimharris: I haven't yet tried this without Seth's patches. [13:55:52] jimharris: I have a branch I can quickly change to and do a build there. Hang tight. [13:56:04] not urgent - was just curious [14:00:39] jimharris: A build without Seth's patches followed by an attempt to link just against the resulting libspdk.so does *not* produce the linker complaints. Dunno if it runs yet, but it didn't complain at link time. [14:01:03] ok - well that's a good hint where to look i guess [14:01:32] i have a couple of ideas i'd like to get feedback on - regarding patch workflow for maintainers [14:02:07] i use a dashboard w/ custom search url that shows me the "next 20 patches" i should think about reviewing [14:02:35] this list will ignore: patches with a -1, patches that haven't passed the test pool, patches marked "work in progress" [14:03:01] there are a couple of other cases where i want to remove a patch from this list: [14:03:40] 1) i have a question on the patch - i want to remove the patch from the list until the submitter answers the question [14:03:53] 2) i'm waiting for a +1 from another reviewer [14:04:55] for #1, I could mark the patch -1, but that's not really ideal, and then i don't have an easy way to know when the question has been answered [14:06:12] so i'd like to use hashtags - for #1, I could add a "Question" hashtag on the patch - this would remove it from my queue, then when the submitter answers the question, they clear the hashtag and it pops back up in my queue [14:06:29] same for #2 - except hashtag "Waiting for +1" [14:09:46] sethhowe: Not only was I able to link using just libspdk.so to produce the app nvmf_tgt, I was able to start the app up without complaint. I haven't run any traffic, but no segfault or complaint thus far running. [14:10:07] just talking with sethhowe here in the lab - he brought up another great point in favor of the "Waiting for +1" hashtag - anyone could set up a search url for patches with that hashtag to know which patches the maintainers are specifically looking for help on [14:10:15] The above was against a build before your patches. [14:12:43] lhodev: Thanks for that. I think I might be onto something here. I am going to try to compile the nvmf_tgt app with the additional linker flag -rpath=$HOME/usr/local/lib. [14:13:23] sethhowe: I tried that rpath linker flag as well during my experiments. No go (for me). [14:13:49] lhodev: Darn. Back to the drawing board. [14:14:12] sethhowe: And, again, I didn't have to do that or specify any additional flags/args when I built against the libspdk.so before your patches, and the app not only linked but started up. [14:15:15] if you dump symbols from libspdk.so before and after this patch - do they look wildly different? [14:16:49] jimharris: Yes, but we assumed that was because they were all located in the other individual shared libraries and they would be linked to at run time. [14:18:31] jimharris: Definitely, because pre-Seth's patches, the libspdk.so was comprised of the static spdk libs. After the patches are applied, those symbols don't appear directly in the libspdk.so because they're expected to be resolved by dependencies to other shared spdk libs. [14:19:06] ah - so libspdk.so now contains no symbols - it just depends on all of the other spdk shared libraries? [14:20:09] jimharris: Not "no symbols" because the libspdk.so is still produced from *some* static libs. In the case of building against DPDK submodule, then it uses the static DPDK libs for rte_XXXX funcs/syms. [14:29:15] jimharris: I like the hashtag idea [14:30:45] jimharris: It gets a +1 from me too. #hashtags [15:00:26] ok - i think i'll write this up on the development page and then send out an e-mail to the mailing list referencing it [17:36:49] *** Joins: peluse (peluse@nat/intel/x-spaviogdwvvmbqoi) [17:36:49] *** ChanServ sets mode: +o peluse [17:37:05] just dusted this one off, please have a look... https://review.gerrithub.io/c/spdk/spdk/+/406824 [17:37:23] not sure about the condition for when to dump it out based on drv's comment in the previous patch [20:17:25] *** Joins: travis-ci (~travis-ci@ec2-54-166-161-14.compute-1.amazonaws.com) [20:17:26] (spdk/master) bdev: Avoid assert and factor out queue IO operation in _bdev_write_zero_buffer_next (Shuhei Matsumoto) [20:17:27] Diff URL: https://github.com/spdk/spdk/compare/ff852667f84e...6fa7e3866765 [20:17:27] *** Parts: travis-ci (~travis-ci@ec2-54-166-161-14.compute-1.amazonaws.com) ()