On 7/5/23 15:54, Eelco Chaudron wrote: > > > On 4 Jul 2023, at 15:11, Ilya Maximets wrote: > >> 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. > > Thanks, yes it looks cleaner. One comment inline below. > > //Eelco > > >> Fixes: fe13ccdca6a2 ("vswitchd: Add --cleanup option to the 'appctl exit' >> command") >> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org> >> --- >> vswitchd/ovs-vswitchd.c | 38 +++++++++++++++++++------------------- >> 1 file changed, 19 insertions(+), 19 deletions(-) >> >> diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c >> index a244d2f70..55b437871 100644 >> --- a/vswitchd/ovs-vswitchd.c >> +++ b/vswitchd/ovs-vswitchd.c >> @@ -68,19 +68,18 @@ 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 *conn; >> + 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]); >> @@ -111,14 +110,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()) { >> @@ -137,16 +134,20 @@ 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); >> + >> + if (exit_args.conn) { >> + unixctl_command_reply(exit_args.conn, NULL); >> + } >> unixctl_server_destroy(unixctl); >> service_stop(); >> vlog_disable_async(); >> @@ -304,10 +305,9 @@ 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.conn = conn; > > Should we try to protect from two exit command in the same > unixctl_server_run()? > Something like: > > if (exit_args.conn) { > unixctl_command_reply(conn, NULL); > } else { > exit_args.conn = conn; > } >
Good point. It's unlikely, but can happen. We could either do what you suggested or store an array of connections and try to reply to all of them. It becomes a bit inconsistent though if different calls have different options. What do you think? Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev