[linux-kernel] Update patches for v2 #738

terrelln · 2017-06-27T02:57:52Z

Use div_u64 for division of u64s (2/4)
Reduce stack usage of FSE_buildCTable_wksp(), FSE_compress_wksp, and HUF_readDTableX4 (2/4)

terrelln · 2017-06-27T18:58:36Z

I'm going to reduce the stack usage of a few more functions.

terrelln · 2017-06-27T23:49:28Z

contrib/linux-kernel/lib/zstd/huf_compress.c

 	U32 tableLog = 0;
 	U32 nbSymbols = 0;
+	size_t readSize;
+
+	if ((HUF_SYMBOLVALUE_MAX + 1) * (sizeof(U32) + sizeof(BYTE)) > workspaceSize)


This is wrong, fixing it.

Cyan4973 · 2017-06-27T23:51:13Z

contrib/linux-kernel/lib/zstd/compress.c

-	U32 count[MaxSeq + 1];
-	S16 norm[MaxSeq + 1];
+	U32 *count = (U32 *)zc->tmpCounters;
+	S16 *norm = (S16 *)(count + MaxSeq + 1);


I would recommend to re-order space
to have U32 areas first, followed by U16, then BYTE.
It naturally solves alignment risks.

Since the next usage is inside of FSE_.*() functions, to make that work here I would have to put norm at the end of the workspace. Do you think its worth it?

I'll definitely update the other places though.

-Wcast-align is supposed to trigger a warning if one upgrade a U16 pointer into a U32 one,
even if you manually take good care of address alignment,
which is what happens below when norm get used as a base for workspace.

The order in memory can be different.
You can keep norm right after count if it's better,
but workspace will have to be based from count, because it is also a U32.

terrelln · 2017-06-27T23:54:12Z

contrib/linux-kernel/lib/zstd/huf_decompress.c

+	workspace = rankStart0 + HUF_TABLELOG_MAX + 2;
+	if ((BYTE *)workspace > wkspEnd)
+		return ERROR(tableLog_tooLarge);
+  workspaceSize = wkspEnd - (BYTE *)workspace;


Fixing formatting.

Cyan4973 · 2017-06-27T23:55:57Z

contrib/linux-kernel/lib/zstd/fse_decompress.c

+	size_t NCountLength;
+
+	workspace = counting + FSE_MAX_SYMBOL_VALUE + 1;
+	if ((BYTE *)workspace > wkspEnd)


In theory, such comparison is UB, because to be true, workspace must be outside of valid memory object.
Would it be possible to convert this test
into a size comparison ?
Something like if ( ((FSE_MAX_SYMBOL_VALUE+1)*sizeof(short)) > workspaceSize ) ?

Cyan4973 · 2017-06-27T23:56:42Z

contrib/linux-kernel/lib/zstd/huf_compress.c

-	U32 count[HUF_TABLELOG_MAX + 1];
-	S16 norm[HUF_TABLELOG_MAX + 1];
+	workspace = norm + HUF_TABLELOG_MAX + 1 + ((HUF_TABLELOG_MAX + 1) & 1);
+	if ((BYTE *)workspace > wkspEnd)


same comment here, about size comparison

Cyan4973 · 2017-06-27T23:57:27Z

contrib/linux-kernel/lib/zstd/huf_decompress.c

 	U32 tableLog = 0;
 	U32 nbSymbols = 0;
 	size_t iSize;
 	void *const dtPtr = DTable + 1;
 	HUF_DEltX2 *const dt = (HUF_DEltX2 *)dtPtr;

+	workspace = rankVal + HUF_TABLELOG_ABSOLUTEMAX + 1;
+	if ((BYTE *)workspace > wkspEnd)


and same here

Cyan4973 · 2017-06-27T23:57:38Z

contrib/linux-kernel/lib/zstd/huf_decompress.c

-	if ((BYTE *)(rankVal + HUF_TABLELOG_MAX) > wkspEnd)
+
+	workspace = rankStart0 + HUF_TABLELOG_MAX + 2;
+	if ((BYTE *)workspace > wkspEnd)


Cyan4973 · 2017-06-27T23:58:26Z

contrib/linux-kernel/test/Makefile

-CXXFLAGS += -std=c++11
-CFLAGS += -g -O0
+CXXFLAGS += -std=c++11 -g -O3
+CFLAGS += -g -O3 -Wframe-larger-than=400


why 400 ?
I expected linux kernel robot to test 1024 ?

Yeah it does, but the reviewer was concerned about stack usage, since the kernel stack is 16 KB at most. There were a few functions between 512 and 1024 bytes, so I fixed them up as well. After that, they were all below 400.

Cyan4973 · 2017-06-28T01:28:21Z

contrib/linux-kernel/lib/zstd/compress.c

-	S16 norm[MaxSeq + 1];
+	U32 *count = (U32 *)zc->tmpCounters;
+	S16 *norm = (S16 *)(count + MaxSeq + 1);
+	U32 *const workspace = PTR_ALIGN((U32 *)(norm + MaxSeq + 1), sizeof(U32));


I know this is supposed to work, since PTR_ALIGN makes a good job at properly aligning the resulting ptr,
but still, from a compiler perspective, promoting a U16* into a U32* is ground for a -Wcast-align warning :
https://godbolt.org/g/vy7fR3

