On Thu, Jun 15, 2023 at 11:09:59AM +0100, Emil Velikov wrote:
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/ EmilSomewhat 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.
I think it makes sense to report all the decompressions supported by the kernel. Looking to the commit messages where this was added, it seems it was decided to add just 1 for simplicity.
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
However I don't think userspace should go ahead and assume the kernel interface will be updated in a certain way. Particularly because if the kernel decides it'd be better to do it in different way and is not compatible with what userspace assumed, then there would be a regression. +Luis on if there is intention to update the decompression list.
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.
at least the documentation, yes... would be good to have it. Lucas De Marchi
Thanks Emil
