Apropos of nothing... for Zen2 folks in the audience:

It doesn't impact illumos. We do not use xsaves.

We have plenty of other problems, but at least not that one.

^ so true.

But at least most of the fpu problems should be resolved before too long once review wraps up.

did the fix ever land for that AMD register that golang was using that we were messing up?

I'm still hoping to get someone to work on illumos.org/issues/15302 before I have to buy more hardware.

→ BUG 15302: broadcom 3108 kernel driver update (New)

nahamu: I'm not sure what you're referring to.

speaking of our own problems...

rmustacc: I think golang/go #53289 turned out to be something deeper. Trying to dig up the link.

→ BUG 15254: %ymm registers not restored after signal handler (New) | code.illumos.org/c/illumos-gate/+/2607

Yes, that's what I was referring to as being under review.

Oh, haha

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)

I saw that it was filed, but I haven't had a chance to look any more than that.

I'm not sure I can look this week, but I'll try to note it as something to come back to.

rmustacc: Where does it say that this depends on XSAVE?

What is 'it'?

The bug Tavis reported?

If that, it states it in the AMD erratum.

And it relies on 'xsavs', not 'xsave'.

*xsaves

rmustacc: Do you have a link onhand? I have not found the official erratum yet.

It was in the thread, but I can pull up the public erratum link.

amd.com/system/files/TechDocs/56323-PUB_1.00.pdf #1386

Thanks.

Seems likely tied to the xsaves vs. xsave mxcsr handling based on what little information is there.

OpenZFS is using XSAVES since Linux decided to GPL export the symbol for FPU/SIMD context save/restore. :/

Thanks for the heads up.

You need supervisor state?

rmustacc: No.

Then you never need to use xsaves, xsavec will do you fine.

Thanks for the tip. I will note that in the OpenZFS issue I made to track this.

I recall that OpenSolaris was subject to at least periodic coverity scans. Is there any plan to resume those for illumos: scan.coverity.com/projects/illumos

I'd love to see them done...

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: openzfs/zfs 27803f6

I imagine that will eventually be garbage collected by github, so if anyone does want it, I suggest saving it somewhere.

coverity scan seems very unreactive to me

paulf: What do you mean?

made a project request in January, still pending

(we have Coverity at work, so I can run local scans when noone is looking)

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.

Also, speaking of coverity reminded me of a few patches that fix things static analysis found that are applicable to illumos: openzfs/zfs 6ae2f90 openzfs/zfs a2163a9

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.

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: openzfs/zfs f58e513

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.

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?

I wasn't referring to the illumos scan project

paulf: Which one did you mean?

sommerfeld: That sounds better than my solution.

In addition, the attribute((format, ...)) stuff (gcc version of lint's PRINTFLIKE*) can be used to catch this sort of thing at compile time.

you want to catch this at the source, several layers up the call chain.

if it really is printf-like, it should be annotated as such.

we have compilers capable of doing such checks, so we should really use that.

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.

yep, format passes straight through to vfprintf

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: openzfs/zfs #13956

Is there anything I could contribute to help push illumos.org/issues/12455 forward?

→ FEATURE 12455: SO_REUSEPORT support (New) | code.illumos.org/c/illumos-gate/+/464

ryao: while the documentation doesn't say that it specifically checks for NULL format strings, a quick check shows that -Wformat complains.

Nice. :)

ryao valgrind

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.

(there's also attribute((nonnull, x, y)) but it may make the optimizer more likely to summon nasal daemons)

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.

By patches, I mean patches that will let anyone using clang's static analyzer have it understand KM_SLEEP.

ah, conditional attribute((returns_nonnull)) ?

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.

It also eliminates the need to encode this information into the coverity model file.

The OpenZFS one is in git. Remove the linuxisms and it should be useful for illumos. github.com/openzfs/zfs/blob/master/contrib/coverity/model.c

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..)

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?

The latter

Caddy, a reverse proxy written in go, currently has to be patched like this to to make it compile on illumos

Currently gracefully reloading caddy is not possible like this, it shows an error when trying to re-binding to its API port 2019

listen tcp 127.0.0.1:2019: bind: address already in use

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

[illumos-gate] 15371 want kstat for vmm reservoir -- Patrick Mooney <pmooney⊙pc>

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

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)...

if this was just a simple mutex, an analogous fix would be to drop the mutex while doing the wait

but i'm not sure there's really a way to do the equivalent w/ the mac perimeter

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.

in general, there are dragons

also there is a non-public smartos bug I filed that might be a similar deadlock

and I remember coming to the conlusion there was no way to fix it

not saying they are the same, but IIRC it involved module unload and softmac

OS-6356

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

if someone has the ability to get you the content of that bug

but it'd be nice to fix the actual bug

(I suspect it may be possible to hit this on a DEBUG kernel as well if the timing is just right)

well I think that's how I hit OS-6356

but without access to it I don't remember for sure

just doing teh quick workaround always feels like a recipie for it to bite you in the ass when you least want it to :)

well that's why I'm warning you

you'll want to test it hard

it this was 2019 I could have given you a much more authoritative answer, but alas most of that has paged out

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..).

sommerfeld (LIBERA-IRC): Thank you, will do

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

it's sketchy enough i'm skeptical

but i also haven't figured out how else the refcounting for dls_link_t is otherwise safe

as the examples i find are basically 'mod_hash_find(..., &dlp); dlp->dl_ref++' without any obvious lock held across that

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.

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

