diff mbox series

[5/5] gzip: move huffman code table tracking into gzip state

Message ID 20240411152518.2995-6-dpsmith@apertussolutions.com (mailing list archive)
State Superseded
Headers show
Series Clean up of gzip decompressor | expand

Commit Message

Daniel P. Smith April 11, 2024, 3:25 p.m. UTC 
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/common/gzip/gunzip.c  |  2 ++
 xen/common/gzip/inflate.c | 26 ++++++++++++--------------
 2 files changed, 14 insertions(+), 14 deletions(-)

Comments

Andrew Cooper April 11, 2024, 7:49 p.m. UTC | #1 
On 11/04/2024 4:25 pm, Daniel P. Smith wrote:
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>  xen/common/gzip/gunzip.c  |  2 ++
>  xen/common/gzip/inflate.c | 26 ++++++++++++--------------
>  2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
> index a1b516b925c9..79a641263597 100644
> --- a/xen/common/gzip/gunzip.c
> +++ b/xen/common/gzip/gunzip.c
> @@ -31,6 +31,8 @@ struct gzip_data {
>  
>      unsigned long crc_32_tab[256];
>      unsigned long crc;
> +
> +    unsigned hufts;        /* track memory usage */
>  };
>  
>  #define OF(args)        args
> diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
> index 6c8c7452a31f..53ee1d8ce1e3 100644
> --- a/xen/common/gzip/inflate.c
> +++ b/xen/common/gzip/inflate.c
> @@ -140,7 +140,7 @@ struct huft {
>  };
>  
>  /* Function prototypes */
> -static int huft_build OF((unsigned *, unsigned, unsigned,
> +static int huft_build OF((struct gzip_data *, unsigned *, unsigned, unsigned,
>                            const ush *, const ush *, struct huft **, int *));
>  static int huft_free OF((struct huft *));
>  static int inflate_codes OF((struct gzip_data *, struct huft *, struct huft *, int, int));
> @@ -311,8 +311,6 @@ static const int dbits = 6;          /* bits in base distance lookup table */
>  #define BMAX 16         /* maximum bit length of any code (16 for explode) */
>  #define N_MAX 288       /* maximum number of codes in any set */
>  
> -static unsigned __initdata hufts;      /* track memory usage */
> -
>  /*
>   * Given a list of code lengths and a maximum table size, make a set of
>   * tables to decode that set of codes.  Return zero on success, one if
> @@ -329,8 +327,8 @@ static unsigned __initdata hufts;      /* track memory usage */
>   * @param m Maximum lookup bits, returns actual
>   */
>  static int __init huft_build(
> -    unsigned *b, unsigned n, unsigned s, const ush *d, const ush *e,
> -    struct huft **t, int *m)
> +    struct gzip_data *gd, unsigned *b, unsigned n, unsigned s, const ush *d,
> +    const ush *e, struct huft **t, int *m)
>  {
>      unsigned a;                   /* counter for codes of length k */
>      unsigned f;                   /* i repeats in table every f entries */
> @@ -492,7 +490,7 @@ static int __init huft_build(
>                      goto out;
>                  }
>                  DEBG1("4 ");
> -                hufts += z + 1;         /* track memory usage */
> +                gd->hufts += z + 1;         /* track memory usage */
>                  *t = q + 1;             /* link to list for huft_free() */
>                  *(t = &(q->v.t)) = (struct huft *)NULL;
>                  u[h] = ++q;             /* table starts after link */
> @@ -787,7 +785,7 @@ static int noinline __init inflate_fixed(struct gzip_data *gd)
>      for (; i < 288; i++)          /* make a complete, but wrong code set */
>          l[i] = 8;
>      bl = 7;
> -    if ((i = huft_build(l, 288, 257, cplens, cplext, &tl, &bl)) != 0) {
> +    if ((i = huft_build(gd, l, 288, 257, cplens, cplext, &tl, &bl)) != 0) {
>          free(l);
>          return i;
>      }
> @@ -796,7 +794,7 @@ static int noinline __init inflate_fixed(struct gzip_data *gd)
>      for (i = 0; i < 30; i++)      /* make an incomplete code set */
>          l[i] = 5;
>      bd = 5;
> -    if ((i = huft_build(l, 30, 0, cpdist, cpdext, &td, &bd)) > 1)
> +    if ((i = huft_build(gd, l, 30, 0, cpdist, cpdext, &td, &bd)) > 1)
>      {
>          huft_free(tl);
>          free(l);
> @@ -894,7 +892,7 @@ static int noinline __init inflate_dynamic(struct gzip_data *gd)
>  
>      /* build decoding table for trees--single level, 7 bit lookup */
>      bl = 7;
> -    if ((i = huft_build(ll, 19, 19, NULL, NULL, &tl, &bl)) != 0)
> +    if ((i = huft_build(gd, ll, 19, 19, NULL, NULL, &tl, &bl)) != 0)
>      {
>          if (i == 1)
>              huft_free(tl);
> @@ -971,7 +969,7 @@ static int noinline __init inflate_dynamic(struct gzip_data *gd)
>  
>      /* build the decoding tables for literal/length and distance codes */
>      bl = lbits;
> -    if ((i = huft_build(ll, nl, 257, cplens, cplext, &tl, &bl)) != 0)
> +    if ((i = huft_build(gd, ll, nl, 257, cplens, cplext, &tl, &bl)) != 0)
>      {
>          DEBG("dyn5b ");
>          if (i == 1) {
> @@ -983,7 +981,7 @@ static int noinline __init inflate_dynamic(struct gzip_data *gd)
>      }
>      DEBG("dyn5c ");
>      bd = dbits;
> -    if ((i = huft_build(ll + nl, nd, 0, cpdist, cpdext, &td, &bd)) != 0)
> +    if ((i = huft_build(gd, ll + nl, nd, 0, cpdist, cpdext, &td, &bd)) != 0)
>      {
>          DEBG("dyn5d ");
>          if (i == 1) {
> @@ -1090,15 +1088,15 @@ static int __init inflate(struct gzip_data *gd)
>      /* decompress until the last block */
>      h = 0;
>      do {
> -        hufts = 0;
> +        gd->hufts = 0;
>  #ifdef ARCH_HAS_DECOMP_WDOG
>          arch_decomp_wdog();
>  #endif
>          r = inflate_block(gd, &e);
>          if (r)
>              return r;
> -        if (hufts > h)
> -            h = hufts;
> +        if (gd->hufts > h)
> +            h = gd->hufts;
>      } while (!e);
>  
>      /* Undo too much lookahead. The next read will be byte aligned so we


AFAICT, hothing in inflate() reads h.  So hufts is a write-only variable?

Can't we just delete it, rather than plumb it through into the state
block?  It would certainly shrink this patch somewhat.

~Andrew
Daniel P. Smith April 12, 2024, 11:47 a.m. UTC | #2 
On 4/11/24 15:49, Andrew Cooper wrote:
> On 11/04/2024 4:25 pm, Daniel P. Smith wrote:
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>>   xen/common/gzip/gunzip.c  |  2 ++
>>   xen/common/gzip/inflate.c | 26 ++++++++++++--------------
>>   2 files changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
>> index a1b516b925c9..79a641263597 100644
>> --- a/xen/common/gzip/gunzip.c
>> +++ b/xen/common/gzip/gunzip.c
>> @@ -31,6 +31,8 @@ struct gzip_data {
>>   
>>       unsigned long crc_32_tab[256];
>>       unsigned long crc;
>> +
>> +    unsigned hufts;        /* track memory usage */
>>   };
>>   
>>   #define OF(args)        args
>> diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
>> index 6c8c7452a31f..53ee1d8ce1e3 100644
>> --- a/xen/common/gzip/inflate.c
>> +++ b/xen/common/gzip/inflate.c
>> @@ -140,7 +140,7 @@ struct huft {
>>   };
>>   
>>   /* Function prototypes */
>> -static int huft_build OF((unsigned *, unsigned, unsigned,
>> +static int huft_build OF((struct gzip_data *, unsigned *, unsigned, unsigned,
>>                             const ush *, const ush *, struct huft **, int *));
>>   static int huft_free OF((struct huft *));
>>   static int inflate_codes OF((struct gzip_data *, struct huft *, struct huft *, int, int));
>> @@ -311,8 +311,6 @@ static const int dbits = 6;          /* bits in base distance lookup table */
>>   #define BMAX 16         /* maximum bit length of any code (16 for explode) */
>>   #define N_MAX 288       /* maximum number of codes in any set */
>>   
>> -static unsigned __initdata hufts;      /* track memory usage */
>> -
>>   /*
>>    * Given a list of code lengths and a maximum table size, make a set of
>>    * tables to decode that set of codes.  Return zero on success, one if
>> @@ -329,8 +327,8 @@ static unsigned __initdata hufts;      /* track memory usage */
>>    * @param m Maximum lookup bits, returns actual
>>    */
>>   static int __init huft_build(
>> -    unsigned *b, unsigned n, unsigned s, const ush *d, const ush *e,
>> -    struct huft **t, int *m)
>> +    struct gzip_data *gd, unsigned *b, unsigned n, unsigned s, const ush *d,
>> +    const ush *e, struct huft **t, int *m)
>>   {
>>       unsigned a;                   /* counter for codes of length k */
>>       unsigned f;                   /* i repeats in table every f entries */
>> @@ -492,7 +490,7 @@ static int __init huft_build(
>>                       goto out;
>>                   }
>>                   DEBG1("4 ");
>> -                hufts += z + 1;         /* track memory usage */
>> +                gd->hufts += z + 1;         /* track memory usage */
>>                   *t = q + 1;             /* link to list for huft_free() */
>>                   *(t = &(q->v.t)) = (struct huft *)NULL;
>>                   u[h] = ++q;             /* table starts after link */
>> @@ -787,7 +785,7 @@ static int noinline __init inflate_fixed(struct gzip_data *gd)
>>       for (; i < 288; i++)          /* make a complete, but wrong code set */
>>           l[i] = 8;
>>       bl = 7;
>> -    if ((i = huft_build(l, 288, 257, cplens, cplext, &tl, &bl)) != 0) {
>> +    if ((i = huft_build(gd, l, 288, 257, cplens, cplext, &tl, &bl)) != 0) {
>>           free(l);
>>           return i;
>>       }
>> @@ -796,7 +794,7 @@ static int noinline __init inflate_fixed(struct gzip_data *gd)
>>       for (i = 0; i < 30; i++)      /* make an incomplete code set */
>>           l[i] = 5;
>>       bd = 5;
>> -    if ((i = huft_build(l, 30, 0, cpdist, cpdext, &td, &bd)) > 1)
>> +    if ((i = huft_build(gd, l, 30, 0, cpdist, cpdext, &td, &bd)) > 1)
>>       {
>>           huft_free(tl);
>>           free(l);
>> @@ -894,7 +892,7 @@ static int noinline __init inflate_dynamic(struct gzip_data *gd)
>>   
>>       /* build decoding table for trees--single level, 7 bit lookup */
>>       bl = 7;
>> -    if ((i = huft_build(ll, 19, 19, NULL, NULL, &tl, &bl)) != 0)
>> +    if ((i = huft_build(gd, ll, 19, 19, NULL, NULL, &tl, &bl)) != 0)
>>       {
>>           if (i == 1)
>>               huft_free(tl);
>> @@ -971,7 +969,7 @@ static int noinline __init inflate_dynamic(struct gzip_data *gd)
>>   
>>       /* build the decoding tables for literal/length and distance codes */
>>       bl = lbits;
>> -    if ((i = huft_build(ll, nl, 257, cplens, cplext, &tl, &bl)) != 0)
>> +    if ((i = huft_build(gd, ll, nl, 257, cplens, cplext, &tl, &bl)) != 0)
>>       {
>>           DEBG("dyn5b ");
>>           if (i == 1) {
>> @@ -983,7 +981,7 @@ static int noinline __init inflate_dynamic(struct gzip_data *gd)
>>       }
>>       DEBG("dyn5c ");
>>       bd = dbits;
>> -    if ((i = huft_build(ll + nl, nd, 0, cpdist, cpdext, &td, &bd)) != 0)
>> +    if ((i = huft_build(gd, ll + nl, nd, 0, cpdist, cpdext, &td, &bd)) != 0)
>>       {
>>           DEBG("dyn5d ");
>>           if (i == 1) {
>> @@ -1090,15 +1088,15 @@ static int __init inflate(struct gzip_data *gd)
>>       /* decompress until the last block */
>>       h = 0;
>>       do {
>> -        hufts = 0;
>> +        gd->hufts = 0;
>>   #ifdef ARCH_HAS_DECOMP_WDOG
>>           arch_decomp_wdog();
>>   #endif
>>           r = inflate_block(gd, &e);
>>           if (r)
>>               return r;
>> -        if (hufts > h)
>> -            h = hufts;
>> +        if (gd->hufts > h)
>> +            h = gd->hufts;
>>       } while (!e);
>>   
>>       /* Undo too much lookahead. The next read will be byte aligned so we
> 
> 
> AFAICT, hothing in inflate() reads h.  So hufts is a write-only variable?

Good question, let me study it to make sure.

> Can't we just delete it, rather than plumb it through into the state
> block?  It would certainly shrink this patch somewhat.

Yes, if I can't find any reads of gd->hufts, directly or indirectly 
intermediate variables like h above, then yes, I can try dropping it and 
testing to see if anything breaks.

v/r,
dps
diff mbox series

Patch

diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
index a1b516b925c9..79a641263597 100644
--- a/xen/common/gzip/gunzip.c
+++ b/xen/common/gzip/gunzip.c
@@ -31,6 +31,8 @@  struct gzip_data {
 
     unsigned long crc_32_tab[256];
     unsigned long crc;
+
+    unsigned hufts;        /* track memory usage */
 };
 
 #define OF(args)        args
diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
index 6c8c7452a31f..53ee1d8ce1e3 100644
--- a/xen/common/gzip/inflate.c
+++ b/xen/common/gzip/inflate.c
@@ -140,7 +140,7 @@  struct huft {
 };
 
 /* Function prototypes */
-static int huft_build OF((unsigned *, unsigned, unsigned,
+static int huft_build OF((struct gzip_data *, unsigned *, unsigned, unsigned,
                           const ush *, const ush *, struct huft **, int *));
 static int huft_free OF((struct huft *));
 static int inflate_codes OF((struct gzip_data *, struct huft *, struct huft *, int, int));
@@ -311,8 +311,6 @@  static const int dbits = 6;          /* bits in base distance lookup table */
 #define BMAX 16         /* maximum bit length of any code (16 for explode) */
 #define N_MAX 288       /* maximum number of codes in any set */
 
-static unsigned __initdata hufts;      /* track memory usage */
-
 /*
  * Given a list of code lengths and a maximum table size, make a set of
  * tables to decode that set of codes.  Return zero on success, one if
@@ -329,8 +327,8 @@  static unsigned __initdata hufts;      /* track memory usage */
  * @param m Maximum lookup bits, returns actual
  */
 static int __init huft_build(
-    unsigned *b, unsigned n, unsigned s, const ush *d, const ush *e,
-    struct huft **t, int *m)
+    struct gzip_data *gd, unsigned *b, unsigned n, unsigned s, const ush *d,
+    const ush *e, struct huft **t, int *m)
 {
     unsigned a;                   /* counter for codes of length k */
     unsigned f;                   /* i repeats in table every f entries */
@@ -492,7 +490,7 @@  static int __init huft_build(
                     goto out;
                 }
                 DEBG1("4 ");
-                hufts += z + 1;         /* track memory usage */
+                gd->hufts += z + 1;         /* track memory usage */
                 *t = q + 1;             /* link to list for huft_free() */
                 *(t = &(q->v.t)) = (struct huft *)NULL;
                 u[h] = ++q;             /* table starts after link */
@@ -787,7 +785,7 @@  static int noinline __init inflate_fixed(struct gzip_data *gd)
     for (; i < 288; i++)          /* make a complete, but wrong code set */
         l[i] = 8;
     bl = 7;
-    if ((i = huft_build(l, 288, 257, cplens, cplext, &tl, &bl)) != 0) {
+    if ((i = huft_build(gd, l, 288, 257, cplens, cplext, &tl, &bl)) != 0) {
         free(l);
         return i;
     }
@@ -796,7 +794,7 @@  static int noinline __init inflate_fixed(struct gzip_data *gd)
     for (i = 0; i < 30; i++)      /* make an incomplete code set */
         l[i] = 5;
     bd = 5;
-    if ((i = huft_build(l, 30, 0, cpdist, cpdext, &td, &bd)) > 1)
+    if ((i = huft_build(gd, l, 30, 0, cpdist, cpdext, &td, &bd)) > 1)
     {
         huft_free(tl);
         free(l);
@@ -894,7 +892,7 @@  static int noinline __init inflate_dynamic(struct gzip_data *gd)
 
     /* build decoding table for trees--single level, 7 bit lookup */
     bl = 7;
-    if ((i = huft_build(ll, 19, 19, NULL, NULL, &tl, &bl)) != 0)
+    if ((i = huft_build(gd, ll, 19, 19, NULL, NULL, &tl, &bl)) != 0)
     {
         if (i == 1)
             huft_free(tl);
@@ -971,7 +969,7 @@  static int noinline __init inflate_dynamic(struct gzip_data *gd)
 
     /* build the decoding tables for literal/length and distance codes */
     bl = lbits;
-    if ((i = huft_build(ll, nl, 257, cplens, cplext, &tl, &bl)) != 0)
+    if ((i = huft_build(gd, ll, nl, 257, cplens, cplext, &tl, &bl)) != 0)
     {
         DEBG("dyn5b ");
         if (i == 1) {
@@ -983,7 +981,7 @@  static int noinline __init inflate_dynamic(struct gzip_data *gd)
     }
     DEBG("dyn5c ");
     bd = dbits;
-    if ((i = huft_build(ll + nl, nd, 0, cpdist, cpdext, &td, &bd)) != 0)
+    if ((i = huft_build(gd, ll + nl, nd, 0, cpdist, cpdext, &td, &bd)) != 0)
     {
         DEBG("dyn5d ");
         if (i == 1) {
@@ -1090,15 +1088,15 @@  static int __init inflate(struct gzip_data *gd)
     /* decompress until the last block */
     h = 0;
     do {
-        hufts = 0;
+        gd->hufts = 0;
 #ifdef ARCH_HAS_DECOMP_WDOG
         arch_decomp_wdog();
 #endif
         r = inflate_block(gd, &e);
         if (r)
             return r;
-        if (hufts > h)
-            h = hufts;
+        if (gd->hufts > h)
+            h = gd->hufts;
     } while (!e);
 
     /* Undo too much lookahead. The next read will be byte aligned so we