[PATCH -mm 0/5] LZO and swap write failure patches for -mm

[PATCH -mm 0/5] LZO and swap write failure patches for -mm

[Richard Purdie]

The following series contains several patches which I'm hoping could see
some testing in -mm. They're all been seen before at some point. The LZO
ones are important due to the dependent patches, the swap write failure
ones have just fallen off the radar.

LZO
===

We've seen a lot of activity in attempts to rewrite this in a
CodingStyle compatible 'clean' kernel style. The last update I read has
convinced me this is not ready for kernel usage at this time,
particularly due to the memory alignment issues.

I would like to see a version merged in the next merge window so other
patches depending on it can follow. If a better LZO implementation is
eventually sorted out, it can replace the lzo core in due course, the
API will be interchangeable. 

I've trimmed my patch down to only contain the "safe" decompression
function, pruned the headers a little further and merged in the cleanup
patch I submitted to -mm previously. I've also included an updated
version of the patch to make resier4 use the shared LZO functions
(fixing a security hole in the process).

Swap Write Failures
===================

Currently write failures to swap are handled badly and these patches
allow more graceful handling. 

These have been looked at before by various people and have no known
issues I'm aware of. I don't think Hugh has time to review these so if
anyone else familiar with the appropriate code area could look over
them, I'd appreciate it.

-- 
Richard

Re: [PATCH -mm 0/5] LZO and swap write failure patches for -mm

[Daniel Hazelton]

On Monday 04 June 2007 11:36:18 Richard Purdie wrote:
> The following series contains several patches which I'm hoping could see
> some testing in -mm. They're all been seen before at some point. The LZO
> ones are important due to the dependent patches, the swap write failure
> ones have just fallen off the radar.
>
> LZO
> ===
>
> We've seen a lot of activity in attempts to rewrite this in a
> CodingStyle compatible 'clean' kernel style. The last update I read has
> convinced me this is not ready for kernel usage at this time,
> particularly due to the memory alignment issues.

I have been involved in benchmarking and testing that stripped down and 
kernel-style version and cannot recall any mention of said alignment errors. 
Perhaps I was removed from the CC: list - could you point me at them?

At last glance the other version you mention has a marginally faster (1 to 
3%) "safe" decompressor. The only problems that Markus FXJ Oberhumer seems to 
have with that stripped down version is the renaming of the 
LZO1X_MEM_COMPRESS constant to LZO1X_WORKMEM_SIZE.  (only real complaint I 
can recall seeing).

If there *are* memory alignment issues, why not fix them in that tiny code 
rather than pushing a different version of the code into the kernel that is 
1) Not kernel style and 2) bloated?

> I would like to see a version merged in the next merge window so other
> patches depending on it can follow. If a better LZO implementation is
> eventually sorted out, it can replace the lzo core in due course, the
> API will be interchangeable.

I do agree that an implementation of LZO should go in the kernel so that the 
various users of the algorithm don't have to include their own copies of the 
code. However, the sheer bloatworthiness of this code is pitiful - perhaps 
it'd be best if you ditched the "diffability against userspace" in favor of 
having the code be non-bloated and kernel-style? (wait - that's what 
the "other" version you previously mentioned does)

> I've trimmed my patch down to only contain the "safe" decompression
> function, pruned the headers a little further and merged in the cleanup
> patch I submitted to -mm previously. I've also included an updated
> version of the patch to make resier4 use the shared LZO functions
> (fixing a security hole in the process).

Then submit the fix for the security hole as a seperate patch to the Rieser4 
people.

> Swap Write Failures
> ===================
>
> Currently write failures to swap are handled badly and these patches
> allow more graceful handling.
>
> These have been looked at before by various people and have no known
> issues I'm aware of. I don't think Hugh has time to review these so if
> anyone else familiar with the appropriate code area could look over
> them, I'd appreciate it.


Unless you can clearly point out those "memory alignment" issues in the 
kernel-style code of the other LZO implementation that was posted as an RFC I 
have to say this: stop the FUD. 

