Re: [PATCH 3/4] Split "auto" compression mode into its own path

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3452 bytes --]

On Thu, Apr 25, 2024 at 06:46:05AM +0200, Stefano Brivio wrote:
> On Sat, 6 Apr 2024 14:06:39 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Apr 05, 2024 at 08:10:02PM +0200, Stefano Brivio wrote:
> > > On Fri, 22 Mar 2024 13:27:38 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > mbuto supports "auto" compression mode where we detect the fastest
> > > > compressor and use it.  This is structured a bit oddly - cpio_compress()
> > > > first handles the case of an explicitly selected compressor, then handles
> > > > the auto-detected case, redundantly actually implementing the compression
> > > > once it has picked one.  
> > > 
> > > Yes, we don't want to risk keeping around several formats if the
> > > archives are big... but you're not changing this, right?  
> > 
> > I don't believe so, no.
> > 
> > > > Make this a bit clearer: first handle the "auto" case by calling out to
> > > > the testing code, and using that to set the parameter for the specific
> > > > compression path.
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >  mbuto | 53 ++++++++++++++++++++++++-----------------------------
> > > >  1 file changed, 24 insertions(+), 29 deletions(-)
> > > > 
> > > > diff --git a/mbuto b/mbuto
> > > > index 49d032c..550f76e 100755
> > > > --- a/mbuto
> > > > +++ b/mbuto
> > > > @@ -566,31 +566,11 @@ cpio_init() {
> > > >  	fi
> > > >  }
> > > >  
> > > > -# cpio_compress() - Compress archive, test available methods if none is selected
> > > > +# compress_select() - Try compressors and pick the fastest  
> > > 
> > > Now we can say we pick the fastest, but it's not clear in what: we care
> > > about decompression, so perhaps:
> > > 
> > > # compress_select() - Try compressors and pick the fastest to decompress image
> > > 
> > > ?  
> > 
> > I don't think that's accurate.  AFAICT the code is selecting the
> > fastest to compress, not decompress,
> 
> No, why? Look:
> 
>                 __start=$("${CAT}" /proc/uptime)
>                 __start="${__start% *}"
> 
>                 for _ in $("${SEQ}" 1 5); do
>                         "${CP}" "${compress_test1}" "${compress_test2}"
>                         ${__cmd} --force -d -c "${compress_test2}" > /dev/null
>                 done
> 
>                 __end=$("${CAT}" /proc/uptime)
>                 __end="${__end% *}"
> 
> see the '-d' there ('-c' is usually '--stdout').

Oh, yes.  My mistake.

> > and I didn't change that.  I agree that optimizing decompression
> > speed would make more sense.
> > 
> > Honestly, I'm not really convinced that the auto mode is useful
> > anyway: even if we changed to decompression speed, the speed on the
> > location running mbuto isn't necessarily the same as on the target
> > doing the decompression (though given mbuto's usage model, it's
> > lkely).
> 
> Well, it's exactly the intended usage -- any other usage would be
> rather tricky (at least, anything with a different architecture for the
> guest). That's how the current passt test suite uses it, too. And it
> really makes a difference if you're working with kselftests.

Hm, ok.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]