Details

Reviewers

allanjude
markj
def
bcr

Summary

Kernel dump Zstd compression can be enabled by adding '-Z' to dumpon_flags
in rc.conf(5). savecore(8) will automatically save kernel zstd dumps with a
.zst extension. Zstd compression level can be configured with the
sysctl/tunable kern.kerneldump_zstdlevel. Note that setting the sysctl at
runtime does not immediately change the compression level that will be used
at dump time. To update that, run "service dumpon restart".

Support is also added for user core dumps compressed with Zstd. User
Zstd-compressed cores can be enabled via the sysctl/tunable
kern.compress_user_cores. If your kernel is configured with both GZIO and
ZSTDIO options, set sysctl/tunable kern.compress_user_cores to "2" to
compress userspace cores with Zstd. The Zstd compression level can be
configured with the sysctl/tunable kern.compress_user_cores_zstdlevel.

For both kernel and userspace core dump compression, the default Zstd level
is arbitrarily chosen as 6.

This revision adds the ZSTDIO option to AMD64 GENERIC by default. GZIO is
still a non-default option.

Test Plan

Userspace cores seem to work:

$ sysctl kern. | grep compress
kern.compress_user_cores_zstdlevel: 6
kern.compress_user_cores: 2
$ sysctl kern.corefile
kern.corefile: %N.core

$ python2 -c 'import os; os.abort()'
[1]    1156 abort (core dumped)  python2 -c 'import os; os.abort()'

$ ls -l python2.7.core.zst
-rw-------    1 conrad  conrad   429K Nov 15 12:17 python2.7.core.zst
$ zstd --test ./python2.7.core.zst
./python2.7.core.zst: 12894208 bytes

$ echo $(( 12894208 / 1024 ))
12592
$ echo $(( 100 * 429 / 12592 ))
3    # compressed size as percent of uncompressed core size

$ zstd -d ./python2.7.core.zst
./python2.7.core.zst: 12894208 bytes
$ file python2.7.core
python2.7.core: ELF 64-bit LSB core file x86-64, version 1 (FreeBSD), FreeBSD-style, from ' -c import os; os.abort()'

Kernel full dumps also work:

$ dumpon -l
gpt/freebsd-swap
$ dumpon -Z /dev/$(dumpon -l)

$ sysctl debug.{trace,debugger}_on_panic debug.minidump
debug.trace_on_panic: 1
debug.debugger_on_panic: 1
debug.minidump: 1

$ debug.minidump=0

$ sysctl kern.kerneldump_zstdlevel
kern.kerneldump_zstdlevel: 6

$ sysctl debug.kdb.panic=1

(core is dumped, reboot)

messages / console:
Nov 15 13:54:33 n savecore: reboot after panic: kdb_sysctl_panic
Nov 15 13:54:33 n savecore: writing core to /var/crash/vmcore.1.zst

$ cd /var/crash ; ls -lhtr
...
-rw-------  1 root  wheel    77M Nov 15 13:54 vmcore.1.zst
lrwxr-xr-x  1 root  wheel     6B Nov 15 13:54 info.last -> info.1
lrwxr-xr-x  1 root  wheel    12B Nov 15 13:54 vmcore.last.zstd -> vmcore.1.zst
-rw-r--r--  1 root  wheel    30B Nov 15 13:54 core.txt.1

$ zstd --test ./vmcore.1.zst ; echo $?
./vmcore.1.zst      : 34235252736 bytes
0

$ less core.txt.1
/var/crash/vmcore.1 not found

(That integration would be nice to fix, but requires unpacking the core somewhere, or creating seekable coredumps.)

$ zstdcat ./vmcore.1.zst | file -
/dev/stdin: ELF 64-bit LSB core file x86-64, invalid version (embedded)

And here's minidumps:

$ dumpon -Z /dev/$(dumpon -l)
$ sysctl debug.kdb.panic=1

(core is dumped, reboot)

Nov 19 19:38:17 n savecore: reboot after panic: kdb_sysctl_panic
Nov 19 19:38:17 n savecore: writing core to /var/crash/vmcore.2.zst

$ cd /var/crash ; ls -lhtr
...
lrwxr-xr-x  1 root  wheel    12B Nov 15 13:54 vmcore.last.zstd -> vmcore.2.zst
-rw-r--r--  1 root  wheel     2B Nov 19 19:38 bounds
-rw-------  1 root  wheel   534B Nov 19 19:38 info.2
-rw-------  1 root  wheel    39M Nov 19 19:38 vmcore.2.zst
lrwxr-xr-x  1 root  wheel     6B Nov 19 19:38 info.last -> info.2
-rw-r--r--  1 root  wheel    30B Nov 19 19:38 core.txt.2

