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
-
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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
