From: Lucas De Marchi <lucas.demarchi@intel.com>
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: linux-modules@vger.kernel.org, live-patching@vger.kernel.org,
fstests@vger.kernel.org, linux-block@vger.kernel.org,
hare@suse.de, dgilbert@interlog.com, jeyu@kernel.org,
osandov@fb.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/3] libkmod-module: add support for a patient module removal option
Date: Wed, 29 Sep 2021 11:48:31 -0700 [thread overview]
Message-ID: <20210929184810.adrqpsvlfybnc5qt@ldmartin-desk2> (raw)
In-Reply-To: <YVJyIGXN/TR8zdU9@bombadil.infradead.org>
On Mon, Sep 27, 2021 at 06:38:40PM -0700, Luis Chamberlain wrote:
>On Thu, Sep 23, 2021 at 01:51:56AM -0700, Lucas De Marchi wrote:
>> On Mon, Aug 09, 2021 at 10:16:02PM -0700, Luis Chamberlain wrote:
>> The story was not kind like that. It wasn't removed "in favor for a 10
>> second sleep" in the sense that the sleep would replace the wait.
>>
>> It was actually for "this wait logic in the kernel is complex and
>> buggy, let's try to remove it". So we decided to deprecate it and add
>> a sleep rmmod to see if anyone complained. 1 year later of no complains
>> we removed it from kernel. This was all after noticing we had never
>> implemented the wait logic in modprobe - it was only done in rmmod.
>
>OK fixed the commit log thanks!
>
>> > --- a/libkmod/libkmod-module.c
>> > +++ b/libkmod/libkmod-module.c
>> > @@ -30,6 +30,9 @@
>> > #include <stdlib.h>
>> > #include <string.h>
>> > #include <unistd.h>
>> > +#include <poll.h>
>> > +#include <time.h>
>> > +#include <math.h>
>> > #include <sys/mman.h>
>> > #include <sys/stat.h>
>> > #include <sys/syscall.h>
>> > @@ -802,6 +805,143 @@ KMOD_EXPORT int kmod_module_remove_module(struct kmod_module *mod,
>> > return err;
>> > }
>> >
>> > +static int timespec_to_ms(struct timespec *t)
>> > +{
>> > + return (t->tv_sec * 1000) + lround(t->tv_nsec / 1000000);
>> > +}
>> > +
>> > +static int time_delta_ms(struct timespec *before, struct timespec *after)
>> > +{
>> > + if (!before || !after)
>> > + return 0;
>> > + return timespec_to_ms(after) - timespec_to_ms(before);
>> > +}
>>
>> we have a similar thing in util.[ch]
>
>Alright, this OK?
>
>diff --git a/shared/util.c b/shared/util.c
>index b487b5f..b911e63 100644
>--- a/shared/util.c
>+++ b/shared/util.c
>@@ -466,6 +466,19 @@ unsigned long long ts_usec(const struct timespec *ts)
> (unsigned long long) ts->tv_nsec / NSEC_PER_USEC;
> }
>
>+unsigned long long ts_msec(const struct timespec *ts)
>+{
>+ return ts_usec(ts) * 1000;
>+}
>+
>+unsigned long long ts_delta_ms(const struct timespec *before,
>+ const struct timespec *after)
>+{
>+ if (!before || !after)
>+ return 0;
>+ return ts_msec(after) - ts_msec(before);
>+}
>+
> unsigned long long stat_mstamp(const struct stat *st)
> {
> #ifdef HAVE_STRUCT_STAT_ST_MTIM
>diff --git a/shared/util.h b/shared/util.h
>index c6a31df..f8c28e7 100644
>--- a/shared/util.h
>+++ b/shared/util.h
>@@ -43,6 +43,9 @@ int mkdir_p(const char *path, int len, mode_t mode);
> int mkdir_parents(const char *path, mode_t mode);
> unsigned long long stat_mstamp(const struct stat *st);
> unsigned long long ts_usec(const struct timespec *ts);
>+unsigned long long ts_msec(const struct timespec *ts);
>+unsigned long long ts_delta_ms(const struct timespec *before,
>+ const struct timespec *after);
>
> /* endianess and alignments */
> /* ************************************************************************ */
>
>> > +/**
>> > + * kmod_module_remove_module_wait:
>> > + * @mod: kmod module
>> > + * @flags: flags to pass to Linux kernel when removing the module. The only valid flag is
>> > + * KMOD_REMOVE_FORCE: force remove module regardless if it's still in
>> > + * use by a kernel subsystem or other process;
>> > + * KMOD_REMOVE_NOWAIT is always enforced, causing us to pass O_NONBLOCK to
>> > + * delete_module(2). We do the waiting in userspace, if a wait was desired.
>> > + *
>> > + * Remove a module from Linux kernel patiently.
>> > + *
>> > + * Returns: 0 on success or < 0 on failure.
>> > + */
>> > +KMOD_EXPORT int kmod_module_remove_module_wait(struct kmod_module *mod,
>> > + unsigned int flags,
>> > + bool wait)
>>
>> why do you have kmod_get_refcnt_timeout/kmod_set_refcnt_timeout instead
>> of just doing s/bool wait/unsigned int wait_msec/)?
>
>Because it lets us do a smaller change on the respetive tools:
>
>tools/modprobe.c- flags |= KMOD_REMOVE_FORCE;
>tools/modprobe.c-
>tools/modprobe.c: err = kmod_module_remove_module_wait(mod, flags, do_remove_patient);
>tools/modprobe.c- if (err == -EEXIST) {
>tools/modprobe.c- if (!first_time)
>--
>tools/remove.c- goto unref;
>tools/remove.c-
>tools/remove.c: err = kmod_module_remove_module_wait(mod, 0, do_remove_patient);
>tools/remove.c- if (err < 0)
>tools/remove.c- goto unref;
>--
>tools/rmmod.c- }
>tools/rmmod.c-
>tools/rmmod.c: err = kmod_module_remove_module_wait(mod, flags,
>tools/rmmod.c- do_remove_patient);
>tools/rmmod.c- if (err < 0) {
>
>That is, the timeout is contextual of the context.
>
>> > + if ((refcnt <= 0) || (refcnt > 0 && !wait)) {
>> > + NOTICE(mod->ctx, "%s refcnt is %d\n", mod->name, (int) refcnt);
>> > + err_time = clock_gettime(CLOCK_MONOTONIC, &t2);
>> > + if (err_time != 0)
>> > + kmod_set_removal_timeout(mod->ctx, 0);
>>
>> I don't follow why kmod_module_get_refcnt_wait() is setting the removal
>> timeout at all. This seems to be doing it behind users back.
>
>Because if clock_gettime() returns something other than 0 then
>your clock is messed up and you should not be using a timeout, so
>yes, we correct that then. We can scream loud, or use a default.
>
>I figured not using one would be better in that case.
>
>> The idea of using the refcnt fd was actually that then
>> users could integrate it on their mainloops (probably using epoll). And
>> then the same impl could be shared by kmod_module_remove_module_wait(),
>> which would do a select().
>>
>> This seems more like a kmod_module_refcnt_wait_zero() using poll()
>> + adjusting the timeout
>
>Sorry don't follow. And since I have one day before vacation, I suppose
>I won't get to this until I get back. But I'd be happy if you massage
>it as you see fit as you're used to the code base and I'm sure have
>a better idea of what likely is best for the library.
sure, np. I will take a look as time permits.
thanks
Lucas De Marchi
>
>> > + ret = kmod_module_get_refcnt_wait(mod, do_remove_patient);
>>
>> for tool implementation, shouldn't we just ignore
>> kmod_module_get_refcnt() and proceed to
>> kmod_module_remove_module_wait()?
>
>I'll let you decide. Otherwise this will have to wait until I get back
>from vacation.
>
> Luis
next prev parent reply other threads:[~2021-09-29 18:48 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-10 5:15 [PATCH v2 0/3] kmod: add patient module removal support Luis Chamberlain
2021-08-10 5:16 ` [PATCH v2 1/3] libkmod: add a library notice log level print Luis Chamberlain
2021-09-21 5:53 ` Lucas De Marchi
2021-09-21 18:40 ` Luis Chamberlain
2021-09-23 8:02 ` Lucas De Marchi
2021-08-10 5:16 ` [PATCH v2 2/3] libkmod/libkmod-module: add refcnt fd helper Luis Chamberlain
2021-08-10 5:16 ` [PATCH v2 3/3] libkmod-module: add support for a patient module removal option Luis Chamberlain
2021-09-23 8:51 ` Lucas De Marchi
2021-09-28 1:38 ` Luis Chamberlain
2021-09-29 18:48 ` Lucas De Marchi [this message]
2021-11-15 20:21 ` Luis Chamberlain
2021-12-08 12:53 ` Luis Chamberlain
2021-09-15 17:40 ` [PATCH v2 0/3] kmod: add patient module removal support Luis Chamberlain
2021-09-21 5:51 ` Lucas De Marchi
2022-02-17 8:13 ` Luis Chamberlain
2022-02-18 8:20 ` Lucas De Marchi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210929184810.adrqpsvlfybnc5qt@ldmartin-desk2 \
--to=lucas.demarchi@intel.com \
--cc=dgilbert@interlog.com \
--cc=fstests@vger.kernel.org \
--cc=hare@suse.de \
--cc=jeyu@kernel.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-modules@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=mcgrof@kernel.org \
--cc=osandov@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).