In the following,
- is Nitin's patch
+ is my version.
+ for (;;) {
- DINDEX1(dindex, ip);
+ dindex = DMS((0x21 * DX3(ip,5,5,6)) >> 5, 0);
m_pos = dict[dindex];
Probably makes sense to expand the define since its single use.
- if ((m_pos < in) || (m_off = (size_t)(ip - m_pos)) <= 0
- || m_off > M4_MAX_OFFSET)
+ if (m_pos < in)
+ goto literal;
+
+ m_off = ip - m_pos;
+ if (m_off == 0 || m_off > M4_MAX_OFFSET)
goto literal;
This can be expanded to be more obvious. Need to be careful about
signage, the <= becomes a == but we can then lose the cast.
- op[-2] |= (unsigned char)t;
+ op[-2] |= t;
The unsigned char casts are all unneeded.
I'll skip future cases of these issues but there are more.
@@ -203,61 +165,60 @@
break;
}
- *out_len = (size_t)(op - out);
- return (size_t)(in_end - ii);
+ *out_len = op - out;
+ return in_end - ii;
unneeded casts.
- if (!workmem)
- return -EINVAL;
Could be confused with LZO's own error codes. Do we need this?
+#define HAVE_IP_OR(x, ip_end, ip) ((ip_end - ip) < (size_t)(x))
+#define HAVE_OP_OR(x, op_end, op) ((op_end - op) < (size_t)(x))
+#define HAVE_LB_OR(m_pos, out, op) (m_pos < out || m_pos >= op)
Your NEED_* defines affect flow control contra to CodingStyle, hence my
choice of these instead and the associated code changes which I'll skip
over.
-MODULE_LICENSE("GPL");
-MODULE_DESCRIPTION("LZO1X Decompression");
+#if defined(LZO_UNALIGNED_OK_4)
+#define COPY4(dst,src) *(u32 *)(dst) = *(const u32 *)(src)
+#endif
Only the decompressor uses COPY4 so I moved it to this file.
- while (TEST_IP) {
+ while ((ip < ip_end)) {
Might as well expand this...
- /* copy literals */
- NEED_OP(t + 3);
- NEED_IP(t + 4);
-#ifndef UNALIGNED_OK
- if (((ip | op ) & 3) == 0) {
-#endif
+ if (HAVE_OP_OR(t + 3, op_end, op))
+ goto output_overrun;
+ if (HAVE_IP_OR(t + 4, ip_end, ip))
+ goto input_overrun;
+
+#if defined(LZO_UNALIGNED_OK_4)
COPY4(op, ip);
op += 4;
ip += 4;
We have a fairly major difference in the code here. This is where you've
assumed LZO_ALIGNED_OK_4 was set and it wasn't set in any in LZO in any
of my tests. Assuming LZO_ALIGNED_OK_4 should be safe for the kernel and
the code path you've added probably performs better but why was it
disabled in minilzo?
-#ifdef UNALIGNED_OK
+#if defined(LZO_UNALIGNED_OK_4)
if (t >= 2 * 4 - (3 - 1) && (op - m_pos) >= 4) {
-#else
- if (t >= 2 * 4 - (3 - 1) &&
- (((op | m_pos) & 3) == 0)) {
-#endif
Same again here.
@@ -229,42 +230,45 @@
t = *ip++;
- } while (TEST_IP);
+ } while (ip < ip_end);
Might as well expand this.
diff -uwr 1/lzodefs.h 2/lzodefs.h
--- 1/lzodefs.h 2007-06-07 09:33:34.000000000 +0100
+++ 2/lzodefs.h 2007-06-06 17:40:56.000000000 +0100
-#ifndef __LZO1X_INT_H
-#define __LZO1X_INT_H
Pointless since only the LZO code will include this and it will do it
once.
-#include <linux/types.h>
Uneeded?
+#define LZO_VERSION 0x2020
+#define LZO_VERSION_STRING "2.02"
+#define LZO_VERSION_DATE "Oct 17 2005"
A good idea to leave these so the exact version this was created from is
documented.
+#define DMS(v,s) ((size_t) (((v) & (D_MASK >> (s))) << (s)))
You've merged this into DINDEX2. Since s is 0, that probably makes sense
(we can both lose the cast too).
+/* Which machines to allow unaligned accesses on */
+#if defined(CONFIG_X86_32) || defined(CONFIG_X86_64)
+#define LZO_UNALIGNED_OK_2
+#define LZO_UNALIGNED_OK_4
#endif
Probably preferable to do this here using CONFIG options rather than in
the Makefile.
I still need to do some further tests to ascertain whether
we can use get/put_unaligned without affecting performance instead.
So in summary apart from style issues, the code is the same apart from
the alignment access issues.
http://folks.o-hand.com/richard/lzo/lzo_kernel-r5.patch is a version
I've updated as I went through this which should combine the good bits
from both *apart* from the alignment issues which I want to look at more
carefully before taking an approach.