On 7 Jul 2023, at 12:58, Ilya Maximets wrote:
> 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?
I was thinking of an array also but what happens with the one not fitting in
the array!?
However giving it another thought, it might just break with a pipe error and
quit. So maybe we should just reply to the first one and let the other dingle
until the application get killed?
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