Anyway... It may mean absolutely nothing but...

NACK

DRH

Re: [PATCH -mm 0/5] LZO and swap write failure patches for -mm

[Richard Purdie]

On Mon, 2007-06-04 at 12:14 -0400, Daniel Hazelton wrote:
> On Monday 04 June 2007 11:36:18 Richard Purdie wrote:
> I have been involved in benchmarking and testing that stripped down and 
> kernel-style version and cannot recall any mention of said alignment errors. 
> Perhaps I was removed from the CC: list - could you point me at them?

I've forwarded you the full mail. To quote Nitin Gupta:

> The author (Markus Oberhumer) of LZO  provided these comments for this
> patch:
[...]
> I've only briefly looked over it, but it's obvious that your version
> does not work on architechtures which do not allow unaligned access
> (arm, mips, ...).
[...]
> Still a lot to do...

So its author admits it isn't ready yet. I'm not saying that code is bad
or shouldn't be included, just that we've ascertained it isn't ready
*yet*. Which brings us back to my last mail and its proposal.

> If there *are* memory alignment issues, why not fix them in that tiny 
> code rather than pushing a different version of the code into the 
> kernel that is 
> 1) Not kernel style and 2) bloated?

The zlib code isn't kernel style and is arguably bloated, perhaps we
should remove that?

If Nitin or others can fix that patch, fine but I can't afford the time
to do it and until those issues are fixed, it isn't ready.

Also, Nitin has already claimed several times that he's made no
functional changes to that code only to find that actually there are
functional changes. That does give rise to concern (not least for
security). There are ways to address this but I don't have time to do it
so it needs someone, preferably independent who has.

> > I've trimmed my patch down to only contain the "safe" decompression
> > function, pruned the headers a little further and merged in the cleanup
> > patch I submitted to -mm previously. I've also included an updated
> > version of the patch to make resier4 use the shared LZO functions
> > (fixing a security hole in the process).
> 
> Then submit the fix for the security hole as a seperate patch to the Rieser4 
> people.

I have done and they are aware of it.

> Unless you can clearly point out those "memory alignment" issues in
> the 
> kernel-style code of the other LZO implementation that was posted as
> an RFC I 
> have to say this: stop the FUD. 

This is not FUD, I've actually done things to help the other LZO
implementation forward but my available time to help is limited.

Note that I am not the only person to have expressed concerns with the
alternative LZO implementation and some questions raised by others as
well as myself regarding some of the code differences still remain
unanswered too.

Regards,

Richard

Re: [PATCH -mm 0/5] LZO and swap write failure patches for -mm

[Daniel Hazelton]

On Monday 04 June 2007 12:52:55 Richard Purdie wrote:
> On Mon, 2007-06-04 at 12:14 -0400, Daniel Hazelton wrote:
> > On Monday 04 June 2007 11:36:18 Richard Purdie wrote:
> > I have been involved in benchmarking and testing that stripped down and
> > kernel-style version and cannot recall any mention of said alignment
> > errors. Perhaps I was removed from the CC: list - could you point me at
> > them?
>
> I've forwarded you the full mail. To quote Nitin Gupta:
> > The author (Markus Oberhumer) of LZO  provided these comments for this
> > patch:
>
> [...]
>
> > I've only briefly looked over it, but it's obvious that your version
> > does not work on architechtures which do not allow unaligned access
> > (arm, mips, ...).
>
> [...]
>
> > Still a lot to do...
>
> So its author admits it isn't ready yet. I'm not saying that code is bad
> or shouldn't be included, just that we've ascertained it isn't ready
> *yet*. Which brings us back to my last mail and its proposal.

Yes - most of that work, IIRC, is related to the alignment issues that Herr 
Oberhumer noted. As it stands, the alternative does work well for a large 
number of the platforms that the kernel supports. With a little Kconfig magic 
it could be made available *only* for those platforms that it currently 
supports. Then people can help work on the alignment issues - possibly by 
providing platform conditional code.

