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

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(a)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(a)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(a)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