[PATCH] v3: print current and wanted LZMA scratchpad size

[Carl-Daniel Hailfinger]

Print current and wanted LZMA scratchpad size in the decompression
routine. That allows people to either adjust compression parameters
or scratchpad size.
Having a similar check during build time would be nice.

Signed-off-by: Carl-Daniel Hailfinger

Index: LinuxBIOSv3-tmp/lib/lzma.c
===================================================================
--- LinuxBIOSv3-tmp/lib/lzma.c	(Revision 682)
+++ LinuxBIOSv3-tmp/lib/lzma.c	(Arbeitskopie)
@@ -14,6 +14,7 @@
 #include "string.h"
 #include "console.h"

+#define LZMA_SCRATCHPAD_SIZE 15980

unsigned long ulzma(u8 *src, u8 *dst)
 {
@@ -24,7 +25,7 @@
    int res;
    CLzmaDecoderState state;
    SizeT mallocneeds;
-	unsigned char scratchpad[15980];
+	unsigned char scratchpad[LZMA_SCRATCHPAD_SIZE];

memcpy(properties, src, LZMA_PROPERTIES_SIZE);
    outSize = *(UInt32 *)(src + LZMA_PROPERTIES_SIZE);
@@ -32,8 +33,9 @@
    	printk(BIOS_WARNING, "Incorrect stream properties\n");
    }
    mallocneeds = (LzmaGetNumProbs(&state.Properties) * sizeof(CProb));