> > If there *are* memory alignment issues, why not fix them in that tiny
> > code rather than pushing a different version of the code into the
> > kernel that is
> > 1) Not kernel style and 2) bloated?
>
> The zlib code isn't kernel style and is arguably bloated, perhaps we
> should remove that?

I'm not familiar with the zlib code, but it was included a long time ago - 
since zlib was included I'm pretty certain that if it had been proposed today 
it would have been NACK'd for the style violations and bloat.

> If Nitin or others can fix that patch, fine but I can't afford the time
> to do it and until those issues are fixed, it isn't ready.

You can take the time to produce a patch and spread FUD about the speed of a 
competing patches code but you don't have the time to work on fixing a 
cleaner implementation? I'll admit that actually working on fixing problems 
in code can take more time, but still - the time taken for those pursuits 
*could* have been spent actually working on fixing the problems.

> Also, Nitin has already claimed several times that he's made no
> functional changes to that code only to find that actually there are
> functional changes. That does give rise to concern (not least for
> security). There are ways to address this but I don't have time to do it
> so it needs someone, preferably independent who has.

Time and again? Only "Functional Changes" he made that negatively impacted the 
code was an attempt to replace an open-coded copy with a call to memcpy(). 
That it broke on PPC (and likely other big-endian architectures) is something 
I have been trying to figure out, since it should not have occurred. (Though 
it might just be that the kernel's highly optimized memcpy() somehow munged 
the byte-ordering)

And that *is* the only time I can remember someone mentioning that the code 
had broken - back around version 2 of his patch. Making it sound like this 
has happened numerous times certainly does qualify as something other than 
being honest, however you phrase it. As it stands you are the *first* and, 
IMHO, *only* person to have made a claim like this. From the comments that 
the original author of LZO has made I'd say he isn't concerned - after all, 
he's said: "As for further quality assurance, your version should generate 
byte-identical object code to LZO 2.02 when using the same compiler & flags."

The differences that *might* exist could possibly be tracked down to the use 
of a helper function and things like the likely()/unlikely() macros and the 
use of cpu_to_le16() (which should, instead be le16_to_cpu according to at 
least one commentor - I *think* it was Jan)

> > > I've trimmed my patch down to only contain the "safe" decompression
> > > function, pruned the headers a little further and merged in the cleanup
> > > patch I submitted to -mm previously. I've also included an updated
> > > version of the patch to make resier4 use the shared LZO functions
> > > (fixing a security hole in the process).
> >
> > Then submit the fix for the security hole as a seperate patch to the
> > Rieser4 people.
>
> I have done and they are aware of it.

Ah, okay.

>
> > Unless you can clearly point out those "memory alignment" issues in
> > the
> > kernel-style code of the other LZO implementation that was posted as
> > an RFC I
> > have to say this: stop the FUD.
>
> This is not FUD, I've actually done things to help the other LZO
> implementation forward but my available time to help is limited.

Call me paranoid, but even with MFXJ Oberhumer making a statement about the 
codes suitability for platforms that don't allow unaligned access I'm not 
convinced that it is a problem with the LZO implementation itself. After all, 
a processor really can't make it impossible to access single bytes without 
making life hard for a lot of people and almost all memory alignment issues 
processors have come back to how and/or where buffers are allocated. *IF* 
that is the case with Nitin's stripped down LZO code, then isn't it the job 
of the user of the library code to assure that the buffers are properly 
aligned?

> Note that I am not the only person to have expressed concerns with the
> alternative LZO implementation and some questions raised by others as
> well as myself regarding some of the code differences still remain
> unanswered too.

