Page MenuHomeFreeBSD

Add support for zstd to subr_compressor.c.
ClosedPublic

Authored by markj on Dec 26 2017, 3:11 AM.

Details

Summary

This is effectively a copy of kern_zstdio.c from D13101. The only
difference is that we always use a static context.

Test Plan

Kernel and user core dumping works. I will integrate the zstd
feature into my kernel dump test suite and verify that it plays
nicely with other kernel dump parameters (mini vs. full, dump
device sector size, etc.)

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 14186
Build 14352: arc lint + arc unit

Event Timeline

markj created this revision.Dec 26 2017, 3:11 AM
markj edited the test plan for this revision. (Show Details)Dec 26 2017, 3:14 AM
markj added reviewers: allanjude, cem, def.
markj updated this revision to Diff 37027.Dec 26 2017, 3:16 AM
  • Remove dead code.
markj updated this revision to Diff 37633.Jan 8 2018, 5:02 PM
  • Need to define COMPRESSOR_ZSTD
cem added a comment.Jan 8 2018, 8:41 PM

We're missing savecore support, manual page additions, as well as inclusion in GENERIC, right?

markj added a comment.Jan 8 2018, 8:42 PM
In D13633#289147, @cem wrote:

We're missing savecore support, manual page additions, as well as inclusion in GENERIC, right?

Yes, this is only the kernel code changes from your original patch. Sorry for not specifying that.

cem added a comment.Jan 8 2018, 8:52 PM

Yes, this is only the kernel code changes from your original patch. Sorry for not specifying that.

Ok. I like the approach, but don't think it makes sense to commit without the other pieces in place as well.

markj added a comment.Jan 8 2018, 8:56 PM
In D13633#289164, @cem wrote:

Yes, this is only the kernel code changes from your original patch. Sorry for not specifying that.

Ok. I like the approach, but don't think it makes sense to commit without the other pieces in place as well.

If by "other pieces" you mean D13632, I am going to commit them momentarily. :)

cem added a comment.Jan 8 2018, 9:03 PM

If by "other pieces" you mean D13632, I am going to commit them momentarily. :)

No, I meant the corresponding userspace and documentation changes. Though yes, this patch doesn't make sense without D13632 either :-).

markj added a comment.Jan 8 2018, 9:09 PM
In D13633#289172, @cem wrote:

If by "other pieces" you mean D13632, I am going to commit them momentarily. :)

No, I meant the corresponding userspace and documentation changes. Though yes, this patch doesn't make sense without D13632 either :-).

D'oh, of course. Feel free to commandeer the review and upload the other bits, or update your original review with this diff.

cem added inline comments.Jan 10 2018, 1:44 AM
sys/kern/subr_compressor.c
252

This necessitates using ZSTD_C (or otherwise hijacking the includes path) because zstd.h tries to include userland headers.

cem added inline comments.Jan 10 2018, 1:45 AM
sys/kern/subr_compressor.c
252

Hmmm. Doing that also hijacks malloc.

cem added inline comments.Jan 10 2018, 1:46 AM
sys/kern/subr_compressor.c
252

Ugly: it can be blocked with #define ZSTD_KFREEBSD_H.

cem added inline comments.Jan 10 2018, 1:54 AM
sys/kern/subr_compressor.c
252

Yeah. That compiles.

What do you think of adding GZIO and ZSTDIO to GENERIC on AMD64? Other platforms (aarch64)?

markj added inline comments.Jan 10 2018, 3:06 PM
sys/kern/subr_compressor.c
252

That would be ok with me, I would just do it as a separate commit. amd64 and arm64 seems like a reasonable starting point. How much bigger does the kernel binary get with those options configured?

As for enabling compressed user or kernel dumps by default, I'm not sure. It's probably the right configuration for a lot of systems. Note that compressing core dumps can consume a ton of CPU time; as a followup to this change we should look at adding a periodic sched_yield() or so somewhere in the write path. IMO this is especially important seeing as there's no way to abort a user core dump. I'd probably ask on -arch for opinions on compressing by default. One compromise might be to add installer support for configuring compressed dumps.

cem added a comment.Jan 28 2018, 9:28 AM

Ping — were you going to carry this forward?

markj added a comment.Jan 28 2018, 2:07 PM
In D13633#295769, @cem wrote:

Ping — were you going to carry this forward?

I assumed you were going to. If not I can pick it up this week.

cem added a comment.Jan 29 2018, 8:37 PM

I assumed you were going to. If not I can pick it up this week.

I'm having difficulty finding time to work on it, so I'd very much appreciate it if you picked it up. Thanks!

markj accepted this revision.Feb 14 2018, 7:38 PM

Accepting so that I can close this.

This revision is now accepted and ready to land.Feb 14 2018, 7:38 PM
markj closed this revision.Feb 14 2018, 7:38 PM

Committed as r329240.