Re: [RFC] [-mm] Remove 'unsafe' LZO decompressor
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] [-mm] Remove 'unsafe' LZO decompressor
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] [-mm] Remove 'unsafe' LZO decompressor
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] [-mm] Remove 'unsafe' LZO decompressor
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] [-mm] Remove 'unsafe' LZO decompressor
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] [-mm] Remove 'unsafe' LZO decompressor
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] [-mm] Remove 'unsafe' LZO decompressor
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] [-mm] Remove 'unsafe' LZO decompressor
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] [-mm] Remove 'unsafe' LZO decompressor
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? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC] [-mm] Remove 'unsafe' LZO decompressor
Hi there, 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 :) Michael-Luke Jones lzo-remove-unsafe-decompressor.patch Description: Binary data
[RFC] [-mm] Remove 'unsafe' LZO decompressor
Hi there, 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 :) Michael-Luke Jones lzo-remove-unsafe-decompressor.patch Description: Binary data
Re: [RFC] [-mm] Remove 'unsafe' LZO decompressor
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? - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] [-mm] Remove 'unsafe' LZO decompressor
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] [-mm] Remove 'unsafe' LZO decompressor
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