ryao: I'm filing a bug on the PRINTFLIKE->attribute((format(...))) thing.

jbk: smartos.org/bugview/OS-3506

→ OS-3506: dls and mac lock ordering isn't honored (Resolved) | joyent/illumos-joyent aefd5a8

see the 3rd to last comment (Created at 2016-11-09T02:00:00.000Z)

Filed as illumos.org/issues/15427

→ BUG 15427: many functions marked /*PRINTFLIKE..*/ should get attribute((format, ...) annotations. (New)

I have a lot of lock ordering fixes from SmartOS in illumos 15167 (fenix) which is up for review.

BUG 15167: Panic when halting a zone with self-created links (In Progress)

↳ illumos.org/issues/15167 | code.illumos.org/c/illumos-gate/+/2662

andyf: yea I think you pulled in those fixes I made, did you made additional lock order changes?

No, after that change things are aligned with SmartOS there.

So it may not fix the specific dls_devnet_destroy() issue that jbk has hit

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.

So when I was done, I diffed the two code bases too, and picked up any additional helpful comments you'd left.

yeah at first glance those changes I don't think will help

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

code.illumos.org/c/illumos-gate/+/2563 is ready now, if you like, please add your +1:)

→ CODE REVIEW 2563: 15265 loader: add mechanism to set os console based on last input device (NEW) | illumos.org/issues/15265

i think what aggr is doing with walking the notify list seems ok there

tsoome: thanks for loader, in general, by the way.

tsoome: I just noticed I have 46 BEs, which has to be over the grub limit before you get screwed.

@tsoome + int res __maybe_unused;

That seems to trainwreck my build.

(Which is weird as it didn't seem to trainwreck it locally.)

smartos-live does build sgs outside illumos-gate for $HISTORICAL_REASONS but I guess I'll have to figure out a few things.

[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'

Why would __maybe_unused fail in specific compilation environments?

Because it's coming from sys/ccopmile.h

It was added in April.

Hmmm.... oh shit...

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.

I gotta check some things locally.

Thank you rmustacc

danmcd: Probably comes from something I did before there was a tools proto build of ld.

But you'd have to look at the history there.

that definitely looks like old headers

but we (should) always build the tools ld against the system headers, except a magical subset of ELF-y ones

so it's probably a matter of keeping your build machines up-to-date

which EVERYONE SHOULD

andyf: good catch on that asy bug -- I'm pretty sure we hit that more than a few times at Joyent

but never were able to completely track it down

I notice nothing in sgs explicitly includes sys/ccompile.h.

I wonder if simply doing that might help?

nope

keep your build machine up to date

(I mean, unless toomas broke it completely somehow)

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.

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.

And yes I just proved it's right. I have one small fix to try...

... nope that failed BECAUSE I believe `dmake setup` for some bizarre reason does NOT install headers in proto.

(And I thought it would!)

it does

(should)

unless you smartos-d something weirdly

see the *headers targets in usr/src/Makefile, I believe ultimately under the sgs target.

(an unfortunate use of "sgs" there, for those who remember "software generation system")

setup: closedbins bldtools sgs mapfiles

I think bldtools is building *part* of sgs before the sgs target is ready. :(

bldtools uses _the system headers_ though, because the tools run on the build system.

they explicitly (if not always successfully) _don't_ use the workspace headers.

think about my situation, for eg, where the tools we build and the workspace headers we install aren't for the same platform.

Yep!

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

So basically fenix illumos#15292 is a update-your-build-machine flag day.

BUG 15292: sgs: set but not used variable (Closed)

↳ illumos.org/issues/15292 | code.illumos.org/c/illumos-gate/+/2580

Either THAT or we need to have tools use of that file be more clever somehow.

So glad my Jenkins systems run older PIs. :)

Anyway, I await further advise. I might, in the meantime, have 15292 get lofs-patched on my older PI build machines.

It should be a NOP for everything else.

ouch...

I can file a bug if need be.

you should update your build machine

or as I hinted in the aside to andyf, we should get full adjuncts as a first-class thing

but that's less workable with NATIVE_ADJUNCT

Then we need to announce the heads-up/flag-day.

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

I'm going to try lofs-patching my Jenkins machines older than 20220417 (when __maybe_unused landed in SmartOS) and see if that helps.

The change for fenix illumos#14622 is pretty self-contained.

FEATURE 14622: want __maybe_unused (Closed)

↳ illumos.org/issues/14622 | code.illumos.org/c/illumos-gate/+/2100

jbk: we do that for elf-y things that are integral, it's not a catch all.

and we generally don't flag-day/heads-up "update your build machine", you're just expected to

especially in cases like this, where it's basically a year old.

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.

Okay then.

It will be a flag day for SmartOS then. But that's a my-distro problem not a here problem.

it's not a bad idea, but it's hard to do, because it involves everyone knowing when all their dependencies appeared :)

I believe this is what FreeBSD do with the __FreeBSD_version define

which would probably be a thing to raise with core folks? it seems a good idea?

Probably worth thinking about and how we want to tie it to produced sysroots and related bits. There are a bunch of tradeoffs here.

well, for dan's specific problem a sysroot won't help, because it really is _native_ stuff.

unless the native_adjunct stuff becomes much smarter.

I understand.

I was thinking a __FreeBSD_version-ish scheme at least would let us check

though it has the same problem of knowing your dependencies are recent, and that people's systems are old.

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.

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.

more analysis more better.

not that I'm a good example of that half the time