Date:   Sun, 2 Jan 2022 18:58:15 -0800
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Luis Chamberlain <mcgrof@kernel.org>
Cc:     Martin Wilck <martin.wilck@suse.com>, Jessica Yu <jeyu@kernel.org>,
        Kees Cook <keescook@chromium.org>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] module: add in-kernel support for decompressing
Message-ID: <YdJmRzDwruE5jt8W@google.com>
References: <YbLvDWdyFivlj7pP@google.com>
 <YbPsqR5ZyiFwJul3@bombadil.infradead.org>
 <YbP6Q9J++OVKqPfn@google.com>
 <YcC0zpFV8ppOCtZw@bombadil.infradead.org>
 <YcFHZVHbIG3ujDlC@google.com>
 <YcJOUvQJJGSHmlCE@bombadil.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YcJOUvQJJGSHmlCE@bombadil.infradead.org>
Precedence: bulk

Hi Luis,

Happy New Year!

On Tue, Dec 21, 2021 at 01:59:46PM -0800, Luis Chamberlain wrote:
> On Mon, Dec 20, 2021 at 07:17:57PM -0800, Dmitry Torokhov wrote:
> > On Mon, Dec 20, 2021 at 08:52:30AM -0800, Luis Chamberlain wrote:
> > > On Fri, Dec 10, 2021 at 05:09:23PM -0800, Dmitry Torokhov wrote:
> > > > On Fri, Dec 10, 2021 at 04:11:21PM -0800, Luis Chamberlain wrote:
> > > > > On Thu, Dec 09, 2021 at 10:09:17PM -0800, Dmitry Torokhov wrote:
> > > > > > diff --git a/init/Kconfig b/init/Kconfig
> > > > > > index cd23faa163d1..d90774ff7610 100644
> > > > > > --- a/init/Kconfig
> > > > > > +++ b/init/Kconfig
> > > > > > @@ -2305,6 +2305,19 @@ config MODULE_COMPRESS_ZSTD
> > > > > >  
> > > > > >  endchoice
> > > > > >  
> > > > > > +config MODULE_DECOMPRESS
> > > > > > +	bool "Support in-kernel module decompression"
> > > > > > +	depends on MODULE_COMPRESS_GZIP || MODULE_COMPRESS_XZ
> > > > > > +	select ZLIB_INFLATE if MODULE_COMPRESS_GZIP
> > > > > > +	select XZ_DEC if MODULE_COMPRESS_XZ
> > > > > 
> > > > > What if MODULE_COMPRESS_GZIP and MODULE_COMPRESS_XZ are enabled?
> > > > > These are not mutually exclusive.
> > > > 
> > > > They are mutually exclusive, the kernel uses the same (one) compression
> > > > method for all kernel modules that it generates (i.e we do not compress
> > > > drivers/usb/... with gzip while drivers/net/... with xz).
> > > 
> > > Ah yes I failed to see the choice/prompt for it.
> > > 
> > > > The idea here is to allow the kernel consume the same format that was
> > > > used when generating modules. Supporting multiple formats at once is
> > > > overkill IMO.
> > > 
> > > Indeed.
> > > 
> > > > > > +	help
> > > > > > +
> > > > > > +	  Support for decompressing kernel modules by the kernel itself
> > > > > > +	  instead of relying on userspace to perform this task. Useful when
> > > > > > +	  load pinning security policy is enabled.
> > > > > 
> > > > > Shouldn't kernel decompression be faster too? If so, what's the
> > > > > point of doing it in userspace?
> > > > 
> > > > Make the kernel smaller?
> > > 
> > > Yes this I buy.
> > > 
> > > > Have more flexibility with exotic compression
> > > > formats?
> > > 
> > > I just have a hunch that doing module decompression in the kernel will
> > > speed things quite a bit... any chance you can provide some before and
> > > after systemd-analyze ?
> > 
> > If you insist I can try running it, 
> 
> If you can run the test, yes it would be appreciated.

OK, so I finally got around to doing it and the differences are pretty
much noise, as I expected:

5.16.0-rc7:         Startup finished in 5.022s (firmware) + 6.106s (loader) + 1.370s (kernel) + 5.685s (initrd) + 10.842s (userspace) = 29.026s
5.16.0-rc7-patched: Startup finished in 4.958s (firmware) + 6.701s (loader) + 1.382s (kernel) + 5.278s (initrd) + 10.822s (userspace) = 29.145s
5.16.0-rc7-patched: Startup finished in 4.953s (firmware) + 5.912s (loader) + 1.385s (kernel) + 5.327s (initrd) + 10.457s (userspace) = 28.036s

Also see attached.

> 
> > but it should be slower unless your
> > memory controller is so slow that reading file from disk and dealing
> > with page by page decompression is quicker than copying already
> > decompressed data from userspace. 
> 
> With userspace decompression I'd imagine we also have more context switches.

We have plenty of rescheduling points when we allocate new pages, so I
expect we'll be getting preempted while in kernel as well if needed.

> 
> > We still reading and uncompressing
> > file in kmod (to make sure the format is valid)
> 
> I don't understand, that seems wasteful.

This way we can make sure we are not feeding kernel garbage and abort
early. Yes, we could just check signature and hope that the data is good
(and if it is not the kernel will reject it) but this is not a hot path
at all and amount of data we decompress is relatively small, so I do not
think trying to optimize this makes much sense (as shown by the numbers
above).

> 
> > and we can uncompress
> > using large buffers (we are not concerned with using unswappable kernel
> > memory).
> > 
> > Maybe in the future when we have streaming and accelerated in-kernel
> > decompression API we could optimize for that in kmod and see some
> > savings on very large modules.
> 
> That would be very nice.

Again, practical benefit of doing this is pretty much close to 0 in this
particular case.

Thanks.

-- 
Dmitry