[00:57:37] *** Joins: gila (~gila@5ED4D979.cm-7-5d.dynamic.ziggo.nl) [00:58:22] *** Quits: ziyeyang_ (~ziyeyang@192.102.204.38) (Remote host closed the connection) [01:45:40] @bwalker i've also tested it locally on my machines with no bonds, the same problem occurs. [07:03:57] anyone aware of recent (within the last week) issues with Jenkins either skipping a patch or having build logs missing from a patch that's run through Jenkins? [07:46:20] *** Joins: pniedzwx (pniedzwx@nat/intel/x-aoookqhumofvqjwp) [09:47:29] *** Joins: travis-ci (~travis-ci@ec2-54-166-59-1.compute-1.amazonaws.com) [09:47:30] (spdk/master) bdev/error: remove an unnecessary "enabled" flag (Darek Stojaczyk) [09:47:30] Diff URL: https://github.com/spdk/spdk/compare/94f6d54e2f1a...1abf660320a0 [09:47:30] *** Parts: travis-ci (~travis-ci@ec2-54-166-59-1.compute-1.amazonaws.com) () [09:57:30] *** Joins: travis-ci (~travis-ci@ec2-34-230-69-133.compute-1.amazonaws.com) [09:57:31] (spdk/master) thread: Eliminate use of pthread_self and thread_ids (Ben Walker) [09:57:31] Diff URL: https://github.com/spdk/spdk/compare/1abf660320a0...5977aad8f748 [09:57:31] *** Parts: travis-ci (~travis-ci@ec2-34-230-69-133.compute-1.amazonaws.com) () [10:06:04] *** Joins: travis-ci (~travis-ci@ec2-54-166-43-37.compute-1.amazonaws.com) [10:06:05] (spdk/master) vhost fix potential memleak in vhost_backend_cleanup (wanghonghui) [10:06:05] Diff URL: https://github.com/spdk/spdk/compare/5977aad8f748...554aaffe3bbd [10:06:05] *** Parts: travis-ci (~travis-ci@ec2-54-166-43-37.compute-1.amazonaws.com) () [10:07:20] jimharris, do you want to try and get the reduce deps into the release? Should be read, the CH TP -1 are 2 related failures... https://review.gerrithub.io/c/spdk/spdk/+/429523 [10:08:15] we're currently tagging all of the patches for 19.01 now in an attempt to get organized [10:08:17] definitely [10:08:33] yeah - if you can put a "19.01" hashtag on those patches, i'll make sure to get to them [10:17:27] https://review.gerrithub.io/q/hashtag:%252219.01%2522+status:open [10:21:48] thx [10:22:01] *** Joins: travis-ci (~travis-ci@ec2-107-22-34-167.compute-1.amazonaws.com) [10:22:02] (spdk/master) nvmf/tcp: fix the qpair disconnect handling. (Ziye Yang) [10:22:02] Diff URL: https://github.com/spdk/spdk/compare/554aaffe3bbd...a13a359ebe58 [10:22:02] *** Parts: travis-ci (~travis-ci@ec2-107-22-34-167.compute-1.amazonaws.com) () [10:24:09] peluse: i looked at the close callback for lib/reduce, and i think we can remove it for now - i'm pushing a patch that does that [10:24:48] and for the other issue you saw - where we aren't correctly marking all of the backing io units that contain metadata - i have a patch for that too if you want it [10:24:53] if you already did one we can use yours [10:26:26] jimharris, thanks, nope been on vacation since Wed so I didn't prepare a patch, I'll take yours once its up there. thanks! [10:29:54] *** Joins: travis-ci (~travis-ci@ec2-54-166-70-138.compute-1.amazonaws.com) [10:29:55] (spdk/master) nvme_rdma: multi-element sgl support for inline reqs (Seth Howell) [10:29:55] Diff URL: https://github.com/spdk/spdk/compare/a13a359ebe58...3018bf907bc5 [10:29:55] *** Parts: travis-ci (~travis-ci@ec2-54-166-70-138.compute-1.amazonaws.com) () [11:52:49] *** Joins: travis-ci (~travis-ci@ec2-54-156-91-180.compute-1.amazonaws.com) [11:52:50] (spdk/master) vhost: allow allocating per-session context data (Darek Stojaczyk) [11:52:50] Diff URL: https://github.com/spdk/spdk/compare/b17e0ae7dbfc...0e46c8f63829 [11:52:50] *** Parts: travis-ci (~travis-ci@ec2-54-156-91-180.compute-1.amazonaws.com) () [11:54:39] *** Quits: zhouhui (~wzh@114.255.44.139) (Ping timeout: 268 seconds) [12:05:24] peluse: reviewing the isa-l dependency patch [12:05:44] why do we need to link rte_compressdev if CONFIG_CRYPTO=y but CONFIG_REDUCE=n? [12:12:41] need a few min, on a con call.... [12:21:53] jimharris, this was the big mess that I ran into when building both crypto and compress; the order matters on the DPDK side and we can't control that via the makefile. The reason we ended up splitting like it is now, both API components are built no matter what because of the QAT PMD module, if one the API modules is there but not the other it fails to link. [12:22:16] ok [12:22:19] good enough for me :) [12:22:28] i posted some comments on that first patch [12:22:31] working through the second one now [12:24:48] *** Quits: lhodev (~lhodev@66-90-218-190.dyn.grandenetworks.net) (Ping timeout: 244 seconds) [12:28:37] great, thanks! yeah just saw the comments and will circle back to that after testing a few more ways of doing crypto w/non-contiguous physical buffers. We might have to implement chaining to handle in cleanly [13:26:14] *** Joins: lhodev (~lhodev@inet-hqmc08-o.oracle.com) [13:28:17] *** Quits: lhodev (~lhodev@inet-hqmc08-o.oracle.com) (Remote host closed the connection) [13:29:16] *** Joins: lhodev (~lhodev@148.87.19.222) [13:31:17] *** Quits: lhodev (~lhodev@148.87.19.222) (Remote host closed the connection) [13:31:29] jimharris, your suggestion to lump together the DPDK_LIB_LIST requirements together for either crypto or compress doesn't work, DPD won't build for example if I include the lib 'rte_pmd_isal_comp' without also configuring for compress. So they have to individually include all the libs needed but when configured together not include duplicates. That was the whole reason for _DPDK_LIB_LIST and the sort call at the end. I don't know another way [13:31:29] to do it and looked at this before xmas too with bwalker [13:31:51] *** Joins: lhodev (~lhodev@inet-hqmc08-o.oracle.com) [13:32:16] aren't there basically three sets of libraries? [13:32:39] 1) DPDK framework (anything needed if either compress *or* crypto is enabled) [13:32:46] 2) cryptodev only [13:32:49] 3) compressdev only [13:35:18] i'm just asking that we try to not specify the same library twice if we can avoid it [13:35:44] no, there's just 2 and 3 and then their individual dependencies [13:38:11] well, you need both 2 & 3 no matter what but the problem is you can's add the dependencies one unless its fully configured, an example is above [13:39:28] on the plus side, using the sort method allows us to be explicit about why a lib is being included.... [13:39:58] but there are a lot of cases where the order of the libraries on the link line matters [13:40:17] that doesn't matter now but if we were ever to try to use DPDK shared libraries this wouldn't work [13:40:53] we're careful with SPDK libraries to not include the same library twice on the link line - if there's absolutely no way to do that with the DPDK libraries, then I guess this patch is fine [13:42:24] it looks like if cryptodev *or* compressdev is enabled, then we need to link in this list (rte_cryptodev, rte_compressdev, rte_bus_vdev, rte_mbuf) - I'm guessing rte_pmd_qat will be added to this too once we add QAT offload support for compression [13:42:27] wrt order I can't give you a good answer and Fiona couldn't either, we experimented for well over an hour and her suggestion was aligned with what I did, the use of QAT by both compressdev and cryptodev apparently added a good amount of complexity to the dpdk build process [13:43:40] to your last comment yes, those are all common *but* I'd then have to have another condition for CRYPTO that included the crypto QAT and another condition for REEUCE that, when ready, includes its QAT module [13:44:04] Can do that but IMHO it isn't any better or worse than whats there. I'll see if it works.... [13:46:48] if it works I'll post it on gist and you then you can let me know... thanks! [13:51:17] which of these libraries depends on rte_reorder? [13:51:34] ugh - i forgot how awful these dpdk libraries dependencies were [13:51:44] they are messy [13:52:31] i believe only crypto needs reorder [13:52:52] so do we need reorder if user specifies REDUCE but not CRYPTO? [13:53:24] what if we just *always* link rte_cryptodev, rte_reorder, rte_bus_vdev, etc? [13:54:06] :) [13:54:24] one experiment at a time.... I don't trust "what should work" anymore wrt DPDK deps, LOL [13:55:46] but I don't think that would work because of options we set to dpdkbuild/Makefile. I might have something sorta simple to look at here in a minute, hang tight [13:56:18] i think early on, we tried to only build cryptodev when we needed it, which means we could only link it when we built it [13:56:23] but i'm wondering if that's changed [14:02:46] that's the history for sure, what's changed is the need to always build the 2 API modules, compressdev and cryptodev. I can't generically include rte_reorder, fyi, because we only set the config option to Y if crypto is enabled. I can post what I have now which I do think is actually a bit better or... [14:03:36] we can redo all of the associated options and always build everything that we need for both, we'd have to change the dpdk config file, dpdkbuild/Makefile and the lib/env_dpdk makefile as well [14:04:01] go ahead and post what you have now [14:04:10] well, minus the crypto and compress PMDs because then we'e make everyone have to install ISAL and IPSEC [14:04:17] will do :) [14:04:39] will do a clean build of each option one more time first though... [14:04:42] did you see that e-mail i sent just a bit ago? i'd like to have ISAL on by default since we're going to need it for CRC in iSCSI and NVMe-oF [14:05:01] (checking that I actually copied you on that e-mail) [14:12:24] not yet... will read now. Just pushed https://review.gerrithub.io/c/spdk/spdk/+/429523 let me know what ya think, thanks!! [14:14:51] hmmm, OK. maybe it would be better to submit a new patch on top of this one that includes a configure option for just isa-l? [14:15:17] you mean patch #1 adds ISA-L, then #2 does the compressdev stuff? [14:18:24] well, I was thinking it was easier the other way around since the current patch has all the pkgdep.sh stuff, the submodule, etc., all in it. If you are good with that then a follow-on patch would just be the changes to always include isa-l [14:19:03] or I can go update this patch too, maybe that's easiesst since it doesn't have a +2 yet. Whatcha think? [14:19:44] wait, that's what you suggested in the email :) On it... thanks [14:19:47] hmmm - i was wanting isa-l to *always* get built, meaning you don't even need a configure option - that way iSCSI and NVMe-oF can always just rely on it for CRC calculations [14:19:57] gotcha [14:20:10] but now i'm wondering if that breaks non-x86 [14:21:26] maybe there's a configure option that can only be specified on non-x86 [14:21:38] and is enabled automatically on x86 [14:21:47] then when ARM is added to ISA-L, it can be automatic there too? [14:23:02] hmmm, good point. Other than that I believe the only change needed is to always link it (removing one condition). We rely on configure options to check whether isaL and/or ipsec are there or not so whatever other module wants to use it needs to perform the same check in configure.sh and spit out a message, like the others do, telling the user to install whatever [14:24:19] or if we really want it always on for anyone, I can change pkgdep in this patch and just remove that condition as well, so that it always tells people to install isa-l [14:24:43] we can't always link it - we'll still need to check if this CONFIG_ISAL flag is set (for non-x86 platforms) [14:25:10] it'll just be broken out separately from all of the compressdev stuff that's there currently [14:29:27] OK, its still a pretty small change though, should I proceed to update the current patch or let it land and then update? Doing the ISA-L only first is doable of course but more work. Let me know [14:29:58] i'm fine with either - just want to make sure the first patch is correct wrt all of the rte_reorder, rte_mbuf, etc. [14:36:46] OK, its small enough I'll submit it in the patch after some more testing and if it looks too jumbled just let me know... the rte_ stuff is up there already and I think better than before at least [15:03:28] jimharris, OK its up there. [15:26:05] peluse: overall looks good - a few minor comments [15:30:03] gracias [15:51:10] jimharris, I'm not sure why everything works without linking rte_mbuf. Maybe because we only use the struct definitions and not any functions out of the library?? I'm certain its working and confirmed the linker arguments, its not sneaking in anywhere.... [15:51:57] oh - that could be - if you've confirmed you don't need it, you can ignore that part of my comments [15:54:23] bwalker: question on https://review.gerrithub.io/c/spdk/spdk/+/440598 [15:54:56] jimharris, 10-4 [15:57:31] jenkins issue? [15:57:32] https://review.gerrithub.io/c/spdk/spdk/+/437888/4 [15:57:57] it shows a +1 vote from Jenkins an hour ago, build 19793 [15:58:21] jenkins status page says this patch is still running though - at 52% [15:59:01] if I go to the Jenkins UI itself, it shows that it is completed though [15:59:34] the webpage isn't real time sync'd with the actual status [15:59:45] but 50% seems like a lot [16:00:11] I'll check and see if the page building machine looks OK [16:00:35] jenkins says 19793 started 2:05 ago, and took 40 minutes, meaning it completed in jenkins 1:25 ago [16:02:27] yeah, that seems like something is stuck on the static page builder [16:14:04] I can't tell what's wrong with it, I'll send a note out on the dist list to let everyone know and ask the guys in Poland to check it out when they get in. Note that both the internal and external pages are incorrect. I also checked the new Jenkins job that pushes the artifacts and it seems to be OK from what I can tell.... [16:14:24] ok [16:19:39] peluse: does the nasm check apply to ISAL as a whole, or just the compression routines? [16:20:33] I believe as a whole, greg is on the phone right now though so I can't ask :) [16:23:48] peluse: almost there on the isal patch - just a few small items just posted [16:24:36] OK, cool. Gathering a bit more info on the CI status page issue and will take a look, thanks [16:35:31] jimharris, yes nasm is needed by all of ISAL [16:45:49] cool [16:46:32] what's the proper was to retrigger on Jenkins, now that we have the polling script? is it still the retrigger keyword, or can core maintainers remove the Jenkins vote (like the CTP?) [16:46:58] or is it both? you guys did a ton on Jenkins over the holidays so I feel like there are some things I missed [16:49:50] retrigger as a comment should always work. if you want to do it from the internal Jenkins instance, find the page for the patch in question and there's a menu option that says "retrigger" (I can show you). Or, if you remove the vote, yes the polling script will pick it up but it make take as long as 20 min. We can probably shorten that, the polling period was long because early versions of the script were slow [16:51:01] cool [16:51:11] does the polling script try to search for retrigger comments? [16:51:36] that doesn't sound easy and i'm not asking anyone to do that - just curious [16:51:49] or rather i could say that being non-trivial [16:58:56] no, the polling script will just pick things up that were missed because there was a network outage so the event didn't trigger the build. If you remove the vote, that's kinda the same thing, I'm 99% sure that this action won't trigger and event so the backup poller would have to catch it. I could be wrong though, we might get and event on a vote change... [16:59:33] I'll double check [17:03:01] according to the docs and the UI, a vote change won't generate an event so you're better off using the 'retrigger' word in a comment and this does for sure. But, if you're not in a hurry and it's less typing you can remove the vote :) [17:05:43] actually I found an option where it looks like we could do that too... we can enable if you want but I'd prefer to wait until we have no known issues with Jenkins (ie status page not updating correctly) [17:40:04] *** Joins: zhouhui (~wzh@114.255.44.139) [18:13:34] *** Quits: lhodev (~lhodev@inet-hqmc08-o.oracle.com) (Remote host closed the connection) [18:14:10] *** Joins: lhodev (~lhodev@66-90-218-190.dyn.grandenetworks.net) [21:45:43] *** Joins: ziyeyang_ (~ziyeyang@192.102.204.38) [22:11:14] *** Quits: lhodev (~lhodev@66-90-218-190.dyn.grandenetworks.net) (Ping timeout: 268 seconds) [22:16:30] *** Joins: travis-ci (~travis-ci@ec2-54-208-96-205.compute-1.amazonaws.com) [22:16:31] (spdk/master) vhost: do not close a closed connfd (wanghonghui) [22:16:31] Diff URL: https://github.com/spdk/spdk/compare/0e46c8f63829...3eb66ba88edd [22:16:31] *** Parts: travis-ci (~travis-ci@ec2-54-208-96-205.compute-1.amazonaws.com) () [23:16:08] *** Quits: ziyeyang_ (~ziyeyang@192.102.204.38) (Quit: Leaving)