Yeah, wrote this before I saw your second comment. I've fixed it so that it doesn't use the U16 * to derive the U32 *.

Cyan4973 · 2017-06-28T01:30:43Z

contrib/linux-kernel/lib/zstd/huf_decompress.c

 {
-	BYTE huffWeight[HUF_SYMBOLVALUE_MAX + 1];
-	U32 rankVal[HUF_TABLELOG_ABSOLUTEMAX + 1]; /* large enough for values from 0 to 16 */


minor : HUF_TABLELOG_ABSOLUTEMAX is quite small, it shouldn't worry stack size

Cyan4973 · 2017-06-28T01:31:43Z

contrib/linux-kernel/lib/zstd/huf_decompress.c

+	spaceUsed += sizeof(sortedSymbol_t) * (HUF_SYMBOLVALUE_MAX + 1);
+	weightList = (BYTE *)workspace + spaceUsed;
+	spaceUsed += HUF_SYMBOLVALUE_MAX + 1;
+	spaceUsed = ALIGN(spaceUsed, sizeof(U32));


Cyan4973 · 2017-06-28T01:32:28Z

contrib/linux-kernel/lib/zstd/huf_compress.c

 {
-	BYTE huffWeight[HUF_SYMBOLVALUE_MAX + 1];  /* init not required, even though some static analyzer may complain */
-	U32 rankVal[HUF_TABLELOG_ABSOLUTEMAX + 1]; /* large enough for values from 0 to 16 */


minor : HUF_TABLELOG_ABSOLUTEMAX is quite small, it shouldn't worry stack size

Yeah, but since we are already passing the workspace for huffWeight and HUF_readStats_wksp(), we might as well take advantage of it to reduce the stack a bit further.

terrelln · 2017-06-28T04:01:10Z

The patch is ready for review again. I've fixed the bugs in the stack computations that my fuzzers revealed. The fuzzers are still running.

Cyan4973 · 2017-06-28T23:44:02Z

contrib/linux-kernel/lib/zstd/compress.c

+		spaceUsed += sizeof(S16) * (MaxSeq + 1);
+		spaceUsed = ALIGN(spaceUsed, sizeof(U32));
+
+		workspace = (U32 *)((BYTE *)zc->tmpCounters + spaceUsed);


If a U32* ptr is created from a BYTE* ptr,
even if the initial source was a U32*,
it's still a concern for -Wcast-align.

See "doubleCast" example :
https://godbolt.org/g/qrWbLq

Solution : express spaceUsed in U32 increment.

Cyan4973 · 2017-06-28T23:47:22Z

There are still some alignment promotion issues in many places in the code.
I've only commented one of them, but the same concern and solution is applicable to the rest of the code.


        [linux-kernel] Update patches for v2

* Reduce stack usage of many zstd functions, none use over 388 B anymore. * Remove an incorrect `const` in `xxhash`.

facebook-github-bot added the CLA Signed label Jun 27, 2017

terrelln reviewed Jun 27, 2017

View changes

Cyan4973 reviewed Jun 27, 2017

View changes

terrelln reviewed Jun 27, 2017

View changes

Cyan4973 reviewed Jun 27, 2017

View changes

contrib/linux-kernel/lib/zstd/huf_decompress.c Outdated

if ((BYTE *)(rankVal + HUF_TABLELOG_MAX) > wkspEnd)

workspace = rankStart0 + HUF_TABLELOG_MAX + 2;

if ((BYTE *)workspace > wkspEnd)

This comment has been minimized.

Sign in to view

Cyan4973 Jun 27, 2017
Contributor

well...

Cyan4973 reviewed Jun 27, 2017

View changes

Cyan4973 reviewed Jun 28, 2017

View changes

terrelln force-pushed the terrelln:lk-div branch from 5b6b77f to 18683b9 Jun 28, 2017

Cyan4973 reviewed Jun 28, 2017

View changes

terrelln force-pushed the terrelln:lk-div branch from 18683b9 to ebbda16 Jun 29, 2017

[linux-kernel] Update patches for v2

Loading status checks…

ce91b64

* Reduce stack usage of many zstd functions, none use over 388 B anymore. * Remove an incorrect `const` in `xxhash`.

terrelln force-pushed the terrelln:lk-div branch from ebbda16 to ce91b64 Jun 29, 2017

facebook / zstd

[linux-kernel] Update patches for v2 #738

[linux-kernel] Update patches for v2 #738

terrelln commented Jun 27, 2017

terrelln commented Jun 27, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

terrelln commented Jun 28, 2017

This comment has been minimized.

Cyan4973 commented Jun 28, 2017

facebook / zstd

Join GitHub today

[linux-kernel] Update patches for v2 #738

[linux-kernel] Update patches for v2 #738

Conversation

terrelln commented Jun 27, 2017

terrelln commented Jun 27, 2017

This comment has been minimized.

This comment has been minimized.

Cyan4973 Jun 27, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

terrelln commented Jun 28, 2017

This comment has been minimized.

Cyan4973 commented Jun 28, 2017

Cyan4973 Jun 27, 2017 •

edited