lkml.org 
[lkml]   [2007]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC] LZO1X de/compression support
    Hi,

    Thanks for review. My comments inline.

    On 5/18/07, Heikki Orsila <shdl@zakalwe.fi> wrote:
    > Good work..
    >
    > On Fri, May 18, 2007 at 03:28:31PM +0530, Nitin Gupta wrote:
    > > Facts for LZO (at least for original code. Should hold true for this
    > > port also - hence the RFC!):
    > > - The compressor can never overrun buffer.
    > > - The "non-safe" version of decompressor can never overrun buffer if
    > > compressed data is unmodified. I am not sure about this if compressed
    > > data is malicious (to be confirmed from the author).
    > > - The "safe" version can never crash (buffer overrun etc.) - confirmed
    > > from the author.
    >
    > What's the proof?

    I confirmned these from the author - I just ported this code. I think
    he can answer you better - CC'ed him :-)

    >
    > > +/* LZO1X_1 compression */
    > > +int
    > > +lzo1x_compress(const unsigned char *src, size_t src_len,
    > > + unsigned char *dst, size_t *dst_len,
    > > + void *workmem);
    >
    > int lzo1x_compress(const unsigned char *src, size_t src_len,
    > unsigned char *dst, size_t *dst_len,
    > void *workmem);
    >
    > is the preferred style.
    >

    OK. Changed.

    <snip>
    > > + register const unsigned char *ip;
    >
    > Is the register directive really useful? Or any subsequent usage of that
    > directive?

    The author must be having some performance gain with this directive.
    Though I didn't test performance changes with/without this directive.

    > > + DINDEX1(dindex,ip);
    >
    > Put a space after the delimiter: DINDEX1(dindex, ip); This happens in
    > many places in the source, fix them all.

    OK.

    > Useless brackets: (unsigned char) tt
    >
    OK.

    > > + }
    > > + do *op++ = *ii++; while (--t > 0);
    >
    > memcpy(op, ii, t); ? Happens in other places as well.

    I looked more carefully into such cases. Following type of code blocks
    are repeated at several places:

    ---
    COPY4(op,ip);
    op += 4;
    ip += 4;
    if (--t > 0) {
    if (t >= 4) {
    do {
    COPY4(op,ip);
    op += 4; ip += 4; t -= 4;
    } while (t >= 4);
    if (t > 0)
    do
    *op++ = *ip++;
    while (--t > 0);
    } else
    do
    *op++ = *ip++;
    while (--t > 0);
    }
    ---
    Such entire blocks can be replaced by simple:
    memcpy(op, ip, t + 4);
    Since kernel has separate memcpy() implementation optimized for
    specific archs, we shouldn't loose on perf while having simpler (and
    shorter) code.

    I will work on this and post again.


    > > +#define COPY4(dst,src) *(uint32_t *)(dst) = *(uint32_t *)(src)
    >
    > Use u32.
    >

    What is the problem with uint32_t? Anyhow, I think COPY4 will
    disappear after those memcpy changes :)

    Thanks for comments. I will post revised patch soon.


    Cheers,
    Nitin
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-05-18 13:29    [W:0.025 / U:1.264 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site