Re: [PATCH 3/5] gzip: refactor state tracking

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 4/12/24 08:53, Andrew Cooper wrote:
> On 12/04/2024 1:51 pm, Daniel P. Smith wrote:
> > On 4/12/24 08:18, Andrew Cooper wrote:
> > > On 12/04/2024 12:41 pm, Daniel P. Smith wrote:
> > > > On 4/12/24 07:34, Daniel P. Smith wrote:
> > > > > On 4/11/24 15:24, Andrew Cooper wrote:
> > > > > > On 11/04/2024 4:25 pm, Daniel P. Smith wrote:
> > > > > > > diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
> > > > > > > index 1bcb007395ba..9b4891731b8b 100644
> > > > > > > --- a/xen/common/gzip/gunzip.c
> > > > > > > +++ b/xen/common/gzip/gunzip.c
> > > > > > > @@ -102,12 +109,13 @@ __init int gzip_check(char *image, unsigned
> > > > > > > long image_len)
> > > > > > >     __init int perform_gunzip(char *output, char *image, unsigned
> > > > > > > long image_len)
> > > > > > >     {
> > > > > > > +    struct gzip_data gd;
> > > > > > >         int rc;
> > > > > > By the end of this series,
> > > > > >
> > > > > > Reading symbols from xen-syms...
> > > > > > (gdb) p sizeof(struct gzip_data)
> > > > > > $1 = 2120
> > > > > >
> > > > > > x86 has an 8k stack and this takes 1/4 of it.  Other bits of state
> > > > > > are
> > > > > > dynamically allocated, even in inflate.c, so I'd highly recommend
> > > > > > doing
> > > > > > the same for this.
> > > > > I take it you are mainly talking about crc_32_tab? Yes, I can switch
> > > > > that to being dynamically allocated.
> > > > Never mind, reading your comment on patch4 made me realize you wanted
> > > > the instance of struct dynamically allocated. Though the answer is
> > > > still, yes, we can dynamically allocate it.
> > > I wrote this before realising that crc_32_tag could be shrunk.
> > >
> > > If it's only1k on the stack, then that's a whole lot less bad, and is
> > > perhaps ok.  I guess it depends on the stack size of the other
> > > architectures.
> > >
> > > Still - I expect dynamically allocating would be a safer course of
> > > action.  Internal blocks are dynamically allocated already, so this is
> > > "just" one more.
> > Another course of action that could be considered is making a unit
> > file global instance of the struct, and then memset() it to zero
> > instead of allocating and freeing from heap. The global instance
> > should be able to be made init_data and dropped after init was complete.
> >
> > I am good with either way, just let me know which would be preferred
> > and I will adjust appropriately.
> Other things inside gzunip() are dynamically allocated.  I'd keep this
> consistent with the others.
Ack.

v/r,
dps