Re: [PATCH 5/5] libkmod: Use kernel decompression when available

From: Lucas De Marchi <lucas.de.marchi@gmail.com>
To: Emil Velikov <emil.l.velikov@gmail.com>
Cc: linux-modules@vger.kernel.org
Subject: Re: [PATCH 5/5] libkmod: Use kernel decompression when available
Date: Thu, 15 Jun 2023 22:12:15 -0700	[thread overview]
Message-ID: <fdftww4ijeh5tngo6koko3syzk7ubtb3n5ixdadfc3lutjqrom@r7cjuzf3qm64> (raw)
In-Reply-To: <CACvgo53yHOd964PoQkM_oX3LGcr+qCagz64T+sxcP-eAveS85g@mail.gmail.com>

On Thu, Jun 15, 2023 at 10:36:31AM +0100, Emil Velikov wrote:
>Greetings Lucas, list,
>
>I've pulled the email off lore.kernel.org manually (haven't played
>with lei yet), so chances are the following will be "slightly"
>malformed.

One easy easy if you want to pick single patch series is to use b4

b4 mbox 20230601224001.23397-1-lucas.de.marchi@gmail.com
neomut -f ./20230601224001.23397-1-lucas.de.marchi@gmail.com.mbx

or instead of neomutt, import the mbox to your favorite email client.

>
>Above all - hell yeah, thank you for wiring this neat functionality.
>
>Out of curiosity: have you done any measurements - CPU cycles, memory
>or other - how well the kernel decompression performs vs the userspace
>one?

no, I didn't.  I think one important aspect is when paired with the
patch to the kernel that deduplicates the module loading. With
compressed modules, that would be irrelevant as libkmod would go the
init_module() path rather than finit_module().

A reasonable test would be to create a VM with a bunch of CPUs (so we
get many parallel requests or the same modules) and then measure
a) 6.4-rcX
b) 6.4-rcX + that kernel patch
c) 6.4-rcX + that kernel patch + this patch series

>
>That said, I may have spotted a small bug, namely:
>
>> --- a/libkmod/libkmod-module.c
>> +++ b/libkmod/libkmod-module.c
>> @@ -864,15 +864,24 @@ extern long init_module(const void *mem, unsigned long len, const char *args);
>>  static int do_finit_module(struct kmod_module *mod, unsigned int flags,
>>     const char *args)
>>  {
>> + enum kmod_file_compression_type compression, kernel_compression;
>>  unsigned int kernel_flags = 0;
>>  int err;
>>
>>  /*
>> - * Re-use ENOSYS, returned when there is no such syscall, so the
>> - * fallback to init_module applies
>> + * When module is not compressed or its compression type matches the
>> + * one in use by the kernel, there is no need to read the file
>> + * in userspace. Otherwise, re-use ENOSYS to trigger the same fallback
>> + * as when finit_module() is not supported.
>>  */
>> - if (!kmod_file_get_direct(mod->file))
>> - return -ENOSYS;
>> + compression = kmod_file_get_compression(mod->file);
>> + kernel_compression = kmod_get_kernel_compression(mod->ctx);
>> + if (!(compression == KMOD_FILE_COMPRESSION_NONE ||
>> +       compression == kernel_compression))
>> + return ENOSYS;
>> +
>
>Old code returns negative -ENOSYS (negative), the new one a positive
>ENOSYS. Where the fallback, mentioned in the comment just above,
>triggers on the former negative ENOSYS.
>
>Mind you I'm still sipping coffee, so chances are I'm missing something here.

no, apparently I didn't have enough coffee when I wrote this.
I'll fix this up on next version.

thanks
Lucas De Marchi

>
>Thanks again and HTH o/
>Emil