inflate: route Z_BLOCK/Z_TREES streams to zlib - #68
Open
asonje wants to merge 2 commits into
Open
Conversation
inflate() ignored its flush argument on every offloaded path, and no backend ever wrote z_stream.data_type. A caller passing Z_BLOCK or Z_TREES got the whole stream instead of one block, with the bit accounting it asked for silently missing -- a correct prefix, but not the requested behavior. No backend can honor these values. QAT and IAA decompress whole streams in one submission with no notion of a block boundary, and ISA-L transits ISAL_BLOCK_NEW_HDR/ISAL_BLOCK_HDR inside a single isal_inflate() call with no way to stop there. zlib is the only implementation here that can honor both the early return and data_type, so pin such a stream to it. The pin is stream-wide rather than per call, matching the level-0 deflate and dictionary gates: the bit accounting spans the whole stream, so a later Z_NO_FLUSH call migrating the stream onto an accelerator would break it just as thoroughly. Pinning also reaches orig_inflate when use_zlib_uncompress=0 -- a request that was never offloadable is not a fallback. A stream already in flight on IGZIP is exempt, since ISA-L holds unflushed state that cannot be handed to zlib without corrupting the output; that residual is documented in the README, mirroring the compress-side Z_BLOCK exception. data_type is documented, not synthesized: no backend exposes the bit-level state to compute it faithfully, and a partially-correct value would be indistinguishable from a correct one to the caller. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
tests/zlib_accel_test.cpp:3449
- RunInflateSteps() uses EXPECT_EQ for inflateInit2(); if initialization fails, the helper continues and calls inflate()/inflateEnd() on an invalid stream. This should be ASSERT_EQ to avoid undefined behavior in the rest of the helper.
memset(&stream, 0, sizeof(stream));
EXPECT_EQ(inflateInit2(&stream, -15), Z_OK);
tests/zlib_accel_test.cpp:3528
- InflateFlushGateTest::SetUp/TearDown save and restore only the USE_* configs, but the helper methods called by these tests (SetCompressPath/SetUncompressPath) also modify other global configs like IAA_PREPEND_EMPTY_BLOCK and QAT_COMPRESSION_ALLOW_CHUNKING. Not restoring those can leak state into later tests and make the suite order-dependent.
void SetUp() override {
saved_use_zlib_uncompress_ = GetConfig(USE_ZLIB_UNCOMPRESS);
saved_use_iaa_uncompress_ = GetConfig(USE_IAA_UNCOMPRESS);
saved_use_qat_uncompress_ = GetConfig(USE_QAT_UNCOMPRESS);
saved_use_igzip_uncompress_ = GetConfig(USE_IGZIP_UNCOMPRESS);
saved_use_zlib_compress_ = GetConfig(USE_ZLIB_COMPRESS);
saved_use_iaa_compress_ = GetConfig(USE_IAA_COMPRESS);
saved_use_qat_compress_ = GetConfig(USE_QAT_COMPRESS);
saved_use_igzip_compress_ = GetConfig(USE_IGZIP_COMPRESS);
Comment on lines
+3411
to
+3412
| EXPECT_EQ(deflateInit2(&stream, 6, Z_DEFLATED, -15, 8, Z_DEFAULT_STRATEGY), | ||
| Z_OK); |
BuildMultiBlockRawDeflate() and RunInflateSteps() checked their deflateInit2()/inflateInit2() return with EXPECT_EQ and carried on, so a failed init left the rest of each helper operating on an uninitialized z_stream. Return early instead, with the empty result the callers already detect. InflateFlushGateTest saved and restored the eight USE_* configs but not IAA_PREPEND_EMPTY_BLOCK or QAT_COMPRESSION_ALLOW_CHUNKING, both of which SetCompressPath()/SetUncompressPath() write unconditionally. Restore them too, so the fixture cannot leak them into later tests. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
inflate() ignored its flush argument on every offloaded path, and no backend ever wrote z_stream.data_type. A caller passing Z_BLOCK or Z_TREES got the whole stream instead of one block, with the bit accounting it asked for silently missing -- a correct prefix, but not the requested behavior.
No backend can honor these values. QAT and IAA decompress whole streams in one submission with no notion of a block boundary, and ISA-L transits ISAL_BLOCK_NEW_HDR/ISAL_BLOCK_HDR inside a single isal_inflate() call with no way to stop there. zlib is the only implementation here that can honor both the early return and data_type, so pin such a stream to it.
The pin is stream-wide rather than per call, matching the level-0 deflate and dictionary gates: the bit accounting spans the whole stream, so a later Z_NO_FLUSH call migrating the stream onto an accelerator would break it just as thoroughly. Pinning also reaches orig_inflate when use_zlib_uncompress=0 -- a request that was never offloadable is not a fallback. A stream already in flight on IGZIP is exempt, since ISA-L holds unflushed state that cannot be handed to zlib without corrupting the output; that residual is documented in the README, mirroring the compress-side Z_BLOCK exception.
data_type is documented, not synthesized: no backend exposes the bit-level state to compute it faithfully, and a partially-correct value would be indistinguishable from a correct one to the caller.