Re: [Suspend2][ 0/2] Cryptoapi deflate fix.

[Nigel Cunningham]

Hi.

On Tuesday 27 June 2006 16:00, Herbert Xu wrote:
> Nigel Cunningham <nigel@xxxxxxxxxxxx> wrote:
> > Sorry for the bad choice of heading. I have posted this before and
> > emailed it direct to Herbert, but he (rightly) doesn't see the need while
> > suspend2 isn't merged.
>
> Hmm, I don't recall ever telling you that.

Ok. Sorry for my wonky memory then :)

> I searched through my mail archive here is what I said to you last year
> on these two patches.  As far as I can see I don't have a reply from you
> on either subject.  So if you could reply to my questions below then I
> can consider your patches again.
>
> 1. Deflate fix:
> > > Hi Nigel, I need a bit more information about this patch.
> > > Do you have a specific input stream that requires a fix like
> > > this?
> >
> > Yes, Suspend2 if the user selects deflate as their compressor. The
> > output data will be PAGE_SIZE chunks, but deflate sometimes thinks it
> > has an extra byte to give us back.
>
> Are you saying that you're feeding it PAGE_SIZE chunks and it's
> giving you back something bigger than a page? That is expected
> since the nature of compression in general is that not everything
> is compressible.  Data streams which are not compressible will
> end up bigger than what you start with.
>
> What would be a bug is if you feed it something that's
> deflateBound^-1(PAGE_SIZE) bytes long and it spits back
> something that's longer than a page.

Yes, I'm always feeding it PAGE_SIZE chunks and compressing each page 
separately. It's a long time since I looked at or thought about this, so I'll 
spend some more time getting it fresh in my head if you like.

> > I agree that it's ugly and don't recall using it when I had gzip support
> > in an earlier version of Suspend2. Are you thinking there might be a
> > better way? If so, I can dig out the old (non crypto api) code.
>
> Well if you could give me a snippet of code that actually uses this
> stuff to compress pages then I might have a better idea of what it's
> trying to do.
>
> 2. LZF:
> > +static int lzf_compress_init(void *context)
> > +{
> > +     struct lzf_ctx *ctx = (struct lzf_ctx *)context;
> > +
> > +     /* Get LZF ready to go */
> > +     ctx->hbuf = vmalloc_32((1 << hlog) * sizeof(char *));
>
> Any reason why vmalloc can't be used?

I just left Marc's original code as it was, so I'm not completely sure, but I 
guess it's because we want lowmem.

> > +     /* Allocate local buffer */
> > +     ctx->local_buffer = (char *)get_zeroed_page(GFP_ATOMIC);
> >
> > +     /* Allocate page buffer */
> > +     ctx->page_buffer = (char *)get_zeroed_page(GFP_ATOMIC);
>
> Where are these two buffers used anyway?

Page_buffer is the destination buffer passed to the compressor. Local buffer 
is used to buffer the output for writing (and vv).

> > +     if (!ctx->page_buffer) {
> > +             free_page((unsigned long)ctx->local_buffer);
> > +             lzf_compress_exit(ctx);
>
> This is a double-free of local_buffer.

Is that from our previous correspondence? I can't find anything like it now.

Regards,

Nigel

> Cheers,

-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

Attachment: pgp00000.pgp
Description: PGP signature