$ cat info.2
Dump header from device: /dev/gpt/freebsd-swap
...
  Compression: zstd

$ zstd -d --keep < ./vmcore.2.zst | strings | head -n1
minidump FreeBSD/amd64

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 12937
Build 13197: arc lint + arc unit

Event Timeline

cem created this revision.Nov 15 2017, 10:41 AM

Harbormaster completed remote builds in B12757: Diff 35273.Nov 15 2017, 10:41 AM

cem added a parent revision: D10407: Modify zstd so it can be built into the kernel as well.Nov 15 2017, 10:42 AM

OK from manpages (implicit). ;-)

This revision is now accepted and ready to land.Nov 15 2017, 12:25 PM

cem edited the summary of this revision. (Show Details)Nov 15 2017, 9:03 PM

cem edited the test plan for this revision. (Show Details)

cem edited the test plan for this revision. (Show Details)Nov 15 2017, 10:14 PM

Add missed 'Z' to dumpon(8) getopt invocation.

This revision now requires review to proceed.Nov 15 2017, 10:57 PM

Harbormaster completed remote builds in B12770: Diff 35303.Nov 15 2017, 10:57 PM

cem added inline comments.Nov 15 2017, 11:27 PM

sbin/dumpon/dumpon.c
276–278	Darius points out that `if (zstd && gzip)` should probably `err()`.

Any chance of zstdio_*(9) man pages? (even a stub would be useful IMO)

sbin/dumpon/dumpon.c
224	Please add a check for zstd && gzip and produce an error if true.
sys/kern/kern_zstdio.c
123	If this is expected to never happen it should probably be an assert instead.

rpokala added a subscriber: rpokala.Nov 15 2017, 11:39 PM

rpokala added inline comments.

sys/kern/imgact_elf.c
1190	s/gzsts/zsts/g, since there's nothing GNUish about zstd.
1225	Shouldn't there also be a `coredump_gz`? Or else an enum for all the supported compression types?
1471–1479	Most other places, you do the GZIO case first, then the ZSTDIO case; here, it's gratuitously reversed.
sys/kern/kern_shutdown.c
1418	"enough"

Have dumpon reject zstd && gzip (darius)
Assert resetStream success (darius)
Rename gzsts, since indeed it has nothing to do with GNU (Ravi)
"enouch" (Ravi)

Harbormaster completed remote builds in B12772: Diff 35305.Nov 16 2017, 3:15 AM

cem added inline comments.Nov 16 2017, 3:15 AM

sys/kern/imgact_elf.c
1225	This is essentially an enum of all supported compression types :-D. I'm open to expanding it to an int or whatever, but right now there are only two types. Maybe an int would be more future proof.
1471–1479	I tend to favor `if (foo) { } else { }` over `if (!foo) { } else { }`. The sense of the boolean `use_zstd` puts that case first. I can swap just the cases, or the boolean as well, if you like. I don't have a strong opinion about it.

rpokala added inline comments.Nov 16 2017, 3:26 AM

sys/kern/imgact_elf.c
1225	Right now, there are three formats: uncompressed, gz, zstd. I'd think an enum would be appropriate. For that matter, couldn't `KERNELDUMP_COMP_{NONE,GZIP,ZSTD}` be converted to an enum, and then used throughout?
1471–1479	As noted above, if you were to switch off of a `KERNELDUMP_COMP_{NONE,GZIP,ZSTD}` enum, then all this becomes cleaner.

Switch off of a compression mode enumeration for userspace cores.

Harbormaster completed remote builds in B12773: Diff 35306.Nov 16 2017, 4:09 AM

In D13101#272595, @cem wrote:

Switch off of a compression mode enumeration for userspace cores.

That looks much better. Thanks for humoring me. :-)

Why do you disable debug.minidump in the "testing" section? I don't see any obvious reason this shouldn't work with both full dumps and minidumps. At least, the gzip support doesn't have any such limitations.

[Sorry for the delay in reviewing; I'm about to start flying back home, so I'll be able to comment further on Monday.]

In D13101#273868, @markj wrote:

Why do you disable debug.minidump in the "testing" section? I don't see any obvious reason this shouldn't work with both full dumps and minidumps. At least, the gzip support doesn't have any such limitations.

Oh, I didn’t realize that. I thought it didn’t work. I don’t recall whether I tested it and it didn’t work or whether I just skipped it because I thought it wouldn’t work. I’ll try it out.

[Sorry for the delay in reviewing; I'm about to start flying back home, so I'll be able to comment further on Monday.]

No worries, this is all pending on Allan’s Zstd kernel patch landing.

In D13101#273976, @cem wrote:

In D13101#273868, @markj wrote:

