Page MenuHomeFreeBSD

Modify zstd so it can be built into the kernel as well
ClosedPublic

Authored by allanjude on Apr 15 2017, 9:38 PM.

Details

Summary

Change includes ifdef _KERNEL
Replace malloc/free with kernel version

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bapt added a subscriber: cem.May 6 2017, 3:44 PM
cem accepted this revision.May 6 2017, 3:58 PM
This revision is now accepted and ready to land.May 6 2017, 3:58 PM
allanjude updated this revision to Diff 30302.Jul 1 2017, 8:36 PM
allanjude edited edge metadata.

Reduce diff against zstd

No longer need to rename any macros

Change temporarily malloc()'s to user kernel malloc() instead

This revision now requires review to proceed.Jul 1 2017, 8:36 PM
allanjude updated this revision to Diff 33734.Oct 6 2017, 4:57 AM

Updated for newer version of zstd

Uses its own kernel memory type instead of M_TEMP

cem added a comment.Oct 6 2017, 7:23 PM

(The inter-version diffs are messed up due to the zstd version bump that happened during this review, unfortunately, but the full patch isn't too big.)

The current patch looks good to me, modulo all of the M_NOWAIT. I think allocations should generally be M_WAITOK unless we are truly in a sleep-free context; and I think it's unlikely compression should be done from sleep-free contexts anyway.

contrib/zstd/lib/common/fse_decompress.c
95 ↗(On Diff #33734)

Should this be WAITOK?

contrib/zstd/lib/common/zstd_kmalloc.c
33 ↗(On Diff #33734)

Maybe remove "temporary". They're just Zstd allocations in general.

allanjude edited the summary of this revision. (Show Details)Oct 7 2017, 3:17 AM
allanjude updated this revision to Diff 33787.Oct 7 2017, 3:18 AM
allanjude edited the summary of this revision. (Show Details)

Address Conrad's feedback re: M_NOWAIT

cem accepted this revision.Oct 7 2017, 3:20 AM
This revision is now accepted and ready to land.Oct 7 2017, 3:20 AM
allanjude updated this revision to Diff 33857.Oct 10 2017, 2:55 AM

Sparc fixes

Needs a better fix, since for gcc __has_builtin always returns false (a freebsd macro)

This revision now requires review to proceed.Oct 10 2017, 2:55 AM
allanjude updated this revision to Diff 33858.Oct 10 2017, 2:58 AM

Cleanup debugging cruft

cem added inline comments.Oct 10 2017, 3:10 AM
contrib/zstd/lib/common/fse_decompress.c
95 ↗(On Diff #33858)

Seems like debugging cruft got added here

allanjude updated this revision to Diff 35107.Nov 11 2017, 3:15 PM

Rebase to zstd 1.3.2

Apply fixes from cem

Still need to apply no-MT from upstream, and some compiler warning fixes from cem

bapt accepted this revision.Nov 11 2017, 3:22 PM
This revision is now accepted and ready to land.Nov 11 2017, 3:22 PM
imp requested changes to this revision.Nov 11 2017, 4:25 PM

I don't understand _BOOTSTRAP as an ifdef. We don't use that elsewhere. We use a couple of other things, but are trying to converge on _STAND
I don't understand why you aren't just creating a zstd-kernel.h file that has all the defined, including an appropriate malloc wrapper, and then sed'ing out the bad stuff and in the good stuff. libsa already does that for stand.h. See sys/boot/libsa/Makefile for details. Nothing in this diff seems to prelude that. You need to remove: <stdint.h> and replace <stddef.h>, <string.h>, <stdio.h> and <stdlib.h> I've suggested how to cope with malloc inline.

The builtin changes seem orthogonal, even if the first encounter with them is building inside a kernel. Those changes seem upstreamable, while the rest do not.

contrib/zstd/lib/common/bitstream.h
178 ↗(On Diff #35107)

related? seems like a separate patch that could easily be upstreamed.

contrib/zstd/lib/common/fse_decompress.c
40–42 ↗(On Diff #35107)

I wonder why not have a zstd_kernel.h and put all this stuff in there.

Then you could use the libsa trick of using sed to create the right sources and not have to put a bunch of ifdefs in?

44 ↗(On Diff #35107)

Why _BOOTSTRAP?

96 ↗(On Diff #35107)

If you create a malloc wrapper, you'd have fewer ifdefs.

contrib/zstd/lib/common/zstd_kmalloc.c
33 ↗(On Diff #35107)

#define malloc(x) (malloc)((x), M_ZSTD, M_WAITOK)
#define free(x) (free)((x), M_ZSTD)

would allow you to ditch the malloc/free ifdefs elsewhere.

sys/conf/files
629–641 ↗(On Diff #35107)

Reaches outside of sys. Not good.

sys/conf/kern.pre.mk
140 ↗(On Diff #35107)

This reaches outside of the kernel. That's considered bad since the kernel is supposed to be compilable on its own. Better to move contrib/zstd to sys/contrib/zstd and adjust userland Makefiles.

This revision now requires changes to proceed.Nov 11 2017, 4:25 PM
imp added inline comments.Nov 11 2017, 4:28 PM
contrib/zstd/lib/common/zstd_kmalloc.c
33 ↗(On Diff #35107)

Oh, and
#define calloc(a,b) (malloc)((a)*(b), M_ZSTD, M_WAITOK | M_ZERO)
which is save since a is always 1.

imp added a comment.Nov 11 2017, 4:31 PM

It also occurs to me that moving this to the boot loader will mean we'll likely need to have the same sed'ing I suggested for the kernel, so there's opportunity for reuse of that. I'd be happy to help with that since that's clearly the next stop for these patches.

cem added a comment.Nov 11 2017, 8:10 PM

I am on board with the idea of moving contrib/zstd to sys/contrib/zstd. (I think the kernel build already reaches outside of sys/ for some headers, if not .c files, though.) Maybe bde would complain about the abomination.

I would do that as a separate patch so that the SVN moves are tracked correctly.

cem added inline comments.Nov 11 2017, 8:24 PM
sys/conf/kern.pre.mk
140 ↗(On Diff #35107)

FYI, I find zstd doesn't compile w/ -Werror without adding -Wno-missing-prototypes or marking the relevant functions static.

cem added inline comments.Nov 11 2017, 8:59 PM
sys/conf/files
629 ↗(On Diff #35107)

FYI, this sources list was generated against the previous version of Zstd. It seems 1.3.2 adds some new files that must be included: http://dpaste.com/06K7G57

lib/compress/zstd_double_fast.c
lib/compress/zstd_fast.c
lib/compress/zstd_lazy.c
lib/compress/zstd_ldm.c
lib/compress/zstd_opt.c
imp added a comment.Nov 11 2017, 10:30 PM
In D10407#271258, @cem wrote:

I am on board with the idea of moving contrib/zstd to sys/contrib/zstd. (I think the kernel build already reaches outside of sys/ for some headers, if not .c files, though.) Maybe bde would complain about the abomination.

As far as I know, the only thing outside sys that the kernel build uses is header files from the compiler (not the userland source tree) for intrinsic intel aes instructions, but nothing else. And that's a compiler issue, not grabbing something form userland. If I've missed something, I'd be on board with fixing that asap. OK, there is a 'gdbinit' target that uses debug scripts from src/tools/debugscripts/gdbinit.* to generate a .gdbinit when you do an make install.debug...

bde would complain, but he's likely not the only one...

It is true that sys/boot reaches outside extensively (even before my hacking on it), but that's not the kernel and likely will be moving to src/stand or similar soon anyway...

I would do that as a separate patch so that the SVN moves are tracked correctly.

Agreed.

imp added a comment.Nov 11 2017, 10:43 PM

A more complete grep shows the kernel will also use ${S}/../COPYRIGHT if it exists, but won't if it doesn't. And that's only for a copyright notice inside a comment for vers.c. Otherwise, all other relative path things in the kernel and module builds land inside the kernel.
There is one oddball. In libtekken which lives in sys/tekken/ there is a reference libc, but it's a library that doesn't look to be built at all and looks to be userland code.

cem added a comment.Nov 11 2017, 10:47 PM
In D10407#271305, @imp wrote:

As far as I know, the only thing outside sys that the kernel build uses is header files from the compiler (not the userland source tree) for intrinsic intel aes instructions, but nothing else.

Yeah, that one was all I had in mind.

Thanks for the feedback Warner. I'll work on that stuff later today or tomorrow.

contrib/zstd/lib/common/bitstream.h
178 ↗(On Diff #35107)

This is required for this code to compile on sparc64.

I attempted to upstream this and then realized that GCC doesn't have __has_builtin(), we just get on on FreeBSD via a clang compatibility .h file we have (and it just always returns false).

sys/conf/files
629–641 ↗(On Diff #35107)

So you would prefer we svn mv contrib/zstd to sys/control/zstd ? We had discussed it, and everyone seems fine with that idea, I just didn't do it yet..

cem added a comment.EditedNov 13 2017, 7:07 PM

FYI, I turned on ZSTD assertions in DEBUG kernels with this patch:

diff --git a/contrib/zstd/lib/common/bitstream.h b/contrib/zstd/lib/common/bitstream.h
index e2de370..052dc6c 100644
--- a/contrib/zstd/lib/common/bitstream.h
+++ b/contrib/zstd/lib/common/bitstream.h
@@ -53,17 +53,21 @@ extern "C" {


 /*-*************************************
 *  Debug
 ***************************************/
+#ifdef _KERNEL
+#  define assert(condition)    KASSERT(condition, (#condition))
+#else
 #  if defined(BIT_DEBUG) && (BIT_DEBUG>=1)
 #    include <assert.h>
 #  else
 #    ifndef assert
 #      define assert(condition) ((void)0)
 #    endif
 #  endif
+#endif


 /*=========================================
 *  Target specific
 =========================================*/
diff --git a/contrib/zstd/lib/common/zstd_internal.h b/contrib/zstd/lib/common/zstd_internal.h
index 1431d68..36c8d82 100644
--- a/contrib/zstd/lib/common/zstd_internal.h
+++ b/contrib/zstd/lib/common/zstd_internal.h
@@ -42,17 +42,21 @@ extern "C" {


 /*-*************************************
 *  Debug
 ***************************************/
+#ifdef _KERNEL
+#  define assert(condition)    KASSERT(condition, (#condition))
+#else
 #  if defined(ZSTD_DEBUG) && (ZSTD_DEBUG>=1)
 #    include <assert.h>
 #  else
 #    ifndef assert
 #      define assert(condition) ((void)0)
 #    endif
 #  endif
+#endif

 #define ZSTD_STATIC_ASSERT(c) { enum { ZSTD_static_assert = 1/(int)(!!(c)) }; }

 #if defined(ZSTD_DEBUG) && (ZSTD_DEBUG>=2)
 #  include <stdio.h>
diff --git a/contrib/zstd/lib/compress/zstd_compress.c b/contrib/zstd/lib/compress/zstd_compress.c
index 3cf625a..89fcbcb 100644
--- a/contrib/zstd/lib/compress/zstd_compress.c
+++ b/contrib/zstd/lib/compress/zstd_compress.c
@@ -1338,11 +1338,11 @@ MEM_STATIC size_t ZSTD_buildCTable(void* dst, size_t dstCapacity,
             if (FSE_isError(NCountSize)) return NCountSize;
             CHECK_F(FSE_buildCTable_wksp(CTable, norm, max, tableLog, workspace, workspaceSize));
             return NCountSize;
         }
     }
-    default: return assert(0), ERROR(GENERIC);
+    default: assert(0); return ERROR(GENERIC);
     }
 }

 MEM_STATIC size_t ZSTD_encodeSequences(void* dst, size_t dstCapacity,
     FSE_CTable const* CTable_MatchLength, BYTE const* mlCodeTable,

(Whitespace changes due to indent elided.)

allanjude marked 14 inline comments as done.Dec 3 2017, 5:14 AM
allanjude updated this revision to Diff 36132.Dec 3 2017, 5:15 AM

Update to latest -current and lots of cleanup suggested by imp

cem accepted this revision.Dec 4 2017, 12:32 AM

LGTM modulo nits below.

sys/contrib/zstd/lib/common/zstd_kfreebsd.h
2 ↗(On Diff #36132)

You could throw in an SPDX identifier if you're feeling trendy.

32 ↗(On Diff #36132)

Usually we contract to the shorter #ifdef form if the conditional doesn't require the long form.

37–38 ↗(On Diff #36132)

style(9): (<sys/param.h> includes <sys/types.h>; do not include both.)

39–40 ↗(On Diff #36132)

whitespace looks funky here on Phabricator, but it might just be Phabricator.

sys/contrib/zstd/lib/common/zstd_kmalloc.c
30 ↗(On Diff #36132)

This should be removed

sys/contrib/zstd/lib/compress/zstd_compress.h
206 ↗(On Diff #36132)

Is the intent to submit these changes upstream eventually?

imp added a comment.Dec 4 2017, 1:27 AM

I think it might be even easier to do this right.

If you create stddef.h, stdlib.h, string.h, and stdio.h that just includes zstd_kfreebsd.h somewhere in sys/contrib/zstd, then you could add -I for that directory and ditch all the #ifdef_KERNEL changes. No fancy 'sed' is needed like I thought before. stand/libsa will be migrating to this way as well to facilitate the lua import in the coming weeks.

allanjude marked 2 inline comments as done.Dec 4 2017, 1:47 AM
allanjude added inline comments.
sys/contrib/zstd/lib/common/zstd_kmalloc.c
30 ↗(On Diff #36132)

yeah, I thought I fixed that already. I was testing if it was required, that include actually is required there.

sys/contrib/zstd/lib/compress/zstd_compress.h
206 ↗(On Diff #36132)

The versions of GCC that don't work, don't have has_builtin() either. It only works on FreeBSD because of a compat shim we created so we could use clang's has_builtin(). So for GCC is just always returns false. So when I tried to upstream it, it failed the CI testing, and I was confused at first.

allanjude marked 7 inline comments as done.Dec 4 2017, 2:21 AM
allanjude updated this revision to Diff 36171.Dec 4 2017, 2:21 AM

Switch to Warner's fake-files concept

cem accepted this revision.Dec 4 2017, 3:22 AM
imp accepted this revision.Dec 4 2017, 3:47 AM

love it. just one question.

sys/contrib/zstd/lib/freebsd/stddef.h
1 ↗(On Diff #36171)

are links like this allowed in the tree?

This revision is now accepted and ready to land.Dec 4 2017, 3:47 AM
cem added inline comments.Dec 4 2017, 3:56 AM
sys/contrib/zstd/lib/freebsd/stddef.h
1 ↗(On Diff #36171)

Not sure. The src tree currently has exactly one symlink in it already: contrib/tcpdump/README -> README.md

allanjude updated this revision to Diff 36172.Dec 4 2017, 4:51 AM

Switch from symlink to include files

This revision now requires review to proceed.Dec 4 2017, 4:51 AM
cem accepted this revision.Dec 4 2017, 4:54 AM
This revision is now accepted and ready to land.Dec 4 2017, 4:54 AM
imp added inline comments.Dec 4 2017, 2:21 PM
sys/contrib/zstd/lib/freebsd/stdint.h
1 ↗(On Diff #36172)

/* $FreeBSD$ */
may be needed.
/* This file is in the public domain */
is also needed
And the trailing newline will drive people nuts :)

cem added a comment.Dec 23 2017, 7:06 PM

Please go ahead and commit this, after fixing the easy issues brought up by Warner. :-)

cem added a comment.EditedJan 5 2018, 11:50 PM

Please commit this :-). If you don't have time to make Warner's changes and commit, I'm happy to assist.

Closed by commit rS327706: Integrate zstd into the kernel (authored by cem, committed by ). · Explain WhyJan 8 2018, 8:14 PM
This revision was automatically updated to reflect the committed changes.