IMHO, Nitin has been great about addressing all questions raised. The 
differences in the generated code - at least here on my machine - all appear 
to be related to the macro's I defined to allow the kernel-level code to 
compile in userspace and Nitin's code's use of a helper-function to do a lot 
of the work. Other differences could be related to differences in how the 
code generator reacts to the lack of the "register" specifier on the 
variables (I can't really say - I don't have more than a passing familiarity 
with GCC's code generator)

Markus Oberhumer himself has stated that the output code for "cc -O2 -o 
minilzo.o minilzo.c" and "cc -O2 -o nitinlzo.o nitinlzo.c" should be almost 
exactly the same. What differences *might* exist can be tracked down to the 
functional changes that have been made - such as removal of the 
LZO_CHECK_MPOS_NON_DET macro, use of size_t/ssize_t/u32 in place of the 
renamed C99 types and a lot of other small things.

All told I wasn't surprised by the differences between miniLZO's object code 
and the object code for Nitin's stripped down "Tiny" version.

> Regards,
>
> Richard

DRH

Re: [PATCH -mm 0/5] LZO and swap write failure patches for -mm

[Nitin Gupta]

Hey Guys, please calm down :)

I now understand the memory alignment/other problems that author
pointed out and I am working on same - will post version 7 soon.

Thanks,
Nitin

Re: [PATCH -mm 0/5] LZO and swap write failure patches for -mm

[Richard Purdie]

On Mon, 2007-06-04 at 13:37 -0400, Daniel Hazelton wrote:
> Yes - most of that work, IIRC, is related to the alignment issues that Herr 
> Oberhumer noted. As it stands, the alternative does work well for a large 
> number of the platforms that the kernel supports. With a little Kconfig magic 
> it could be made available *only* for those platforms that it currently 
> supports. Then people can help work on the alignment issues - possibly by 
> providing platform conditional code.

My patch was actually written with ARM machines in mind and has been
extremely well tested on it. A version which doesn't run on ARM is not
acceptable. Its also ironic that "platform conditional code" is what a
lot of that bloat you're so keen to remove is about.

> I'm not familiar with the zlib code, but it was included a long time ago - 
> since zlib was included I'm pretty certain that if it had been proposed today 
> it would have been NACK'd for the style violations and bloat.

Adrian's covered this. I also know how hard updating something like zlib
is (I was the last person to do it).

> You can take the time to produce a patch and spread FUD about the speed of a 
> competing patches code but you don't have the time to work on fixing a 
> cleaner implementation? I'll admit that actually working on fixing problems 
> in code can take more time, but still - the time taken for those pursuits 
> *could* have been spent actually working on fixing the problems.

I *have* spent some time on it.

My speed comments were actually pretty positive. Yes, I screwed up one
of the benchmarks (as have others proving its easily done) but I did
admit to it. My others were fair comment and some issues were addressed
as a result (but not all).

I'm going to stop here. I don't agree with the rest of your email and
you've a distorted view of whats been said.

Regards,

Richard

Re: [PATCH -mm 0/5] LZO and swap write failure patches for -mm

[Daniel Hazelton]

On Monday 04 June 2007 16:45:55 Richard Purdie wrote:
> On Mon, 2007-06-04 at 13:37 -0400, Daniel Hazelton wrote:
> > Yes - most of that work, IIRC, is related to the alignment issues that
> > Herr Oberhumer noted. As it stands, the alternative does work well for a
> > large number of the platforms that the kernel supports. With a little
> > Kconfig magic it could be made available *only* for those platforms that
> > it currently supports. Then people can help work on the alignment issues
> > - possibly by providing platform conditional code.
>
> My patch was actually written with ARM machines in mind and has been
> extremely well tested on it. A version which doesn't run on ARM is not
> acceptable. Its also ironic that "platform conditional code" is what a
> lot of that bloat you're so keen to remove is about.

Done in a very poor manner.

> > I'm not familiar with the zlib code, but it was included a long time ago
> > - since zlib was included I'm pretty certain that if it had been proposed
> > today it would have been NACK'd for the style violations and bloat.
>
> Adrian's covered this. I also know how hard updating something like zlib
> is (I was the last person to do it).

I do agree. I have looked over the zlib code and it is very involved - I, 
personally, would not like to have to maintain it if I couldn't easily diff 
it against userspace.

