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.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
