| Message ID | 20240110192939.15220-2-frode.nordahl@canonical.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | Introduce cooperative multitasking to improve OVSDB RAFT cluster operation. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On 1/10/24 20:29, Frode Nordahl wrote: > At present the timeval timewarp functionality is tightly coupled > with the unixctl interface for external operation by test suite. > > It is however desirable to make use of the timewarp functionality > directly in unit tests. > > Split unixctl callback interface and the actual timewarp > functionality into separate functions. > > This will be used in a patch that implements unit tests for the > cooperative multitasking module. > > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> > --- > lib/timeval.c | 56 ++++++++++++++++++++++++++++++++++++++++----------- > lib/timeval.h | 4 ++++ > 2 files changed, 48 insertions(+), 12 deletions(-) Hi, Frode. Thanks for the patch! I agree that idea is very useful for unit testing. Though, I'm not sure we need to replicate the exact unixctl interface internally. The unixctl interface is designed around a server that calls poll_block() and we also really want to allow other threads to do some work within long warps, so we sleep and we have a stepping interface that adjusts time in small chunks. I'm not sure if internal interface needs most of that, and I'm also not sure if it is actually easy to take advantage of such functionality from within the process. What I'm trying to say is that the program that is using this interface is managing the threads, and it likely knows when exectly it needs the time to be warped without relying on that to be done by the poll_block(). So, I think, a much simpler API might be better here. We can have timeval_stop(void) that stops the timer, i.e. directs it into slow path, and timeval_warp(long long int) that directly advances the time, i.e. just grabs the mutex, converts the argument into timeval and calls timeval_add(). In a worst case, if someone will actually need to advance time gradually in the code, they can always just call timeval_warp() before they call poll_block(). The multi-threading support can be simplified down to adding seq_change(timewarp_seq); into timeval_warp() and adding one more function timewarp_wait() that waits on it. It seems reasonable to expect multi-threaded test applications to call that before poll_block(). But it is not even necessary to have for tests introduced in this patch set, so may be delayed until such fucntionality is actually needed. What do you think? Best regards, Ilya Maximets.
On Thu, Jan 11, 2024 at 10:49 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 1/10/24 20:29, Frode Nordahl wrote: > > At present the timeval timewarp functionality is tightly coupled > > with the unixctl interface for external operation by test suite. > > > > It is however desirable to make use of the timewarp functionality > > directly in unit tests. > > > > Split unixctl callback interface and the actual timewarp > > functionality into separate functions. > > > > This will be used in a patch that implements unit tests for the > > cooperative multitasking module. > > > > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> > > --- > > lib/timeval.c | 56 ++++++++++++++++++++++++++++++++++++++++----------- > > lib/timeval.h | 4 ++++ > > 2 files changed, 48 insertions(+), 12 deletions(-) > > Hi, Frode. Thanks for the patch! Thank you for taking the time to review, much appreciated! > I agree that idea is very useful for unit testing. Though, I'm not > sure we need to replicate the exact unixctl interface internally. > > The unixctl interface is designed around a server that calls > poll_block() and we also really want to allow other threads to do > some work within long warps, so we sleep and we have a stepping > interface that adjusts time in small chunks. > > I'm not sure if internal interface needs most of that, and I'm also > not sure if it is actually easy to take advantage of such functionality > from within the process. What I'm trying to say is that the program > that is using this interface is managing the threads, and it likely > knows when exectly it needs the time to be warped without relying on > that to be done by the poll_block(). > > So, I think, a much simpler API might be better here. We can have > timeval_stop(void) that stops the timer, i.e. directs it into slow path, > and timeval_warp(long long int) that directly advances the time, i.e. > just grabs the mutex, converts the argument into timeval and calls > timeval_add(). I agree, in v1 I was mostly concerned about surfacing the feature for the test with as little change and as much reuse as possible. Adding a new function that advances the time directly does not duplicate too much and allows us to do away with the ugly pointer hack. > In a worst case, if someone will actually need to advance time > gradually in the code, they can always just call timeval_warp() > before they call poll_block(). > > The multi-threading support can be simplified down to adding > seq_change(timewarp_seq); into timeval_warp() and adding one more > function timewarp_wait() that waits on it. It seems reasonable to > expect multi-threaded test applications to call that before > poll_block(). But it is not even necessary to have for tests > introduced in this patch set, so may be delayed until such > fucntionality is actually needed. > > What do you think? No immediate need for multi threaded support for this, so it makes sense to me to expose a simpler interface. Thanks!
diff --git a/lib/timeval.c b/lib/timeval.c index 193c7bab1..4d4951c42 100644 --- a/lib/timeval.c +++ b/lib/timeval.c @@ -533,6 +533,10 @@ nsec_to_timespec(long long int nsec, struct timespec *ts) ts->tv_nsec = nsec; } +#define MONOTONIC_CLOCK_HAS_WARP_CONN(c) \ + ( (struct clock *) c)->large_warp.conn && \ + (uintptr_t) ( (struct clock *) c)->large_warp.conn != UINTPTR_MAX + static void timewarp_work(void) { @@ -540,6 +544,8 @@ timewarp_work(void) struct timespec warp; ovs_mutex_lock(&c->mutex); + /* note that we do not use MONOTONIC_CLOCK_HAS_WARP_CONN here as we want + * to perform work in case of an internal caller has requested warp. */ if (!c->large_warp.conn) { ovs_mutex_unlock(&c->mutex); return; @@ -560,7 +566,9 @@ timewarp_work(void) } if (!c->large_warp.total_warp) { - unixctl_command_reply(c->large_warp.conn, "warped"); + if (MONOTONIC_CLOCK_HAS_WARP_CONN(c)) { + unixctl_command_reply(c->large_warp.conn, "warped"); + } c->large_warp.conn = NULL; } @@ -763,17 +771,22 @@ get_cpu_usage(void) /* "time/stop" stops the monotonic time returned by e.g. time_msec() from * advancing, except due to later calls to "time/warp". */ -static void -timeval_stop_cb(struct unixctl_conn *conn, - int argc OVS_UNUSED, const char *argv[] OVS_UNUSED, - void *aux OVS_UNUSED) +void +timeval_stop(void) { ovs_mutex_lock(&monotonic_clock.mutex); atomic_store_relaxed(&monotonic_clock.slow_path, true); monotonic_clock.stopped = true; xclock_gettime(monotonic_clock.id, &monotonic_clock.cache); ovs_mutex_unlock(&monotonic_clock.mutex); +} +static void +timeval_stop_cb(struct unixctl_conn *conn, + int argc OVS_UNUSED, const char *argv[] OVS_UNUSED, + void *aux OVS_UNUSED) +{ + timeval_stop(); unixctl_command_reply(conn, NULL); } @@ -787,6 +800,25 @@ timeval_stop_cb(struct unixctl_conn *conn, * time to run after the clock has been advanced by MSECS. * * Does not affect wall clock readings. */ +void +timeval_warp(long long int total_warp, long long int msecs) +{ + ovs_mutex_lock(&monotonic_clock.mutex); + if (!MONOTONIC_CLOCK_HAS_WARP_CONN(&monotonic_clock)) { + /* we do not have a unixctl connection for internal callers, but the + * pointer needs to be set for timewarp_work() to know when to + * perform work. */ + monotonic_clock.large_warp.conn = (struct unixctl_conn *) UINTPTR_MAX; + } + atomic_store_relaxed(&monotonic_clock.slow_path, true); + monotonic_clock.large_warp.total_warp = total_warp; + monotonic_clock.large_warp.warp = msecs; + monotonic_clock.large_warp.main_thread_id = ovsthread_id_self(); + ovs_mutex_unlock(&monotonic_clock.mutex); + + timewarp_work(); +} + static void timeval_warp_cb(struct unixctl_conn *conn, int argc OVS_UNUSED, const char *argv[], void *aux OVS_UNUSED) @@ -799,25 +831,25 @@ timeval_warp_cb(struct unixctl_conn *conn, } ovs_mutex_lock(&monotonic_clock.mutex); - if (monotonic_clock.large_warp.conn) { + if (MONOTONIC_CLOCK_HAS_WARP_CONN(&monotonic_clock)) { ovs_mutex_unlock(&monotonic_clock.mutex); unixctl_command_reply_error(conn, "A previous warp in progress"); return; } - atomic_store_relaxed(&monotonic_clock.slow_path, true); monotonic_clock.large_warp.conn = conn; - monotonic_clock.large_warp.total_warp = total_warp; - monotonic_clock.large_warp.warp = msecs; - monotonic_clock.large_warp.main_thread_id = ovsthread_id_self(); ovs_mutex_unlock(&monotonic_clock.mutex); - timewarp_work(); + timeval_warp(total_warp, msecs); } +void +timeval_timewarp_enable(void) { + timewarp_enabled = true; +} void timeval_dummy_register(void) { - timewarp_enabled = true; + timeval_timewarp_enable(); unixctl_command_register("time/stop", "", 0, 0, timeval_stop_cb, NULL); unixctl_command_register("time/warp", "[large_msecs] msecs", 1, 2, timeval_warp_cb, NULL); diff --git a/lib/timeval.h b/lib/timeval.h index 502f703d4..ea7977e0c 100644 --- a/lib/timeval.h +++ b/lib/timeval.h @@ -81,6 +81,10 @@ long long int time_boot_msec(void); void timewarp_run(void); +void timeval_timewarp_enable(void); +void timeval_stop(void); +void timeval_warp(long long int, long long int); + #ifdef __cplusplus } #endif
At present the timeval timewarp functionality is tightly coupled with the unixctl interface for external operation by test suite. It is however desirable to make use of the timewarp functionality directly in unit tests. Split unixctl callback interface and the actual timewarp functionality into separate functions. This will be used in a patch that implements unit tests for the cooperative multitasking module. Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> --- lib/timeval.c | 56 ++++++++++++++++++++++++++++++++++++++++----------- lib/timeval.h | 4 ++++ 2 files changed, 48 insertions(+), 12 deletions(-)