> > You can take the time to produce a patch and spread FUD about the speed
> > of a competing patches code but you don't have the time to work on fixing
> > a cleaner implementation? I'll admit that actually working on fixing
> > problems in code can take more time, but still - the time taken for those
> > pursuits *could* have been spent actually working on fixing the problems.
>
> I *have* spent some time on it.
>
> My speed comments were actually pretty positive. Yes, I screwed up one
> of the benchmarks (as have others proving its easily done) but I did
> admit to it. My others were fair comment and some issues were addressed
> as a result (but not all).

I've looked back over the entire spread of the messages, both from before I 
wrote that quick&dirty benchmark and after. Maintainability is a good thing 
to aim for, however the style used to achieve the complete cross-platform 
mobility could be handled a lot cleaner - give me a few days to really study 
the LZO code and I'll see if I can't reach a middle-ground - code that is 
easy to maintain (and diff against userspace) as well as being stripped to 
its core and very cleanly implemented.

I can't promise results, but I figure I'm at fault for really starting this 
current spate of flames so I should at least take responsibility and do 
something to try and put them out. What I can say, at this point, is that a 
lot of my changes will be in making the code comply to kernel-style in a 
manner *similar* to how Nitin has done it - collapse redundant code together, 
replace open-coded blocks with calls to library functions, etc...


> I'm going to stop here. I don't agree with the rest of your email and
> you've a distorted view of whats been said.

Hindsight is sometimes too perfect. I should have returned to the early posts 
for clarity before making a lot of the comments I did. It shames me to admit 
that I've made such a nasty mistake and made myself seem like nothing more 
than a common troll.

Apologetically,
DRH

Re: [PATCH -mm 0/5] LZO and swap write failure patches for -mm

[Nitin Gupta]

On 6/4/07, Richard Purdie <rpurdie@openedhand.com> wrote:
> On Mon, 2007-06-04 at 12:14 -0400, Daniel Hazelton wrote:
> > On Monday 04 June 2007 11:36:18 Richard Purdie wrote:
> > I have been involved in benchmarking and testing that stripped down and
> > kernel-style version and cannot recall any mention of said alignment errors.
> > Perhaps I was removed from the CC: list - could you point me at them?
>
> I've forwarded you the full mail. To quote Nitin Gupta:
>
> > The author (Markus Oberhumer) of LZO  provided these comments for this
> > patch:
> [...]
> > I've only briefly looked over it, but it's obvious that your version
> > does not work on architechtures which do not allow unaligned access
> > (arm, mips, ...).
> [...]
> > Still a lot to do...
>
> So its author admits it isn't ready yet. I'm not saying that code is bad
> or shouldn't be included, just that we've ascertained it isn't ready
> *yet*. Which brings us back to my last mail and its proposal.
>


> > If there *are* memory alignment issues, why not fix them in that tiny
> > code rather than pushing a different version of the code into the
> > kernel that is
> > 1) Not kernel style and 2) bloated?
>

This is exactly what I have been saying.

> The zlib code isn't kernel style and is arguably bloated, perhaps we
> should remove that?

I don't know - I don't use zlib.
We can make LZO cleaner and perhaps faster. This will be good.

>
> If Nitin or others can fix that patch, fine but I can't afford the time
> to do it and until those issues are fixed, it isn't ready.
>

Yes. Many projects require LZO support and it might be getting
frustrating to delay  full-and-final code in kernel. I have been
extremely busy these days so I could not follow-up with authors
comments - also considering that some people want LZO patch split-up
into patch series which is highly cumbersome and time consuming in
this case.

I hoped that all of us can look into valuable feedback from author and
correct the issues he pointed out. This code duplication is really
pointless.

> Also, Nitin has already claimed several times that he's made no
> functional changes to that code only to find that actually there are
> functional changes. That does give rise to concern (not least for
> security). There are ways to address this but I don't have time to do it
> so it needs someone, preferably independent who has.
>

