On Thu, 15 Jun 2023 at 10:36, Emil Velikov <emil.l.velikov@xxxxxxxxx> 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.
>
> 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?
>
> 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.
>
> Thanks again and HTH o/
> Emil
Somewhat related:
Would it make sense to read /sys/module/compression if it contained
multiple lines - one for each supported compression. This way, kmod
will just work when the kernel is updated to advertise them all.
There is about 0.000001% chance that changing the format of the sysfs
file might cause regression, which can be looked into if an issue.
After all the sysfs entry is just 1 year old and is undocumented
(cough) so nobody should be using it, right :-P
I'm really tempted to send a patch tweaking the sysfs file and adding
documentation. Please let me know if you think that's a bad idea, or
you have one already queued.
Thanks
Emil
