On Mon, Apr 16, 2018 at 07:34:29PM +0000, Yann Collet wrote: > Hi Singh > > I don't have any strong opinion on this topic. > > You made your case clear: > your variant trades a little bit of speed for a little bit more compression ratio. > In the context of zram, it makes sense, and I would expect it to work, as advertised in your benchmark results. > (disclaimer: I haven't reproduced these results, just, they look reasonable to me, I have no reason to doubt them). > > So, the issue is less about performance, than about code complexity. > > As mentioned, this is an incompatible variant. > So, it requires its own entry point, and preferably its own code path > (even if it's heavily duplicated, > mixing it with regular lz4 source code, as proposed in the patch, will be bad for maintenance, > and can negatively impact regular lz4 usage, outside of zram). > > So that's basically the "cost" of adding this option. > > Is it worth it? > Well, this is completely outside of my responsibility area, so I really can't tell. > You'll have to convince people in charge that the gains are worth their complexity, > since _they_ will inherit the duty to keep the system working through its future evolutions. > At a minimum, you are targeting maintainers of zram and the crypto interface. > For this topic, they are the right people to talk to. > > > On 4/16/18, 04:09, "Maninder Singh" <maninder1.s@xxxxxxxxxxx> wrote: > > > Hello Nick/ Yann, > > Any inputs regarding LZ4 dyn results & lz4 dyn approach. > > >Hello Nick/Sergey, > > > >Any suggestion or comments, so that we can change code and resend the patch? > > > >> Hi Nick / Sergey, > >> > >> > >> We have compared LZ4 Dyn with Original LZ4 using some samples of realtime application data(4Kb) > >> compressed/decompressed by ZRAM. For comparison we have used lzbench (https://github.com/inikep/lzbench) > >> we have implemented dedicated LZ4 Dyn API & kept last literal length as 6 to avoid overhead > >> of checks. It seems in average case there is a saving of 3~4% in compression ratio with almost same compression > >> speed and minor loss in decompression speed (~50MB/s) when compared with LZ4. > >> > >> Comparison of Lz4 Dyn with LZO1x is also done as LZO1x is default compressor of ZRAM. > >> Unfortunately the track record of maintaining compression code in the Linux kernel is not great. zlib for example was forked from v1.2.3, which was released in 2005, and hasn't been updated since besides some random drive-by patches which have made it diverge even further from upstream. There have even been bugs assigned CVE numbers in upstream zlib, and I don't think anyone has looked at whether the Linux kernel version has those bugs or not. The story with LZ4 is a bit better as someone updated it to v1.7.3 last year. But, it took a lot of rounds of review in which I had to point out some subtle regressions like the hash table size being accidentally changed, and things not being inlined that should be. And of course now that version is outdated already. We also have LZO and Zstandard in the kernel to maintain too, as well as XZ decompression. And very problematically, *none* of these compression algorithms have a maintainer listed in the MAINTAINERS file. So in my opinion, as a prerequisite for this change, someone would need to volunteer to actually maintain LZ4 in the kernel. Thanks, Eric