Yes there might still be problems - that is why I posted as RFC. I got
useful comments and the code is improving. Going for such fork might
be pain initially but IMHO its worth it. My idea for this 'fork' is
not just clean-ups but potential optimizations that such cleanups
usually bring along. I do not think there will be major overhauls in
such mature de/compression implementations so I believe its okay to go
for such 'fork' for sake of cleaner and perhaps faster code.

> > Unless you can clearly point out those "memory alignment" issues in
> > the
> > kernel-style code of the other LZO implementation that was posted as
> > an RFC I
> > have to say this: stop the FUD.
>
> This is not FUD, I've actually done things to help the other LZO
> implementation forward but my available time to help is limited.
>

I have identified those memory alignment issues and working on same.
Due to time constraints it might take me a bit longer.

> Note that I am not the only person to have expressed concerns with the
> alternative LZO implementation and some questions raised by others as
> well as myself regarding some of the code differences still remain
> unanswered too.

Again, I am working on improving the code as per feedback. This is
what I expect from RFC. Many people raised concerns regarding
bloatware in your patch too, including me.


Regards,
Nitin

Re: [PATCH -mm 0/5] LZO and swap write failure patches for -mm

[Adrian Bunk]

On Mon, Jun 04, 2007 at 11:56:46PM +0530, Nitin Gupta wrote:
> On 6/4/07, Richard Purdie <rpurdie@openedhand.com> wrote:
>...
>> The zlib code isn't kernel style and is arguably bloated, perhaps we
>> should remove that?
>
> I don't know - I don't use zlib.
> We can make LZO cleaner and perhaps faster. This will be good.
>...

"cleaner" = much harder to upgrade to new upstream LZO versions -> bad

"perhaps faster" = different from the well-known original code and
                   might again contain new bugs -> bad

"perhaps faster" = if we fork LZO and actually get it faster, all the 
                   other LZO users will not benefit -> bad


zlib and LZO are special because they are maintained userspace code 
imported into the kernel. 


> Regards,
> Nitin

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH -mm 0/5] LZO and swap write failure patches for -mm

[Nitin Gupta]

Andrew, Andrian,

If you really have the opinion of not going for major cleanups,
optimizations outside of original LZO code (basically a fork), then
there is no point in me continuing this work.

If you think otherwise, please let me know and I will post a newer
version with improvements from all these feedback I got.

Thanks,
Nitin


On 6/5/07, Adrian Bunk <bunk@stusta.de> wrote:
> On Mon, Jun 04, 2007 at 11:56:46PM +0530, Nitin Gupta wrote:
> > On 6/4/07, Richard Purdie <rpurdie@openedhand.com> wrote:
> >...
> >> The zlib code isn't kernel style and is arguably bloated, perhaps we
> >> should remove that?
> >
> > I don't know - I don't use zlib.
> > We can make LZO cleaner and perhaps faster. This will be good.
> >...
>
> "cleaner" = much harder to upgrade to new upstream LZO versions -> bad
>
> "perhaps faster" = different from the well-known original code and
>                    might again contain new bugs -> bad
>
> "perhaps faster" = if we fork LZO and actually get it faster, all the
>                    other LZO users will not benefit -> bad
>
>
> zlib and LZO are special because they are maintained userspace code
> imported into the kernel.
>
>
> > Regards,
> > Nitin
>
> cu
> Adrian
>
> --
>
>        "Is there not promise of rain?" Ling Tan asked suddenly out
>         of the darkness. There had been need of rain for many days.
>        "Only a promise," Lao Er said.
>                                        Pearl S. Buck - Dragon Seed
>
>

Re: [PATCH -mm 0/5] LZO and swap write failure patches for -mm

[Andrew Morton]

On Tue, 5 Jun 2007 11:00:05 +0530 "Nitin Gupta" <nitingupta910@gmail.com> wrote:

> Andrew, Andrian,
> 
> If you really have the opinion of not going for major cleanups,
> optimizations outside of original LZO code (basically a fork), then
> there is no point in me continuing this work.

err, my LZO attention span ran out 200 emails ago.

> If you think otherwise, please let me know and I will post a newer
> version with improvements from all these feedback I got.
> 

