15:44:47 Apropos of nothing... for Zen2 folks in the audience: 15:44:48 https://lkml.org/lkml/2023/2/22/33 15:45:07 It doesn't impact illumos. We do not use xsaves. 15:46:03 We have plenty of other problems, but at least not that one. 15:49:24 ^ so true. 15:50:12 But at least most of the fpu problems should be resolved before too long once review wraps up. 15:50:13 did the fix ever land for that AMD register that golang was using that we were messing up? 15:50:36 I'm still hoping to get someone to work on https://www.illumos.org/issues/15302 before I have to buy more hardware. 15:50:37 → BUG 15302: broadcom 3108 kernel driver update (New) 15:53:35 nahamu: I'm not sure what you're referring to. 15:53:47 speaking of our own problems... 15:54:36 rmustacc: I think https://github.com/golang/go/issues/53289 turned out to be something deeper. Trying to dig up the link. 15:55:00 https://www.illumos.org/issues/15254 15:55:01 → BUG 15254: %ymm registers not restored after signal handler (New) | https://code.illumos.org/c/illumos-gate/+/2607 15:55:11 Yes, that's what I was referring to as being under review. 15:55:19 Oh, haha 16:07:36 rmustacc: have you seen #15420 -- it kinda feels like maybe the mac_perim_enter() calls in dls_devnet_destroy() are maybe unnecessary (since all the mac_() calls themselves currently recursively enter the perimeter, but i haven't done enough there to feel confident in that) 16:08:37 I saw that it was filed, but I haven't had a chance to look any more than that. 16:10:45 I'm not sure I can look this week, but I'll try to note it as something to come back to. 16:11:58 rmustacc: Where does it say that this depends on XSAVE? 16:12:14 What is 'it'? 16:12:27 The bug Tavis reported? 16:12:46 If that, it states it in the AMD erratum. 16:12:58 And it relies on 'xsavs', not 'xsave'. 16:13:04 *xsaves 16:13:11 rmustacc: Do you have a link onhand? I have not found the official erratum yet. 16:13:34 It was in the thread, but I can pull up the public erratum link. 16:14:02 https://www.amd.com/system/files/TechDocs/56323-PUB_1.00.pdf #1386 16:14:04 Thanks. 16:14:36 Seems likely tied to the xsaves vs. xsave mxcsr handling based on what little information is there. 16:15:28 OpenZFS is using XSAVES since Linux decided to GPL export the symbol for FPU/SIMD context save/restore. :/ 16:17:47 Thanks for the heads up. 16:23:38 You need supervisor state? 16:26:54 rmustacc: No. 16:27:09 Then you never need to use xsaves, xsavec will do you fine. 16:27:52 Thanks for the tip. I will note that in the OpenZFS issue I made to track this. 16:38:20 I recall that OpenSolaris was subject to at least periodic coverity scans. Is there any plan to resume those for illumos: https://scan.coverity.com/projects/illumos 16:39:50 I'd love to see them done... 16:42:19 Speaking of which, coverity caught a NULL pointer dereference in libuutil. I wrote a fix for it for OpenZFS, but it was dead code for us, so we deleted the code from our libuutil library instead. I feel somewhat bad about that. Anyway, the fix is here if anyone wants it: https://github.com/openzfs/zfs/commit/27803f6be11ab7f2457ee6df956cc297297a0a48 16:42:52 I imagine that will eventually be garbage collected by github, so if anyone does want it, I suggest saving it somewhere. 16:44:03 coverity scan seems very unreactive to me 16:44:28 paulf: What do you mean? 16:44:52 made a project request in January, still pending 16:45:31 (we have Coverity at work, so I can run local scans when noone is looking) 16:49:15 paulf: There are two projects for illumos. One for illumos proper and another for illumos/illumos-gate on github. The github one should be usable immediately. 16:50:08 Also, speaking of coverity reminded me of a few patches that fix things static analysis found that are applicable to illumos: https://github.com/openzfs/zfs/commit/6ae2f9088853737b87998408656fe4c3a7b9235b https://github.com/openzfs/zfs/commit/a2163a96ae8708bb083e8da7658c02a7047516ba 16:50:33 There are more patches in OpenZFS that are applicable, but those two struck me as being something I should highlight since they involve encryption / checksums. 16:52:17 There had been some refactoring of the ICP code, so things done there no longer always apply, but I want to mention that we also did some work to zero memory prior to exit from encryption functions. It does not apply directly to the illumos icp code since it builds on prior work in this area: https://github.com/openzfs/zfs/commit/f58e513f7408f353bf0151fdaf235d4e062e8950 16:52:23 https://github.com/openzfs/zfs/commit/d634d20d1be31dfa8cf06ef2dc96285baf81a2fb 16:53:40 That was two patches rather than one since I was late to review the recent PR for this. I ended up writing a second patch that went into a PR (and was merged very quickly) to fix the first patch, since memset() was being optimized away by dead store optimization. 16:57:14 the concern I have with the libuutil fix is that generally passing a NULL format is itself an error -- perhaps an ASSERT or VERIFY is in order? 16:57:21 I wasn't referring to the illumos scan project 17:10:44 paulf: Which one did you mean? 17:11:09 sommerfeld: That sounds better than my solution. 17:15:36 In addition, the attribute((format, ...)) stuff (gcc version of lint's PRINTFLIKE*) can be used to catch this sort of thing at compile time. 17:15:52 you want to catch this at the source, several layers up the call chain. 17:16:39 if it really is printf-like, it should be annotated as such. 17:17:48 we have compilers capable of doing such checks, so we should really use that. 17:21:20 sommerfeld: I did not know that it could catch that, but I did know that it could catch things like improper types in the format string. I should have a WIP patch to use it in OpenZFS. It will be finished eventually. 17:21:27 yep, format passes straight through to vfprintf 17:22:04 Oh, it was just for ZED since the kernel code was a bit of a pain, but if I recall correctly, I was planning to revise it to cover the kernel too: https://github.com/openzfs/zfs/pull/13956 17:25:15 Is there anything I could contribute to help push https://www.illumos.org/issues/12455 forward? 17:25:16 → FEATURE 12455: SO_REUSEPORT support (New) | https://code.illumos.org/c/illumos-gate/+/464 17:33:09 ryao: while the documentation doesn't say that it specifically checks for NULL format strings, a quick check shows that -Wformat complains. 17:33:24 Nice. :) 17:34:21 ryao valgrind 17:35:05 paulf: What about valgrind? You are the second person in 15 minutes to mention valgrind to me. The other mentioned it in the context of it quitting when it encounters avx2/avx512 instructions in OpenZFS that it does not understand. 17:35:55 (there's also attribute((nonnull, x, y)) but it may make the optimizer more likely to summon nasal daemons) 17:36:44 sommerfeld: I am actually using that to improve clang's static analyzer to understand KM_SLEEP locally if I recall correctly. I want to submit patches to OpenZFS eventually, but they are a bit messy. 17:37:13 By patches, I mean patches that will let anyone using clang's static analyzer have it understand KM_SLEEP. 17:38:46 https://github.com/ryao/zfs/commit/6fc6ceaa2dc9eb6c5984191032b727e36a5aff5c 17:39:25 ah, conditional attribute((returns_nonnull)) ? 17:40:13 sommerfeld: Yes. I noticed that abusing memset() would teach it about kmem_zalloc() and doing a conditional call to a shim function that has the attribute would teach it about KM_SLEEP. 17:40:53 It also eliminates the need to encode this information into the coverity model file. 17:41:32 The OpenZFS one is in git. Remove the linuxisms and it should be useful for illumos. https://github.com/openzfs/zfs/blob/master/contrib/coverity/model.c 17:43:04 teutat3s: I have some exposure to SO_REUSEPORT. There are a bunch of subtleties around it depending on what you want out of it (I've seen it used for MP loadbalancing and also for seamless handover when upgrading a binary..) 17:47:25 teutat3s: are you looking for improved performance or efficiency through using SO_REUSEPORT, or are you just looking to compile some code that works on linux and have it work on illumos? 17:51:19 The latter 17:52:10 Caddy, a reverse proxy written in go, currently has to be patched like this to to make it compile on illumos 17:52:12 https://github.com/NetBSD/pkgsrc/blob/trunk/www/caddy/patches/patch-listen__illumos.go 17:54:17 Currently gracefully reloading caddy is not possible like this, it shows an error when trying to re-binding to its API port 2019 17:54:18 listen tcp 127.0.0.1:2019: bind: address already in use 17:55:19 jbk: re #15420: dls_devnet_destroy() calls dls_link_rele_by_name() which asserts the mac perim and then calls into the mac_link_has_flows(), and if you look at the mac_flow code you'll see it asserts the perim is held when adding/removing flows 17:56:52 [illumos-gate] 15371 want kstat for vmm reservoir -- Patrick Mooney 17:57:32 sommerfeld (LIBERA-IRC): If I understand it correctly, SO_REUSEPORT would enable the caddy reload command to work on illumos, which is crucial for a proxy, 0 downtime reloading it's config 18:00:51 rzezeski: yeah there's a lot of that, but in a lot of cases then it's before calling functions that go ahead and (in that case) do a recursive enter.. i think that might (at least in the destroy case) be the one spot where it doesn't (but could be worked around)... 18:01:47 if this was just a simple mutex, an analogous fix would be to drop the mutex while doing the wait 18:02:11 but i'm not sure there's really a way to do the equivalent w/ the mac perimeter 18:07:15 right but in those cases it might be to make things atomic, I'd have to reread notes from 5 years ago, but I do know there was at least some plans to merge DLS/mac based on some bug/PSARC I read, and I want to say that maybe in places the mac perim is doing more duty than just protecting mac structures...I know Robert wrote a very details big theory statement on all this that might cover it. 18:07:19 in general, there are dragons 18:07:38 also there is a non-public smartos bug I filed that might be a similar deadlock 18:07:49 and I remember coming to the conlusion there was no way to fix it 18:08:09 not saying they are the same, but IIRC it involved module unload and softmac 18:08:40 OS-6356 18:08:40 yeah..we're working around it by just removing the modunload -i 0 call from rc3 since the memory savings is basically a rounding error 18:08:51 if someone has the ability to get you the content of that bug 18:09:12 but it'd be nice to fix the actual bug 18:09:29 (I suspect it may be possible to hit this on a DEBUG kernel as well if the timing is just right) 18:09:50 well I think that's how I hit OS-6356 18:09:56 but without access to it I don't remember for sure 18:10:21 just doing teh quick workaround always feels like a recipie for it to bite you in the ass when you least want it to :) 18:10:32 well that's why I'm warning you 18:10:39 you'll want to test it hard 18:11:10 it this was 2019 I could have given you a much more authoritative answer, but alas most of that has paged out 18:13:58 teutat3s: okay, so that's the seamless handover use case. I'd ping the original developer of those patches and see where they are with it (why it stalled out..). 18:15:43 sommerfeld (LIBERA-IRC): Thank you, will do 18:17:50 the one thing i was thinking, but haven't been able to convince myself is actually true is that dls is maybe using the perimeter to make the reference counting on a dls_link_t work correctly 18:17:57 it's sketchy enough i'm skeptical 18:18:29 but i also haven't figured out how else the refcounting for dls_link_t is otherwise safe 18:19:25 as the examples i find are basically 'mod_hash_find(..., &dlp); dlp->dl_ref++' without any obvious lock held across that 18:20:10 teutat3s: on linux, there's a use case where you bind one listener thread per NIC receive queue, and use an eBPF hook to implement the same rx hash distribution as the NIC to shard incoming traffic across cpus. oh, and then you have to hand that off cleanly across a restart, too. 18:21:19 jbk: like I said, there is some bug/PSARC/commit that I stumbled across back in like 2017 that talked about merging DLS/mac, and when the illumos fork happened things were still in flux so it could be that DLS has some reliance on the mac perimeter 18:24:31 ryao: I'm filing a bug on the PRINTFLIKE->attribute((format(...))) thing. 18:32:39 jbk: https://smartos.org/bugview/OS-3506 18:32:42 → OS-3506: dls and mac lock ordering isn't honored (Resolved) | https://github.com/joyent/illumos-joyent/commit/aefd5a8 18:32:54 see the 3rd to last comment (Created at 2016-11-09T02:00:00.000Z) 18:33:50 Filed as https://www.illumos.org/issues/15427 18:33:52 → BUG 15427: many functions marked /*PRINTFLIKE..*/ should get attribute((format, ...) annotations. (New) 18:35:25 I have a lot of lock ordering fixes from SmartOS in illumos 15167 (fenix) which is up for review. 18:35:26 BUG 15167: Panic when halting a zone with self-created links (In Progress) 18:35:26 ↳ https://www.illumos.org/issues/15167 | https://code.illumos.org/c/illumos-gate/+/2662 18:37:53 andyf: yea I think you pulled in those fixes I made, did you made additional lock order changes? 18:39:23 No, after that change things are aligned with SmartOS there. 18:39:34 So it may not fix the specific dls_devnet_destroy() issue that jbk has hit 18:40:06 As you'll know (or deliberately forgotten) there was a lot of back and forth in SmartOS to get this right - it's not an easy problem. 18:40:38 So when I was done, I diffed the two code bases too, and picked up any additional helpful comments you'd left. 18:46:36 yeah at first glance those changes I don't think will help 18:47:52 i suppose to really narrow it down, it's calling `mac_notify_remove(..., B_TRUE)` while holding the mac perimeter (prior to calling that) that's the issue 18:48:26 https://code.illumos.org/c/illumos-gate/+/2563 is ready now, if you like, please add your +1:) 18:48:27 → CODE REVIEW 2563: 15265 loader: add mechanism to set os console based on last input device (NEW) | https://www.illumos.org/issues/15265 18:49:47 i think what aggr is doing with walking the notify list seems ok there 19:51:15 tsoome: thanks for loader, in general, by the way. 19:51:34 tsoome: I just noticed I have 46 BEs, which has to be over the grub limit before you get screwed. 20:10:13 @tsoome + int res __maybe_unused; 20:10:18 That seems to trainwreck my build. 20:10:31 (Which is weird as it didn't seem to trainwreck it locally.) 20:14:52 smartos-live does build sgs outside illumos-gate for $HISTORICAL_REASONS but I guess I'll have to figure out a few things. 20:18:08 [2023-03-01T19:47:54.709Z] /root/data/jenkins/workspace/smartos-master-debug/projects/illumos/usr/src/cmd/sgs/common/string_table.c:730:11: error: expected '=', ',', ';', 'asm' or '__attribute__' before '__maybe_unused' 20:27:29 Why would __maybe_unused fail in specific compilation environments? 20:28:55 Because it's coming from sys/ccopmile.h 20:29:19 It was added in April. 20:29:19 Hmmm.... oh shit... 20:32:33 Yeah... looks like for some bizarre reason the before-we-do-illumos-nightly portion of "tools/build_illumos" we might be taking something out of the platform's /usr/include. 20:32:37 I gotta check some things locally. 20:32:43 Thank you rmustacc 20:34:11 danmcd: Probably comes from something I did before there was a tools proto build of ld. 20:34:24 But you'd have to look at the history there. 20:37:11 that definitely looks like old headers 20:37:22 but we (should) always build the tools ld against the system headers, except a magical subset of ELF-y ones 20:37:36 so it's probably a matter of keeping your build machines up-to-date 20:37:38 which EVERYONE SHOULD 20:41:14 andyf: good catch on that asy bug -- I'm pretty sure we hit that more than a few times at Joyent 20:41:20 but never were able to completely track it down 20:50:54 I notice nothing in sgs explicitly includes sys/ccompile.h. 20:50:59 I wonder if simply doing that might help? 20:52:06 nope 20:52:13 keep your build machine up to date 20:52:35 (I mean, unless toomas broke it completely somehow) 20:52:57 andyf: btw, this is the kind of thing where I was saying that using a full package image as an adjunct is good for everyone. 20:53:48 I have a rigged experiment (revert the definition of __maybe_unused from platform ccompile.h using lofs) which *should* trainwreck if my theory is right. 20:53:57 And yes I just proved it's right. I have one small fix to try... 20:54:54 ... nope that failed BECAUSE I believe `dmake setup` for some bizarre reason does NOT install headers in proto. 20:54:59 (And I thought it would!) 20:55:05 it does 20:55:12 (should) 20:55:17 unless you smartos-d something weirdly 20:55:31 see the *headers targets in usr/src/Makefile, I believe ultimately under the sgs target. 20:55:53 (an unfortunate use of "sgs" there, for those who remember "software generation system") 20:57:09 setup: closedbins bldtools sgs mapfiles 20:57:22 I think bldtools is building *part* of sgs before the sgs target is ready. :( 20:57:50 bldtools uses _the system headers_ though, because the tools run on the build system. 20:58:06 they explicitly (if not always successfully) _don't_ use the workspace headers. 20:59:06 think about my situation, for eg, where the tools we build and the workspace headers we install aren't for the same platform. 20:59:25 Yep! 21:00:14 sgsmsg includes the $CMD/sgs/common/string_table.c source file, which MUST have __maybe_unused defined in the platform/system's /usr/include/sys/ccompile.h 21:00:35 So basically fenix illumos#15292 is a update-your-build-machine flag day. 21:00:36 BUG 15292: sgs: set but not used variable (Closed) 21:00:37 ↳ https://www.illumos.org/issues/15292 | https://code.illumos.org/c/illumos-gate/+/2580 21:00:56 Either THAT or we need to have tools use of that file be more clever somehow. 21:01:16 So glad my Jenkins systems run older PIs. :) 21:02:41 Anyway, I await further advise. I might, in the meantime, have 15292 get lofs-patched on my older PI build machines. 21:02:48 It should be a NOP for everything else. 21:03:10 ouch... 21:03:13 I can file a bug if need be. 21:03:25 you should update your build machine 21:03:45 or as I hinted in the aside to andyf, we should get full adjuncts as a first-class thing 21:04:00 but that's less workable with NATIVE_ADJUNCT 21:05:31 Then we need to announce the heads-up/flag-day. 21:05:39 if it's just that definition, you could probably add something in usr/src/tools/sgs to define it if not present for older build envs.. I know that approach was taken for a few things before 21:06:04 I'm going to try lofs-patching my Jenkins machines older than 20220417 (when __maybe_unused landed in SmartOS) and see if that helps. 21:06:48 The change for fenix illumos#14622 is pretty self-contained. 21:06:49 FEATURE 14622: want __maybe_unused (Closed) 21:06:49 ↳ https://www.illumos.org/issues/14622 | https://code.illumos.org/c/illumos-gate/+/2100 21:08:52 jbk: we do that for elf-y things that are integral, it's not a catch all. 21:09:09 and we generally don't flag-day/heads-up "update your build machine", you're just expected to 21:09:28 especially in cases like this, where it's basically a year old. 21:10:10 in the Sun days it would be done if you needed something magical until the next real upgrade, but iirc you were expected to generally be within a couple of builds. 21:10:18 Okay then. 21:10:43 It will be a flag day for SmartOS then. But that's a my-distro problem not a here problem. 21:10:45 it's not a bad idea, but it's hard to do, because it involves everyone knowing when all their dependencies appeared :) 21:11:58 I believe this is what FreeBSD do with the __FreeBSD_version define 21:12:41 which would probably be a thing to raise with core folks? it seems a good idea? 21:13:30 Probably worth thinking about and how we want to tie it to produced sysroots and related bits. There are a bunch of tradeoffs here. 21:13:57 well, for dan's specific problem a sysroot won't help, because it really is _native_ stuff. 21:14:10 unless the native_adjunct stuff becomes much smarter. 21:14:13 I understand. 21:15:09 I was thinking a __FreeBSD_version-ish scheme at least would let us check 21:15:24 though it has the same problem of knowing your dependencies are recent, and that people's systems are old. 21:16:05 Okay, I'm declaring this a SmartOS-only problem at worst, and a our-jenkins-agents problem at best. Thanks folks, sorry for the distraction. 21:35:45 jbk - thanks, I had help from cross to track that down, it was an interesting one. I have more analysis I should probably copy into the ticket. 22:09:57 more analysis more better. 22:10:05 not that I'm a good example of that half the time