On Thu, 24 May 2007 18:15:17 +0100
Michael-Luke Jones <[email protected]> wrote:
> Attached is a patch which may be desirable for -mm. It applies
> directly to 2.6.22-rc2-mm1.
>
> The patch removes the 'unsafe' LZO decompression function, lowering
> the size of the minilzo.c file by nearly 500 out of an original 1727
> lines. It also removes references to the 'unsafe' decompression
> function in the public LZO header and the EXPORT_SYMBOL_GPL declaration.
>
> This is intended to provoke some discussion over whether a
> decompression function able to scribble on arbitrary memory is
> desirable in the mainline kernel, whatever the performance increases.
>
> Over and above the security/stability implications of using this
> code, it can also be argued to represent an unnecessary duplication
> of the vast majority of LZO decompression code. This is due to the
> lack of likely in-kernel uses of the 'unsafe' function.
>
> Only a single user for this 'unsafe' code has been suggested, the
> 'Compressed Caching' project. This code is highly unlikely to move
> into mainline in the same timeframe as the LZO code. All of the other
> suggested uses require decompression of untrusted data, such that the
> 'safe' function should be used.
>
> Comments / disagreement all welcome :)
This is obviously a highly desirable thing to do for a number of reasons.
But have we quantified the performance difference?
On 24 May 2007, at 19:50, Andrew Morton wrote:
> On Thu, 24 May 2007 18:15:17 +0100
> Michael-Luke Jones <[email protected]> wrote:
>
>> Attached is a patch which may be desirable for -mm. It applies
>> directly to 2.6.22-rc2-mm1.
>>
>> The patch removes the 'unsafe' LZO decompression function, lowering
>> the size of the minilzo.c file by nearly 500 out of an original 1727
>> lines. It also removes references to the 'unsafe' decompression
>> function in the public LZO header and the EXPORT_SYMBOL_GPL
>> declaration.
>>
>> This is intended to provoke some discussion over whether a
>> decompression function able to scribble on arbitrary memory is
>> desirable in the mainline kernel, whatever the performance increases.
>>
>> Over and above the security/stability implications of using this
>> code, it can also be argued to represent an unnecessary duplication
>> of the vast majority of LZO decompression code. This is due to the
>> lack of likely in-kernel uses of the 'unsafe' function.
>>
>> Only a single user for this 'unsafe' code has been suggested, the
>> 'Compressed Caching' project. This code is highly unlikely to move
>> into mainline in the same timeframe as the LZO code. All of the other
>> suggested uses require decompression of untrusted data, such that the
>> 'safe' function should be used.
>>
>> Comments / disagreement all welcome :)
>
> This is obviously a highly desirable thing to do for a number of
> reasons.
> But have we quantified the performance difference?
The author of LZO may be able to help us out here, so he's CCed as of
this mail.
Michael-Luke
On Thu, 2007-05-24 at 11:50 -0700, Andrew Morton wrote:
> On Thu, 24 May 2007 18:15:17 +0100
> Michael-Luke Jones <[email protected]> wrote:
>
> > Attached is a patch which may be desirable for -mm. It applies
> > directly to 2.6.22-rc2-mm1.
> >
> > The patch removes the 'unsafe' LZO decompression function, lowering
> > the size of the minilzo.c file by nearly 500 out of an original 1727
> > lines. It also removes references to the 'unsafe' decompression
> > function in the public LZO header and the EXPORT_SYMBOL_GPL declaration.
[...]
> > Comments / disagreement all welcome :)
>
> This is obviously a highly desirable thing to do for a number of reasons.
> But have we quantified the performance difference?
Ok, I've done some tests:
1. Comparing the safe and unsafe functions
For my minilzo kernel patch, the safe version showed a 7.2% performance
hit. For Nitin's patch, it showed a 3.2% performance hit (but see
below).
Could be a lot worse and I don't object to the removal of the unsafe
version.
2. Comparing Nitin's code with my minilzo based kernel patch.
My kernel patch is about 2.25 times faster at decompression (16725Kb/ms
vs 7399Kb/ms) and fractionally faster at compression (1434kb/ms vs
1490kb/ms). As things stand the performance of Nitin's patch is
therefore unacceptable as that is a significant decompression
performance loss.
These tests are on 32 bit Intel and in userspace but I've found them to
be a pretty good indicator of what happens in the real world and on
other architectures.
For simplicity I made these tests with some existing code I had around
but its licence is such I can't share it, much to my frustration.
Cheers,
Richard
Richard Purdie wrote:
> On Thu, 2007-05-24 at 11:50 -0700, Andrew Morton wrote:
>> On Thu, 24 May 2007 18:15:17 +0100
>> Michael-Luke Jones <[email protected]> wrote:
>>
>>> Attached is a patch which may be desirable for -mm. It applies
>>> directly to 2.6.22-rc2-mm1.
>>>
>>> The patch removes the 'unsafe' LZO decompression function, lowering
>>> the size of the minilzo.c file by nearly 500 out of an original 1727
>>> lines. It also removes references to the 'unsafe' decompression
>>> function in the public LZO header and the EXPORT_SYMBOL_GPL declaration.
> [...]
>>> Comments / disagreement all welcome :)
>> This is obviously a highly desirable thing to do for a number of reasons.
>> But have we quantified the performance difference?
>
> Ok, I've done some tests:
>
> 1. Comparing the safe and unsafe functions
>
> For my minilzo kernel patch, the safe version showed a 7.2% performance
> hit. For Nitin's patch, it showed a 3.2% performance hit (but see
> below).
>
> Could be a lot worse and I don't object to the removal of the unsafe
> version.
>
> 2. Comparing Nitin's code with my minilzo based kernel patch.
>
> My kernel patch is about 2.25 times faster at decompression (16725Kb/ms
> vs 7399Kb/ms) and fractionally faster at compression (1434kb/ms vs
> 1490kb/ms). As things stand the performance of Nitin's patch is
> therefore unacceptable as that is a significant decompression
> performance loss.
Please do _not_ rewrite the LZO implementation just for coding style principles.
The current miniLZO implementation is _extrememly_ well tested, pretty
optimized and quite portable.
I agree that the implementation may look confusing, but you should be able to
make it look much better by removing all the unused #defines and #ifdef code
paths - LZO supports exotic things like 16-bit DOS and CRAY PVP memory models
which obviously are not needed in the kernel and account for quite a number of
abstractions (which are implemented through the preprocessor).
Finally the current version has been tested with a lot of compilers and
contains accumulated knowledge about some hairy things - see
http://gcc.gnu.org/PR25196 for an example, as well as some not-yet identified
aliasing issue.
~Markus
> These tests are on 32 bit Intel and in userspace but I've found them to
> be a pretty good indicator of what happens in the real world and on
> other architectures.
> For simplicity I made these tests with some existing code I had around
> but its licence is such I can't share it, much to my frustration.
>
> Cheers,
>
> Richard
>
--
Markus Oberhumer, <[email protected]>, http://www.oberhumer.com/
Hi Richard,
Thanks for these perf. figures!
On 5/25/07, Richard Purdie <[email protected]> wrote:
> On Thu, 2007-05-24 at 11:50 -0700, Andrew Morton wrote:
> > On Thu, 24 May 2007 18:15:17 +0100
> > Michael-Luke Jones <[email protected]> wrote:
> >
> > > Attached is a patch which may be desirable for -mm. It applies
> > > directly to 2.6.22-rc2-mm1.
> > >
> > > The patch removes the 'unsafe' LZO decompression function, lowering
> > > the size of the minilzo.c file by nearly 500 out of an original 1727
> > > lines. It also removes references to the 'unsafe' decompression
> > > function in the public LZO header and the EXPORT_SYMBOL_GPL declaration.
> [...]
> > > Comments / disagreement all welcome :)
> >
> > This is obviously a highly desirable thing to do for a number of reasons.
> > But have we quantified the performance difference?
>
> Ok, I've done some tests:
>
> 1. Comparing the safe and unsafe functions
>
> For my minilzo kernel patch, the safe version showed a 7.2% performance
> hit. For Nitin's patch, it showed a 3.2% performance hit (but see
> below).
>
> Could be a lot worse and I don't object to the removal of the unsafe
> version.
>
Ok. Let's drop unsafe version. This will also do away will that Makefile debris.
> 2. Comparing Nitin's code with my minilzo based kernel patch.
>
> My kernel patch is about 2.25 times faster at decompression (16725Kb/ms
> vs 7399Kb/ms) and fractionally faster at compression (1434kb/ms vs
> 1490kb/ms). As things stand the performance of Nitin's patch is
> therefore unacceptable as that is a significant decompression
> performance loss.
>
I suspect this is mainly due to replacement of COPY4() and open coded
byte-by-byte copying with memcpy() calls. I will rollback these
particular changes, remove unsafe version and will see again. I have
retained all other code as-is.
I will also try adding benchmarking code to my (GPL) 'compress-test'
module (same which I sent to Bret). Though it's pretty basic module
but should be sufficient to give required perf. figures.
> These tests are on 32 bit Intel and in userspace but I've found them to
> be a pretty good indicator of what happens in the real world and on
> other architectures.
> For simplicity I made these tests with some existing code I had around
> but its licence is such I can't share it, much to my frustration.
>
Cheers,
Nitin
Hi Markus,
On 5/25/07, Markus F.X.J. Oberhumer <[email protected]> wrote:
> Please do _not_ rewrite the LZO implementation just for coding style principles.
>
> The current miniLZO implementation is _extrememly_ well tested, pretty
> optimized and quite portable.
>
> I agree that the implementation may look confusing, but you should be able to
> make it look much better by removing all the unused #defines and #ifdef code
> paths - LZO supports exotic things like 16-bit DOS and CRAY PVP memory models
> which obviously are not needed in the kernel and account for quite a number of
> abstractions (which are implemented through the preprocessor).
>
> Finally the current version has been tested with a lot of compilers and
> contains accumulated knowledge about some hairy things - see
> http://gcc.gnu.org/PR25196 for an example, as well as some not-yet identified
> aliasing issue.
>
> ~Markus
I did not rewrite any part of your code except replacing COPY4() macro
and some open-coded byte-by-byte copying with memcpy(). But this has
resulted in very significant perf. loss (as suggested by results from
Richard's tests) - so will rollback these changes.
Additionally following was done to _greatly_ reduce no. of LOC I had
to retain. These should not affect code correctness and performance:
- Used standard/kernel defined data types equivalent of lzo_* types.
This resulted in removal of huge chunks of #ifdefs:
lzo_byptep -> unsigned char *
lzo_uint -> size_t
lzo_xint -> size_t
lzo_uintptr_t -> unsigned long
lzo_uint32p -> uint32_t *
- Removed everything #ifdefed under COPY_DICT -- from minilzo code I
see that this is not #defined for LZO1X (safe/unsafe) (though I could
not understand meaning behind COPY_DICT).
- Removed everthing #ifdef'ed for LZO1Y, LZO1Z, other variants.
Thanks,
Nitin
On Thu, 24 May 2007 18:15:17 +0100
Michael-Luke Jones <[email protected]> wrote:
> The patch removes the 'unsafe' LZO decompression function
Guys, I'll drop all the lzo patches from -mm. Please wake me up when
we've decided what we want to do.