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 <[email protected]>
>> ---
>> 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
