Skip to content

Commit

Permalink
vswitchd: Wait for a bridge exit before replying to exit unixctl.
Browse files Browse the repository at this point in the history
Before the cleanup option, the bridge_exit() call was fairly fast,
because it didn't include any particularly long operations.  However,
with the cleanup flag, this function destroys a lot of datapath
resources freeing a lot of memory, waiting on RCU and talking to
the kernel.  That may take a noticeable amount of time, especially
on a busy system or under profilers/sanitizers.  However, the unixctl
'exit' command replies instantly without waiting for any work to
actually be done.  This may cause system test failures or other
issues where scripts expect ovs-vswitchd to exit or destroy all the
datapath resources shortly after appctl call.

Fix that by waiting for the bridge_exit() before replying to the user.
At least, all the datapath resources will actually be destroyed by
the time ovs-appctl exits.

Also moving a structure from stack to global.  Seems cleaner this way.

Since we're not replying right away and it's technically possible
to have multiple clients requesting exit at the same time, storing
connections in an rray.

Fixes: fe13ccd ("vswitchd: Add --cleanup option to the 'appctl exit' command")
Signed-off-by: Ilya Maximets <[email protected]>
  • Loading branch information
igsilya committed Jul 18, 2023
1 parent bffffd8 commit abc8bcf
Showing 1 changed file with 27 additions and 19 deletions.
46 changes: 27 additions & 19 deletions vswitchd/ovs-vswitchd.c
Original file line number Diff line number Diff line change
Expand Up @@ -68,19 +68,19 @@ static unixctl_cb_func ovs_vswitchd_exit;
static char *parse_options(int argc, char *argv[], char **unixctl_path);
OVS_NO_RETURN static void usage(void);

struct ovs_vswitchd_exit_args {
bool *exiting;
bool *cleanup;
};
static struct ovs_vswitchd_exit_args {
struct unixctl_conn **conns;
size_t n_conns;
bool exiting;
bool cleanup;
} exit_args;

int
main(int argc, char *argv[])
{
char *unixctl_path = NULL;
struct unixctl_server *unixctl;
char *unixctl_path = NULL;
char *remote;
bool exiting, cleanup;
struct ovs_vswitchd_exit_args exit_args = {&exiting, &cleanup};
int retval;

set_program_name(argv[0]);
Expand Down Expand Up @@ -111,14 +111,12 @@ main(int argc, char *argv[])
exit(EXIT_FAILURE);
}
unixctl_command_register("exit", "[--cleanup]", 0, 1,
ovs_vswitchd_exit, &exit_args);
ovs_vswitchd_exit, NULL);

bridge_init(remote);
free(remote);

exiting = false;
cleanup = false;
while (!exiting) {
while (!exit_args.exiting) {
OVS_USDT_PROBE(main, run_start);
memory_run();
if (memory_should_report()) {
Expand All @@ -137,16 +135,22 @@ main(int argc, char *argv[])
bridge_wait();
unixctl_server_wait(unixctl);
netdev_wait();
if (exiting) {
if (exit_args.exiting) {
poll_immediate_wake();
}
OVS_USDT_PROBE(main, poll_block);
poll_block();
if (should_service_stop()) {
exiting = true;
exit_args.exiting = true;
}
}
bridge_exit(cleanup);
bridge_exit(exit_args.cleanup);

for (size_t i = 0; i < exit_args.n_conns; i++) {
unixctl_command_reply(exit_args.conns[i], NULL);
}
free(exit_args.conns);

unixctl_server_destroy(unixctl);
service_stop();
vlog_disable_async();
Expand Down Expand Up @@ -304,10 +308,14 @@ usage(void)

static void
ovs_vswitchd_exit(struct unixctl_conn *conn, int argc,
const char *argv[], void *exit_args_)
const char *argv[], void *args OVS_UNUSED)
{
struct ovs_vswitchd_exit_args *exit_args = exit_args_;
*exit_args->exiting = true;
*exit_args->cleanup = argc == 2 && !strcmp(argv[1], "--cleanup");
unixctl_command_reply(conn, NULL);
exit_args.n_conns++;
exit_args.conns = xrealloc(exit_args.conns,
exit_args.n_conns * sizeof *exit_args.conns);
exit_args.conns[exit_args.n_conns - 1] = conn;
exit_args.exiting = true;
if (!exit_args.cleanup) {
exit_args.cleanup = argc == 2 && !strcmp(argv[1], "--cleanup");
}
}

0 comments on commit abc8bcf

Please sign in to comment.