On 7 Jul 2023, at 14:19, Ilya Maximets wrote:
> On 7/7/23 14:12, Eelco Chaudron wrote:
>>
>>
>> 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 <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?
>>
>> I was thinking of an array also but what happens with the one not fitting in
>> the array!?
>
> People created realloc for that. :)
>
>>
>> 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?
>
> That also an option. Not sure what is better.
> An array is the most clean solution, I guess.
ACK lets go with the realloc() array ;)
>
> Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