I'd say go with the cleanups.  The code I've seen is going to be quite
unmaintainable by any kernel developer.

Any fixes which come from upstream can be trivially applied by taking the
diffs against the version of upstream we started with and manually applying
them to our version.  If the diffs are too large and complex for that then
a) we shouldn't be merging the code in its current state anyway and b) with
the code as-is we couldn't effectively review or changelog those diffs, so
we shouldn't apply them.

So just fork it and freeze it.

(And please never top-post when working with kernel people).

Re: [PATCH -mm 0/5] LZO and swap write failure patches for -mm

[Richard Purdie]

On Mon, 2007-06-04 at 22:50 -0700, Andrew Morton wrote:
> I'd say go with the cleanups.  The code I've seen is going to be quite
> unmaintainable by any kernel developer.
> 
> Any fixes which come from upstream can be trivially applied by taking the
> diffs against the version of upstream we started with and manually applying
> them to our version.  If the diffs are too large and complex for that then
> a) we shouldn't be merging the code in its current state anyway and b) with
> the code as-is we couldn't effectively review or changelog those diffs, so
> we shouldn't apply them.
> 
> So just fork it and freeze it.

Ok, thanks for the decision.

Since I don't have too much faith in what Nitin has done with it, I'll
produce a version myself. In theory we should end up with identical code
so will be a sanity check of Nitin's code if nothing else.

Cheers,

Richard

Re: [PATCH -mm 0/5] LZO and swap write failure patches for -mm

[Richard Purdie]

On Mon, 2007-06-04 at 23:56 +0530, Nitin Gupta wrote:
> Yes there might still be problems - that is why I posted as RFC. I got
> useful comments and the code is improving. Going for such fork might
> be pain initially but IMHO its worth it. My idea for this 'fork' is
> not just clean-ups but potential optimizations that such cleanups
> usually bring along. I do not think there will be major overhauls in
> such mature de/compression implementations so I believe its okay to go
> for such 'fork' for sake of cleaner and perhaps faster code.

If you want to make cleaner and faster code, why not work on LZO
upstream directly? I'm sure the LZO author would welcome the speedups,
just as much as the kernel would. 

If they're accepted into LZO, I'm nearly certain the kernel will accept
them and if the kernel code is as I've proposed, upgrading is
straightforward too. Working with "upstream" is often harder but
ultimately much more rewarding.

> Again, I am working on improving the code as per feedback. This is
> what I expect from RFC. Many people raised concerns regarding
> bloatware in your patch too, including me.

And there are concerns against your patches including:

* endian issues
* alignment issues
* potential security holes
* unexplained code generation differences
* difficulty of upgrading

Compared to the style/bloat issue, I'd say that's a fair compromise.

Regards,

Richard

Re: [PATCH -mm 0/5] LZO and swap write failure patches for -mm

[Christoph Hellwig]

On Mon, Jun 04, 2007 at 09:58:51PM +0100, Richard Purdie wrote:
> On Mon, 2007-06-04 at 23:56 +0530, Nitin Gupta wrote:
> > Yes there might still be problems - that is why I posted as RFC. I got
> > useful comments and the code is improving. Going for such fork might
> > be pain initially but IMHO its worth it. My idea for this 'fork' is
> > not just clean-ups but potential optimizations that such cleanups
> > usually bring along. I do not think there will be major overhauls in
> > such mature de/compression implementations so I believe its okay to go
> > for such 'fork' for sake of cleaner and perhaps faster code.
> 
> If you want to make cleaner and faster code, why not work on LZO
> upstream directly? I'm sure the LZO author would welcome the speedups,
> just as much as the kernel would. 

Because it's author has shown his preference for crappy code.  Can we
please stop this stupid 'upstream' term here?  LZO is first and most
and algorithm.  There is a really crappy reference implementation, but
we should not put that in but rather have a proper implementation targeted
at the linux kernel.  Whether that implementation starts from scratch or
by gradually improving the existing reference implementation doesn't matter.