[ovs-dev] [PATCH 4/5] ovs-vswitchd: Fire RCU callbacks before exit to reduce memory leak warnings.
William Tu
u9012063 at gmail.com
Mon Jan 29 22:16:24 UTC 2018
On Thu, Jan 25, 2018 at 3:39 PM, Ben Pfaff <blp at ovn.org> wrote:
> ovs-vswitchd makes extensive use of RCU to defer freeing memory past the
> latest time that it could be in use by a thread. Until now, ovs-vswitchd
> has not waited for RCU callbacks to fire before exiting. This meant that
> in many cases, when ovs-vswitchd exits, many blocks of memory are stuck in
> RCU callback queues, which valgrind often reports as "possible" memory
> leaks.
>
> This commit adds a new function ovsrcu_exit() that waits and fires as many
> RCU callbacks as it reasonably can. It can only do so for the thread that
> calls it and the thread that calls the callbacks, but generally speaking
> ovs-vswitchd shuts down other threads before it exits anyway, so this is
> pretty good.
>
> In my testing this eliminates most valgrind warnings for tests that run
> ovs-vswitchd. This ought to make it easier to distinguish new leaks that
> are real from existing non-leaks.
>
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
Looks good to me.
One limitation is that since this patch init the ovs barrier for size=2,
the ovsrcu_exit() can only be used in ovs-vswitchd. Otherwise users
have to remember to bump up this barrier number.
Acked-by: William Tu <u9012063 at gmai.com>
> lib/ovs-rcu.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++--
> lib/ovs-rcu.h | 2 ++
> vswitchd/ovs-vswitchd.c | 2 ++
> 3 files changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/lib/ovs-rcu.c b/lib/ovs-rcu.c
> index 05a46d4524e3..ebc8120f0fd3 100644
> --- a/lib/ovs-rcu.c
> +++ b/lib/ovs-rcu.c
> @@ -19,6 +19,7 @@
> #include "ovs-rcu.h"
> #include "fatal-signal.h"
> #include "guarded-list.h"
> +#include "latch.h"
> #include "openvswitch/list.h"
> #include "ovs-thread.h"
> #include "openvswitch/poll-loop.h"
> @@ -58,6 +59,9 @@ static struct ovs_mutex ovsrcu_threads_mutex;
> static struct guarded_list flushed_cbsets;
> static struct seq *flushed_cbsets_seq;
>
> +static struct latch postpone_exit;
> +static struct ovs_barrier postpone_barrier;
> +
> static void ovsrcu_init_module(void);
> static void ovsrcu_flush_cbset__(struct ovsrcu_perthread *, bool);
> static void ovsrcu_flush_cbset(struct ovsrcu_perthread *);
> @@ -111,6 +115,8 @@ ovsrcu_quiesced(void)
> } else {
> static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> if (ovsthread_once_start(&once)) {
> + latch_init(&postpone_exit);
> + ovs_barrier_init(&postpone_barrier, 2);
> ovs_thread_create("urcu", ovsrcu_postpone_thread, NULL);
> ovsthread_once_done(&once);
> }
> @@ -232,6 +238,49 @@ ovsrcu_synchronize(void)
> ovsrcu_quiesce_end();
> }
>
> +/* Waits until as many postponed callbacks as possible have executed.
> + *
> + * As a side effect, stops the background thread that calls the callbacks and
> + * prevents it from being restarted. This means that this function should only
> + * be called soon before a process exits, as a mechanism for releasing memory
> + * to make memory leaks easier to detect, since any further postponed callbacks
> + * won't actually get called.
> + *
> + * This function can only wait for callbacks registered by the current thread
> + * and the background thread that calls the callbacks. Thus, it will be most
> + * effective if other threads have already exited. */
> +void
> +ovsrcu_exit(void)
> +{
> + /* Stop the postpone thread and wait for it to exit. Otherwise, there's no
> + * way to wait for that thread to finish calling callbacks itself. */
> + if (!single_threaded()) {
> + ovsrcu_quiesced(); /* Ensure that the postpone thread exists. */
> + latch_set(&postpone_exit);
> + ovs_barrier_block(&postpone_barrier);
> + }
> +
> + /* Repeatedly:
> + *
> + * - Wait for a grace period. One important side effect is to push the
> + * running thread's cbset into 'flushed_cbsets' so that the next call
> + * has something to call.
> + *
> + * - Call all the callbacks in 'flushed_cbsets'. If there aren't any,
> + * we're done, otherwise the callbacks themselves might have requested
> + * more deferred callbacks so we go around again.
> + *
> + * We limit the number of iterations just in case some bug causes an
> + * infinite loop. This function is just for making memory leaks easier to
> + * spot so there's no point in breaking things on that basis. */
> + for (int i = 0; i < 8; i++) {
> + ovsrcu_synchronize();
> + if (!ovsrcu_call_postponed()) {
> + break;
> + }
> + }
> +}
> +
> /* Registers 'function' to be called, passing 'aux' as argument, after the
> * next grace period.
> *
> @@ -303,15 +352,17 @@ ovsrcu_postpone_thread(void *arg OVS_UNUSED)
> {
> pthread_detach(pthread_self());
>
> - for (;;) {
> + while (!latch_is_set(&postpone_exit)) {
> uint64_t seqno = seq_read(flushed_cbsets_seq);
> if (!ovsrcu_call_postponed()) {
> seq_wait(flushed_cbsets_seq, seqno);
> + latch_wait(&postpone_exit);
> poll_block();
> }
> }
>
> - OVS_NOT_REACHED();
> + ovs_barrier_block(&postpone_barrier);
> + return NULL;
> }
>
> static void
> diff --git a/lib/ovs-rcu.h b/lib/ovs-rcu.h
> index 2887bb8f7ffc..ecc4c920102c 100644
> --- a/lib/ovs-rcu.h
> +++ b/lib/ovs-rcu.h
> @@ -308,4 +308,6 @@ bool ovsrcu_is_quiescent(void);
> * once. This can block for a relatively long time. */
> void ovsrcu_synchronize(void);
>
> +void ovsrcu_exit(void);
> +
> #endif /* ovs-rcu.h */
> diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c
> index d5e07c0376cd..53e511999594 100644
> --- a/vswitchd/ovs-vswitchd.c
> +++ b/vswitchd/ovs-vswitchd.c
> @@ -37,6 +37,7 @@
> #include "netdev.h"
> #include "openflow/openflow.h"
> #include "ovsdb-idl.h"
> +#include "ovs-rcu.h"
> #include "openvswitch/poll-loop.h"
> #include "simap.h"
> #include "stream-ssl.h"
> @@ -135,6 +136,7 @@ main(int argc, char *argv[])
> bridge_exit(cleanup);
> unixctl_server_destroy(unixctl);
> service_stop();
> + ovsrcu_exit();
>
> return 0;
> }
> --
> 2.10.2
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list