Why do you disable debug.minidump in the "testing" section? I don't see any obvious reason this shouldn't work with both full dumps and minidumps. At least, the gzip support doesn't have any such limitations.

Oh, I didn’t realize that. I thought it didn’t work. I don’t recall whether I tested it and it didn’t work or whether I just skipped it because I thought it wouldn’t work. I’ll try it out.

I believe the compressed dump code in OneFS doesn't work with minidumps, but it's been a long time since I looked at it. The stuff I added to HEAD definitely works with minidumps and didn't require modifications to minidump_machdep.c for each arch.

In D13101#274122, @markj wrote:

I believe the compressed dump code in OneFS doesn't work with minidumps, but it's been a long time since I looked at it.

Oh, definitely. This isn't a port of the OneFS code, though.

The stuff I added to HEAD definitely works with minidumps and didn't require modifications to minidump_machdep.c for each arch.

Yep, I forgot about that. It seems like minidumps do work just fine.

Remove vestigial compress_user_cores_zstd
Add Zstd vmcore link / removal handling to savecore

Harbormaster completed remote builds in B12882: Diff 35486.Nov 20 2017, 3:53 AM

cem planned changes to this revision.Nov 20 2017, 10:09 PM

cem added inline comments.

sys/kern/kern_zstdio.c
93	We should enable the `checksumFlag` for parity with gzip.

Enable checksumFlag to enable compressed coredump checksums, for parity with gzip.

markj added inline comments.Dec 6 2017, 5:02 PM

sys/kern/kern_shutdown.c
195	There's a lot of duplication between the gzip and zstd support, and the gzip support alone makes the kernel dump code pretty ugly. Can we try and use a common interface to provide gzio/zstdio instead? That is, make a generic "compression stream" struct and API, and get rid of the compression-related ifdefs in kern_shutdown.c entirely. Compile-time options would still be used to enable gzip and/or zstd support, and the compression stream init function can just return an error if the caller requests a mode that isn't supported. This way, the ugly ifdeffery is hidden in the compression stream code instead of spilling out to each consumer. I'd be willing to rewrite the gzio bits along the lines of the paragraph above if you're willing to go down this route.
207	Looks like zstd's default compression level is 3 rather than 6, assuming you're copying the gzip default.

cem added inline comments.Dec 6 2017, 6:29 PM

sys/kern/kern_shutdown.c
195	That would be fine with me.
207	Yes, copying the gzip default. In my testing zstd at 6 is still faster than gzip at 6, so it's a reasonable analog to our gzip default.

markj added inline comments.Dec 23 2017, 4:33 PM

sys/kern/kern_shutdown.c
195	I have something working for gzip. Do you have a patch which would let me compile and link zstd into the kernel?

cem added inline comments.Dec 23 2017, 7:05 PM

sys/kern/kern_shutdown.c
195	https://reviews.freebsd.org/D10407 should be enough. My local branch has an earlier version of Allan's patch + build fixups needed to compile it. If this doesn't apply cleanly (or compile) on top of the latest version of Allan's patch, I can rebase it.

markj mentioned this in D13633: Add support for zstd to subr_compressor.c..Dec 26 2017, 3:11 AM

markj added inline comments.Dec 26 2017, 3:16 AM

sys/kern/kern_shutdown.c
195	Thanks. See D13632 and D13633.

Accepting so that this can be closed.

This revision is now accepted and ready to land.Feb 14 2018, 7:29 PM

Committed as r329240.

Add support for Zstd-compressed kernel and userspace dumps
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 35580

sbin/dumpon/dumpon.8

sbin/dumpon/dumpon.c

sbin/savecore/savecore.c

share/man/man5/core.5

sys/amd64/conf/GENERIC

sys/conf/NOTES

sys/conf/files

sys/conf/options

sys/kern/imgact_elf.c

sys/kern/kern_shutdown.c

sys/kern/kern_sig.c

sys/kern/kern_zstdio.c

sys/sys/conf.h

sys/sys/kerneldump.h

sys/sys/zstdio.h

Add support for Zstd-compressed kernel and userspace dumpsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 35580

sbin/dumpon/dumpon.8

sbin/dumpon/dumpon.c

sbin/savecore/savecore.c

share/man/man5/core.5

sys/amd64/conf/GENERIC

sys/conf/NOTES

sys/conf/files

sys/conf/options

sys/kern/imgact_elf.c

sys/kern/kern_shutdown.c

sys/kern/kern_sig.c

sys/kern/kern_zstdio.c

sys/sys/conf.h

sys/sys/kerneldump.h

sys/sys/zstdio.h

Add support for Zstd-compressed kernel and userspace dumps
ClosedPublic
Actions

Revision Contents
Changeset List