[00:48:53] *** Joins: tkulasek (~tkulasek@134.134.139.83) [01:20:04] can someone take at https://review.gerrithub.io/#/c/spdk/spdk/+/419682/? [03:39:09] *** Quits: dlw (~Thunderbi@114.255.44.143) (Ping timeout: 260 seconds) [04:54:56] *** Quits: tomzawadzki (~tomzawadz@134.134.139.74) (Ping timeout: 265 seconds) [04:55:50] *** Joins: tomzawadzki (~tomzawadz@192.55.54.39) [05:41:45] *** Joins: lyan (~lyan@2605:a000:160e:2dd:4a4d:7eff:fef2:eea3) [05:42:08] *** lyan is now known as Guest68718 [07:22:52] *** Joins: philipp-sk (~Philipp@ktnron0916w-lp130-02-76-66-162-159.dsl.bell.ca) [07:35:56] *** Joins: dlw (~Thunderbi@123.122.68.112) [07:38:42] pwodkowx, just added a question [07:42:59] bwalker, two more dependencies for crypto to build in CI. First is a UT tweak that I will make "go away" after your simplification goes in, it's pretty harmless at https://review.gerrithub.io/c/spdk/spdk/+/418549 [07:43:30] second is a one line in DKDP config at https://review.gerrithub.io/c/spdk/dpdk/+/419921 [07:43:52] and then I'll update the submodule and the whole thing should build in CI. Where's the emoji for "fingers crossed" :) [07:47:56] 🤞 [07:47:59] this one? [07:48:10] xD [07:49:28] *** Quits: dlw (~Thunderbi@123.122.68.112) (Ping timeout: 256 seconds) [07:51:38] *** Joins: dlw (~Thunderbi@123.122.68.112) [08:12:57] *** Quits: tomzawadzki (~tomzawadz@192.55.54.39) (Ping timeout: 240 seconds) [08:19:21] *** Joins: peter_turschm (~peter_tur@64.20.177.245) [08:46:01] peluse: I reviewed the crypto patch [08:46:14] I know the spdk_*malloc doxygen says the returned buffer is physically contiguous, but that's going to change soon [08:46:43] darsto, I'm about to upload a complete refactor of the _crypto_operation function, I think I mentioned that was WIP. Thanks though, it's a bit simpler now and covers more complex IOV scenarios [08:48:21] I just wanted to check it out :) [08:50:03] are you splitting the encrypt/decrypt path, by any chance? [08:50:27] it was hard for me to follow which orig_io is which [08:53:47] darsto, no, they're almost identical so that would be a lot of duplicate code. I hope the new stuff is easier to follow. Need to make a few test tweaks and then I'll push. Also added new tests cases that are easy to expand for various IOV situations [08:55:56] darsto, actually just pushed in now, will enable testing in autotest in another update. [09:05:46] darsto, good comments, thanks!! [09:26:27] *** Quits: peter_turschm (~peter_tur@64.20.177.245) (Remote host closed the connection) [09:48:27] peluse: for the first review you linked in the UT, is that ASAN error in one of the nvme unit tests? [09:49:25] which one exactly? (not sure what first review you linked in UT means) [09:49:41] https://review.gerrithub.io/c/spdk/spdk/+/418549 [09:50:59] Ah, I'd have to go re-run it real quick but can no problem if you want. Pretty sure that's my crypto UT. [09:55:57] darsto, see my reply on https://review.gerrithub.io/c/spdk/dpdk/+/419921 Let me know if you have questions, this is a critical one for crypto as the patch won't build in CI until this (and a few other small ones) are in [09:57:11] bwalker, do you want me to spend some more time on that one (the mockable put)? I can probably work it out of my UT and not need it but it's in the style of the others now and I figure you've got a refactor of all this stuff coming up anyways. I'm willing to get rid of it though, let me know [09:57:36] in my patch to simplify the mocks I eliminate the one other place where a free call is mocked [09:57:46] it only occurs in one place on master today [09:58:04] K, want me to kill it then? [09:58:36] (or at least try) [09:59:06] does the test malloc the buffer that gets passed to spdk_dma_free? [09:59:14] and also, does the test itself free it? [09:59:19] is that the asan complaint? [09:59:52] it does not, that's the issue. but I should be able to change that and avoid the patch. [10:00:21] if you're calling a function that calls spdk_dma_free, I think it's good to actually free the buffer [10:00:31] because that catches bugs where the function accesses the buffer after it has been freed [10:00:47] OK, let me spend a few minutes on it and I'll get back to ya [10:01:33] *** Quits: dlw (~Thunderbi@123.122.68.112) (Ping timeout: 264 seconds) [10:03:39] peluse: i meant that you should uncomment CONFIG_RTE_CRYPTO_MAX_DEVS in common_spdk as well [10:04:16] darsto, we don't need to if its uncommented in common_base though [10:04:21] if you configure DPDK, you can use either "make config T=spdk-commonapp-xxx" or "make config T=x86_64-xxx" [10:04:31] does common_spdk include common_base? [10:04:38] OK, why do you keep saying that? :) [10:04:59] oh, to select the config file? [10:05:49] wrt common_spdk and common_base I don't know the link and drv wasn't sure either but simply by trial and error it was clear that if something is defined in base we don't need to do it in common_spdk [10:06:56] well, if it works it's fine to me [10:07:15] darsto, LOL, that's what we were thinking too [10:08:19] yeah, common_spdk includes common_base indirectly [10:09:17] bwalker, OK, so here's the deal. I define a stub for spdk_mempool_get_bulk() so that I can test and error path. In that error path the functional code calls spdk_mempool_put() so I need it to do nothing or get an ASAN. Do you have another suggestion for testing that error path (or any alloc/get) that doesn't also require us to mock the free/put? [10:10:15] if the alloc/get fails, the code shouldn't be calling free [10:10:35] I assume you mocked spdk_mempool_get_bulk to return NULL? [10:11:00] well yeah that makes sense :) Let me go look at the code and see what stupid thing I might have done [10:11:56] your unit test caught a bug [10:16:47] well, actually not [10:16:51] here's the snippet: [10:17:07] " │510 /* Allocate crypto operations. */ │ [10:17:07] │511 rc = rte_crypto_op_bulk_alloc(g_crypto_op_mp, │ [10:17:07] │512 RTE_CRYPTO_OP_TYPE_SYMMETRIC, │ [10:17:07] │513 crypto_ch->crypto_ops, cryop_cnt); │ [10:17:10] │514 if (rc < cryop_cnt) { │ [10:17:15] >│515 spdk_mempool_put_bulk(g_mbuf_mp, (void **)&crypto_ch->mbufs[0], │ [10:17:18] │516 cryop_cnt);" [10:17:55] yuck, that pasted ugly. Anyway, the test here is to mock the op_bulk call and then I free another buffer after that. But actually I don't think I need that get mocked anymore so ignore me again for a minute... :) [10:18:00] ok - what does rte_crypto_op_bulk_alloc do? [10:18:47] is there an rte_crypto_op_bulk_put? [10:21:48] Is the LD_PRELOAD fio parameter something that we can export to the env variables before running an fio job or does it need to be called out at each fio execution to load the engine [10:22:10] Someone shared a test script with me [10:22:12] they are doing [10:22:16] if you export it, it will LD_PRELOAD the fio plugin any time you run any program [10:22:27] including things like ls, cd, etc. [10:22:27] LD_PRELOAD=/root/spdk/examples/nvme/fio_plugin/fio_plugin [10:22:27] export LD_PRELOAD [10:22:42] so within a script, it is slightly different [10:22:48] and then fio [10:22:49] it only affects things within the script [10:22:59] no, that's C code copied from gdb [10:23:37] after my question about rte_crypto_op_bulk_put, everythingI said is addressed to jkkariu [10:24:14] here's the problem though. the code calls spdk_mempool_get_bulk() earlier than in the error path that I'm testing and that func doesn't exist in test_env.c so I mocked it to do nothing with a stub. So, when I test the later error path I try to free something that came from that mock. So... [10:24:40] you can either merge the patch as is or I can add a spdk_mempool_get_bulk() to test_env.c that really allocs [10:25:12] well.... calls the existing spdk_mempool_get() thats in test_env.c that is [10:25:16] but when I run their script I get the following error symbol lookup error: /home/john/spdk/examples/nvme/fio_plugin/fio_plugin: undefined symbol: register_ioengine [10:25:32] bwalker, LOL, OK [10:25:55] but this error goes away when I do LD_PRELOAD=... fio [10:26:42] I'm gonna guess you'd prefer not mocking the put and instead adding a get_bulk to test_env.c that looks just like what we have there for spdk_mempool_put_bulk(). Yes? [10:27:51] without seeing the test in its entirety, I suspect I would. The reason being that if the code under test calls spdk_mempool_put_bulk, you probably really do want to test that it doesn't access a buffer after it has been freed. [10:28:08] and the easiest way to do that is to make it really free a buffer and use a tool like asan during the unit tests [10:29:30] They are running the date command before running fio. So if I understand your input ben I think the error is thrown by the date. Which make sense because the fio job seems to have run just fine [10:29:41] most likely, yeah [10:30:03] I will actually comment out the date and rerun and see if the error goes away [10:30:22] *** Joins: johnmeneghini (~johnmeneg@pool-100-0-53-181.bstnma.fios.verizon.net) [10:31:49] bwalker, I'll make it so :) After I go lift a bunch of heavy shit and put it back down several times [10:32:36] that makes one of us [10:32:55] unless you count trash bags full of diapers [10:35:05] ick [10:38:59] *** Joins: travis-ci (~travis-ci@ec2-54-234-3-120.compute-1.amazonaws.com) [10:39:00] (spdk/master) test/iscsi_tgt: Delete temporary files when trap catches error (Shuhei Matsumoto) [10:39:00] Diff URL: https://github.com/spdk/spdk/compare/e3a71385716a...051297114cb3 [10:39:00] *** Parts: travis-ci (~travis-ci@ec2-54-234-3-120.compute-1.amazonaws.com) () [10:57:51] *** Quits: lhodev (~lhodev@66-90-218-190.dyn.grandenetworks.net) (Ping timeout: 256 seconds) [11:01:59] *** Joins: lhodev (~lhodev@66-90-218-190.dyn.grandenetworks.net) [11:16:03] *** Joins: peter_turschm (~peter_tur@64.20.177.254) [12:24:04] *** Quits: tkulasek (~tkulasek@134.134.139.83) (Ping timeout: 244 seconds) [14:03:18] *** Quits: peter_turschm (~peter_tur@64.20.177.254) (Remote host closed the connection) [14:03:24] *** Joins: peter_turschm (~peter_tur@64.20.177.254) [14:04:03] *** Quits: peter_turschm (~peter_tur@64.20.177.254) (Remote host closed the connection) [15:35:41] *** Quits: Guest68718 (~lyan@2605:a000:160e:2dd:4a4d:7eff:fef2:eea3) (Remote host closed the connection) [16:21:13] *** Joins: dlw (~Thunderbi@123.122.68.112) [17:45:12] *** Joins: lyan (~lyan@2605:a000:160e:2dd:4a4d:7eff:fef2:eea3) [17:45:35] *** lyan is now known as Guest67547 [17:51:27] *** Quits: dlw (~Thunderbi@123.122.68.112) (Ping timeout: 240 seconds) [18:08:12] *** Quits: philipp-sk (~Philipp@ktnron0916w-lp130-02-76-66-162-159.dsl.bell.ca) (Ping timeout: 268 seconds) [18:43:44] *** Quits: Guest67547 (~lyan@2605:a000:160e:2dd:4a4d:7eff:fef2:eea3) (Remote host closed the connection) [19:58:20] *** Quits: johnmeneghini (~johnmeneg@pool-100-0-53-181.bstnma.fios.verizon.net) (Quit: Leaving.) [20:57:07] *** Joins: dlw (~Thunderbi@123.122.68.112) [21:59:56] *** Quits: dlw (~Thunderbi@123.122.68.112) (Quit: dlw)