Re: [PATCH] trace-cmd library: Add ZSTD support.

[Sebastian Andrzej Siewior <sebastian@breakpoint.cc>]

On 2022-02-21 08:08:51 [+0200], Tzvetomir Stoyanov wrote:
> On Sun, Feb 20, 2022 at 1:02 AM Sebastian Andrzej Siewior
> <sebastian@breakpoint.cc> wrote:
> >
> 
> Hi Sebastian,
Hi Tzvetomir,

> Thank you for contributing zstd support for trace-cmd! The zlib was
> chosen for the PoC implementation of the trace file compression
> support, as it is one of the widely available compression libraries.
> But as you have already seen, the design allows easily adding multiple
> compression algorithms. Support for a new compression library is less
> than 100 lines of code, that's why I suggest to keep the zlib support
> - even though the default will be zstd.

Oki.

> > The zstd support is using the context aware function so there is no need
> > to for the library to allocate one (including the memory) for every
> > invocation. This requires to be used in a single threaded environment or
> > the API needs to be extended to pass the context parameter.
> 
> Good point, the current design is according to zlib - that's why there
> is no context. But I like the idea to have a library specific context
> and this should be added now, before the first release.

cool.

> >
> > In most cases the input buffer was 40KiB so it does not make sense to
> > use higher compression levels. Higher compression levels won't
> > significantly improve the compression ration given that the every 40KiB
> > block is independent. However higher compression levels will slow down
> > the compression process.
> >
> 
> By default, the buffer is 10 system pages - so it could be larger on
> some systems.

Just a hint that if we increase the buffer size then the compression
would benefit from it ;)

> > diff --git a/lib/trace-cmd/include/trace-cmd-local.h b/lib/trace-cmd/include/trace-cmd-local.h
> > index 48f179d6f524a..8601a15f86f22 100644
> > --- a/lib/trace-cmd/include/trace-cmd-local.h
> > +++ b/lib/trace-cmd/include/trace-cmd-local.h
> > @@ -30,6 +30,10 @@ void tracecmd_info(const char *fmt, ...);
> >  int tracecmd_zlib_init(void);
> >  #endif
> >
> > +#ifdef HAVE_ZLIB
> 
> I think you mean HAVE_ZSTD here.

Yeah, fixed in v2, thanks.

> > diff --git a/lib/trace-cmd/trace-compress-zstd.c b/lib/trace-cmd/trace-compress-zstd.c
> > new file mode 100644
> > index 0000000000000..fc5e350f32509
> > --- /dev/null
> > +++ b/lib/trace-cmd/trace-compress-zstd.c
…
> > +int tracecmd_zstd_init(void)
> > +{
> > +       int ret = 0;
> > +       size_t r;
> > +
> > +       ctx_c = ZSTD_createCCtx();
> > +       ctx_d = ZSTD_createDCtx();
> > +       if (!ctx_c || !ctx_d)
> > +               goto err;
> > +
> > +       r = ZSTD_CCtx_setParameter(ctx_c, ZSTD_c_contentSizeFlag, 0);
> > +       if (ZSTD_isError(r))
> > +               goto err;
> > +
> > +       ret = tracecmd_compress_proto_register(__ZSTD_NAME,
> > +                                              ZSTD_versionString(),
> > +                                              __ZSTD_WEIGTH,
> > +                                              zstd_compress,
> > +                                              zstd_decompress,
> > +                                              zstd_compress_bound,
> > +                                              zstd_is_supported);
> 
> I think tracecmd_compress_proto_register() should be extend  with two new hooks:
>     void *(*new_context)(void),
>     void (*free_context)(void *),
> and ctx_c,  ctx_d should be allocated and freed there. Currently, that
> logic in trace-cmd is in a single thread. That may change in the
> future, and then the current design will be a limitation.

I would suggest to use

struct tracecmd_compress_algo {
	const char *name,
	const char *version,
	int weight,
	func_t *compress;
	func_t *decompress;
	func_t *bound;
	func_t *supported;
};

and then
	ret = tracecmd_compress_proto_register(&comp_algo);

so that the function has less args and can be extended without touching
every algo.

As for func_t / calling convetion I would prefer something that is not
that close to zlib. Like

	int comp(void *ctx, const void *src, signed int in_size,
		 void *dst, signed int out_size);

This clearly limits the sizes to 31 bit (with the 40kiB probably okay)
and the return value can be either >= 0 returning the number of bytes
produced or negative for an error. And please no errno touching ;)

Sebastian