-	if (mallocneeds > 15980) {
-		printk(BIOS_WARNING, "Decoder scratchpad too small!\n");
+	if (mallocneeds > LZMA_SCRATCHPAD_SIZE) {
+		printk(BIOS_WARNING,("Decoder scratchpad too small, have %i, need %i!\n",
+				LZMA_SCRATCHPAD_SIZE, mallocneeds);
    }
    state.Probs = (CProb *)scratchpad;
    res = LzmaDecode(&state, src + LZMA_PROPERTIES_SIZE + 8, (SizeT)0xffffffff, &inProcessed,

[PATCH] v3: print current and wanted LZMA scratchpad size

[Myles Watson]

On Fri, May 23, 2008 at 1:22 PM, Carl-Daniel Hailfinger
 wrote:
> Print current and wanted LZMA scratchpad size in the decompression
> routine. That allows people to either adjust compression parameters
> or scratchpad size.
> Having a similar check during build time would be nice.
>
> Signed-off-by: Carl-Daniel Hailfinger
> Index: LinuxBIOSv3-tmp/lib/lzma.c
>
> --- LinuxBIOSv3-tmp/lib/lzma.c  (Revision 682)
> +++ LinuxBIOSv3-tmp/lib/lzma.c  (Arbeitskopie)
> @@ -14,6 +14,7 @@
>  #include "string.h"
>  #include "console.h"
>
> +#define LZMA_SCRATCHPAD_SIZE 15980

What's the reason for 15980?  What's the downside to increasing it further?

Thanks,
Myles
> unsigned long ulzma(u8 *src, u8 *dst)
>  {
> @@ -24,7 +25,7 @@
>        int res;
>        CLzmaDecoderState state;
>        SizeT mallocneeds;
>   unsigned char scratchpad[15980];
>   unsigned char scratchpad[LZMA_SCRATCHPAD_SIZE];
>
>  memcpy(properties, src, LZMA_PROPERTIES_SIZE);
>  outSize = *(UInt32 *)(src + LZMA_PROPERTIES_SIZE);
>
> @@ -32,8 +33,9 @@
>                printk(BIOS_WARNING, "Incorrect stream properties\n");
>        }
>        mallocneeds = (LzmaGetNumProbs(&state.Properties) * sizeof(CProb));
>   if (mallocneeds > 15980) {
>           printk(BIOS_WARNING, "Decoder scratchpad too small!\n");
>   if (mallocneeds > LZMA_SCRATCHPAD_SIZE) {
>           printk(BIOS_WARNING,("Decoder scratchpad too small, have %i, need %i!\n",
>                           LZMA_SCRATCHPAD_SIZE, mallocneeds);
>  }
>  state.Probs = (CProb *)scratchpad;
>  res = LzmaDecode(&state, src + LZMA_PROPERTIES_SIZE + 8, (SizeT)0xffffffff, &inProcessed,

[Carl-Daniel Hailfinger]

On 23.05.2008 21:25, Myles Watson wrote:
> On Fri, May 23, 2008 at 1:22 PM, Carl-Daniel Hailfinger
>  wrote:
> > Print current and wanted LZMA scratchpad size in the decompression
> > routine. That allows people to either adjust compression parameters
> > or scratchpad size.
> > Having a similar check during build time would be nice.
> >
> > Signed-off-by: Carl-Daniel Hailfinger
> > Index: LinuxBIOSv3-tmp/lib/lzma.c
> >
> > --- LinuxBIOSv3-tmp/lib/lzma.c  (Revision 682)
> > +++ LinuxBIOSv3-tmp/lib/lzma.c  (Arbeitskopie)
> > @@ -14,6 +14,7 @@
> >  #include "string.h"
> >  #include "console.h"
> >
> > +#define LZMA_SCRATCHPAD_SIZE 15980
>
> What's the reason for 15980?  What's the downside to increasing it further?

It's the value I picked during OLPC early bringup, based on compression
efficiency measurements correlated with stack requirements. It seemed to
be the sweet spot on Geode back then. Besides that, v3 relies on the
stack not to outgrow 32k IIRC, otherwise stack and printk buffer will
corrupt each other.

Right now the patch is a no-op except for the better error message.

Regards,
Carl-Daniel

[PATCH] v3: print current and wanted LZMA scratchpad size

[Myles Watson]

On Fri, May 23, 2008 at 1:32 PM, Carl-Daniel Hailfinger
 wrote:
> On 23.05.2008 21:25, Myles Watson wrote:
> > On Fri, May 23, 2008 at 1:22 PM, Carl-Daniel Hailfinger
> >  wrote:
> > > Print current and wanted LZMA scratchpad size in the decompression
> > > routine. That allows people to either adjust compression parameters
> > > or scratchpad size.
> > > Having a similar check during build time would be nice.
> > >
> > > Signed-off-by: Carl-Daniel Hailfinger
> > > Index: LinuxBIOSv3-tmp/lib/lzma.c
> > >
> > > --- LinuxBIOSv3-tmp/lib/lzma.c  (Revision 682)
> > > +++ LinuxBIOSv3-tmp/lib/lzma.c  (Arbeitskopie)
> > > @@ -14,6 +14,7 @@
> > >  #include "string.h"
> > >  #include "console.h"
> > >
> > > +#define LZMA_SCRATCHPAD_SIZE 15980
> >
> > What's the reason for 15980?  What's the downside to increasing it further?
>
> It's the value I picked during OLPC early bringup, based on compression
> efficiency measurements correlated with stack requirements. It seemed to
> be the sweet spot on Geode back then. Besides that, v3 relies on the
> stack not to outgrow 32k IIRC, otherwise stack and printk buffer will
> corrupt each other.

I don't know enough about the state of v3 at the point of
decompression, but it seems like if you could allocate the buffer on
the heap instead of the stack, it would make this better.  I also
think that it should be an error, not a warning if it can't decompress
the next stage.
> Right now the patch is a no-op except for the better error message.

Yes.  I just thought it might be worth fixing the problem at the same
time if it was simple enough.

Acked-by: Myles Watson

Thanks,
Myles

[Carl-Daniel Hailfinger]

On 23.05.2008 21:45, Myles Watson wrote:
> On Fri, May 23, 2008 at 1:32 PM, Carl-Daniel Hailfinger
>  wrote:
> > On 23.05.2008 21:25, Myles Watson wrote:
> > > On Fri, May 23, 2008 at 1:22 PM, Carl-Daniel Hailfinger
> > >  wrote:
> > > > Print current and wanted LZMA scratchpad size in the decompression
> > > > routine. That allows people to either adjust compression parameters
> > > > or scratchpad size.
> > > > Having a similar check during build time would be nice.
> > > >
> > > > Signed-off-by: Carl-Daniel Hailfinger
> > > > Index: LinuxBIOSv3-tmp/lib/lzma.c
> > > >
> > > > --- LinuxBIOSv3-tmp/lib/lzma.c  (Revision 682)
> > > > +++ LinuxBIOSv3-tmp/lib/lzma.c  (Arbeitskopie)
> > > > @@ -14,6 +14,7 @@
> > > >  #include "string.h"
> > > >  #include "console.h"
> > > >
> > > > +#define LZMA_SCRATCHPAD_SIZE 15980
> > >
> > > What's the reason for 15980?  What's the downside to increasing it further?
> >
> > It's the value I picked during OLPC early bringup, based on compression
> > efficiency measurements correlated with stack requirements. It seemed to
> > be the sweet spot on Geode back then. Besides that, v3 relies on the
> > stack not to outgrow 32k IIRC, otherwise stack and printk buffer will
> > corrupt each other.
>
> I don't know enough about the state of v3 at the point of
> decompression, but it seems like if you could allocate the buffer on
> the heap instead of the stack, it would make this better.  I also
> think that it should be an error, not a warning if it can't decompress
> the next stage.
> > Right now the patch is a no-op except for the better error message.
>
> Yes.  I just thought it might be worth fixing the problem at the same
> time if it was simple enough.
>
> Acked-by: Myles Watson

Thanks, r684 and r685 (fixup of a compilation breakage typo in r684)

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 23.05.2008 21:45, Myles Watson wrote:
> On Fri, May 23, 2008 at 1:32 PM, Carl-Daniel Hailfinger
>  wrote:
> > On 23.05.2008 21:25, Myles Watson wrote:
> > > On Fri, May 23, 2008 at 1:22 PM, Carl-Daniel Hailfinger wrote:
> > > > Index: LinuxBIOSv3-tmp/lib/lzma.c
> > > >
> > > > --- LinuxBIOSv3-tmp/lib/lzma.c  (Revision 682)
> > > > +++ LinuxBIOSv3-tmp/lib/lzma.c  (Arbeitskopie)
> > > > @@ -14,6 +14,7 @@
> > > >  #include "string.h"
> > > >  #include "console.h"
> > > >
> > > > +#define LZMA_SCRATCHPAD_SIZE 15980
> > >
> > > What's the reason for 15980?  What's the downside to increasing it further?
> >
> > It's the value I picked during OLPC early bringup, based on compression
> > efficiency measurements correlated with stack requirements. It seemed to
> > be the sweet spot on Geode back then. Besides that, v3 relies on the
> > stack not to outgrow 32k IIRC, otherwise stack and printk buffer will
> > corrupt each other.
>
> I don't know enough about the state of v3 at the point of
> decompression, but it seems like if you could allocate the buffer on
> the heap instead of the stack, it would make this better.  I also
> think that it should be an error, not a warning if it can't decompress
> the next stage.

And you're absolutely right that it has to error out instead of warning
and causing memory corruption. The same is true for in check for
incorrect stream properties a few lines above. Care to send a patch?

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 23.05.2008 22:03, Carl-Daniel Hailfinger wrote:
> On 23.05.2008 21:45, Myles Watson wrote:
> > On Fri, May 23, 2008 at 1:32 PM, Carl-Daniel Hailfinger
> >  wrote:
> > > On 23.05.2008 21:25, Myles Watson wrote:
> > > > On Fri, May 23, 2008 at 1:22 PM, Carl-Daniel Hailfinger wrote:
> > > > > Index: LinuxBIOSv3-tmp/lib/lzma.c
> > > > >
> > > > > --- LinuxBIOSv3-tmp/lib/lzma.c  (Revision 682)
> > > > > +++ LinuxBIOSv3-tmp/lib/lzma.c  (Arbeitskopie)
> > > > > @@ -14,6 +14,7 @@
> > > > >  #include "string.h"
> > > > >  #include "console.h"
> > > > >
> > > > > +#define LZMA_SCRATCHPAD_SIZE 15980
> > > >
> > > > What's the reason for 15980?  What's the downside to increasing it further?
> > >
> > > It's the value I picked during OLPC early bringup, based on compression
> > > efficiency measurements correlated with stack requirements. It seemed to
> > > be the sweet spot on Geode back then. Besides that, v3 relies on the
> > > stack not to outgrow 32k IIRC, otherwise stack and printk buffer will
> > > corrupt each other.
> >
> > I don't know enough about the state of v3 at the point of
> > decompression, but it seems like if you could allocate the buffer on
> > the heap instead of the stack, it would make this better.  I also
> > think that it should be an error, not a warning if it can't decompress
> > the next stage.
>
> And you're absolutely right that it has to error out instead of warning
> and causing memory corruption. The same is true for in check for
> incorrect stream properties a few lines above. Care to send a patch?

By the way, this is unfixable in v2 and requires a change of ulzma()
return type in v3. v3 discards the return value (outsize) of ulzma()
anyway, so we might as well change ulzma() to return a meaningful error
code.

Regards,
Carl-Daniel