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

[allanjude]

Summary

Change includes ifdef _KERNEL
Replace malloc/free with kernel version

[allanjude]

Reduce diff against zstd

No longer need to rename any macros

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

[allanjude]

Updated for newer version of zstd

Uses its own kernel memory type instead of M_TEMP

[cem]

(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]

Address Conrad's feedback re: M_NOWAIT

[allanjude]

Sparc fixes

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

[allanjude]

Cleanup debugging cruft

[cem]

contrib/zstd/lib/common/fse_decompress.c

95 ↗

(On Diff #33858)

Seems like debugging cruft got added here

[allanjude]

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

[imp]

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

Reaches outside of sys. Not good.

sys/conf/kern.pre.mk

136

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.

[imp]

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]

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]

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]

sys/conf/kern.pre.mk

136

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

[cem]

sys/conf/files

629

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]

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]

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]

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.

[allanjude]

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

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]

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]

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

[cem]

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]

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]

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]

Switch to Warner's fake-files concept

[imp]

love it. just one question.

sys/contrib/zstd/lib/freebsd/stddef.h

2

are links like this allowed in the tree?

[cem]

sys/contrib/zstd/lib/freebsd/stddef.h

2

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

[allanjude]

Switch from symlink to include files

[imp]

sys/contrib/zstd/lib/freebsd/stdint.h

1

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

[cem]

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

[cem]

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