On 1/12/24 16:28, Frode Nordahl wrote: > On Fri, Jan 12, 2024 at 3:11 PM Ilya Maximets <[email protected]> wrote: >> >> On 1/12/24 12:16, Frode Nordahl wrote: >>> On Fri, Jan 12, 2024 at 12:26 AM Ilya Maximets <[email protected]> wrote: >>>> >>>> On 1/10/24 20:29, Frode Nordahl wrote: >>>>> One of the goals of Open vSwitch is to be as resource efficient as >>>>> possible. Core parts of the program has been implemented as >>>>> asynchronous state machines, and when absolutely necessary >>>>> additional threads are used. >>>>> >>>>> Introduce cooperative multitasking module which allow us to >>>>> interleave important processing with long running tasks while >>>>> avoiding the additional resource consumption of threads and >>>>> complexity of asynchronous state machines. >>>>> >>>>> We will use this module to ensure long running processing in the >>>>> OVSDB server does not interfere with stable maintenance of the >>>>> RAFT cluster in subsequent patches. >>>>> >>>>> Suggested-by: Ilya Maximets <[email protected]> >>>>> Signed-off-by: Frode Nordahl <[email protected]> >>>>> --- >>>>> lib/automake.mk | 3 + >>>>> lib/cooperative-multitasking-private.h | 31 +++ >>>>> lib/cooperative-multitasking.c | 195 +++++++++++++++++++ >>>>> lib/cooperative-multitasking.h | 42 ++++ >>>>> tests/automake.mk | 1 + >>>>> tests/library.at | 10 + >>>>> tests/ovsdb-server.at | 1 + >>>>> tests/test-cooperative-multitasking.c | 259 +++++++++++++++++++++++++ >>>>> 8 files changed, 542 insertions(+) >>>>> create mode 100644 lib/cooperative-multitasking-private.h >>>>> create mode 100644 lib/cooperative-multitasking.c >>>>> create mode 100644 lib/cooperative-multitasking.h >>>>> create mode 100644 tests/test-cooperative-multitasking.c >>>> >>>> Thanks for working on this! See some comments inline. >>> >>> Thank you for providing the counter proposal for this approach, I >>> think it will turn out much better than the original approach. >>> >>>>> >>>>> diff --git a/lib/automake.mk b/lib/automake.mk >>>>> index 0dc8a35cc..8596171c6 100644 >>>>> --- a/lib/automake.mk >>>>> +++ b/lib/automake.mk >>>>> @@ -94,6 +94,9 @@ lib_libopenvswitch_la_SOURCES = \ >>>>> lib/conntrack-other.c \ >>>>> lib/conntrack.c \ >>>>> lib/conntrack.h \ >>>>> + lib/cooperative-multitasking.c \ >>>>> + lib/cooperative-multitasking.h \ >>>>> + lib/cooperative-multitasking-private.h \ >>>>> lib/coverage.c \ >>>>> lib/coverage.h \ >>>>> lib/cpu.c \ >>>>> diff --git a/lib/cooperative-multitasking-private.h >>>>> b/lib/cooperative-multitasking-private.h >>>>> new file mode 100644 >>>>> index 000000000..b2e4e7291 >>>>> --- /dev/null >>>>> +++ b/lib/cooperative-multitasking-private.h >>>>> @@ -0,0 +1,31 @@ >>>>> +/* >>>>> + * Copyright (c) 2024 Canonical Ltd. >>>>> + * >>>>> + * Licensed under the Apache License, Version 2.0 (the "License"); >>>>> + * you may not use this file except in compliance with the License. >>>>> + * You may obtain a copy of the License at: >>>>> + * >>>>> + * http://www.apache.org/licenses/LICENSE-2.0 >>>>> + * >>>>> + * Unless required by applicable law or agreed to in writing, software >>>>> + * distributed under the License is distributed on an "AS IS" BASIS, >>>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >>>>> implied. >>>>> + * See the License for the specific language governing permissions and >>>>> + * limitations under the License. >>>>> + */ >>>>> + >>>>> +#ifndef COOPERATIVE_MULTITASKING_PRIVATE_H >>>>> +#define COOPERATIVE_MULTITASKING_PRIVATE_H 1 >>>>> + >>>>> +#include "openvswitch/hmap.h" >>>>> + >>>>> +struct cooperative_multitasking_callback { >>>>> + struct hmap_node node; >>>>> + void (*cb)(void *); >>>>> + void *arg; >>>>> + long long int time_threshold; >>>>> + long long int last_run; >>>>> + const char *msg; >>>>> +}; >>>>> + >>>>> +#endif /* COOPERATIVE_MULTITASKING_PRIVATE_H */ >>>>> diff --git a/lib/cooperative-multitasking.c >>>>> b/lib/cooperative-multitasking.c >>>>> new file mode 100644 >>>>> index 000000000..1b1205e8f >>>>> --- /dev/null >>>>> +++ b/lib/cooperative-multitasking.c >>>>> @@ -0,0 +1,195 @@ >>>>> +/* >>>>> + * Copyright (c) 2023 Canonical Ltd. >>>> >>>> 2024 ? :) >>>> >>>> Same for other files. >>> >>> I started the development before the break, so the files were actually >>> created in 2023, and we are now indeed in 2024 so it makes sense to >>> update :) >>> >>>>> + * >>>>> + * Licensed under the Apache License, Version 2.0 (the "License"); >>>>> + * you may not use this file except in compliance with the License. >>>>> + * You may obtain a copy of the License at: >>>>> + * >>>>> + * http://www.apache.org/licenses/LICENSE-2.0 >>>>> + * >>>>> + * Unless required by applicable law or agreed to in writing, software >>>>> + * distributed under the License is distributed on an "AS IS" BASIS, >>>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >>>>> implied. >>>>> + * See the License for the specific language governing permissions and >>>>> + * limitations under the License. >>>>> + */ >>>>> + >>>>> +#include <config.h> >>>>> + >>>>> +#include "backtrace.h" >>>>> +#include "cooperative-multitasking-private.h" >>>>> +#include "cooperative-multitasking.h" >>>>> +#include "hash.h" >>>>> +#include "openvswitch/hmap.h" >>>>> +#include "openvswitch/vlog.h" >>>>> +#include "timeval.h" >>>>> + >>>>> +VLOG_DEFINE_THIS_MODULE(cooperative_multitasking); >>>>> + >>>>> +static struct hmap *cooperative_multitasking_callbacks = NULL; >>>> >>>> I wonder if we can use a shorter name. You're using 'cm_callbacks' >>>> in the test, for example. It is an internal hash map after all. >>>> Probably, no need to have an extra fancy name. >>> >>> I was mostly concerned with consistent namespacing since it is a >>> "global" variable, as-is it is contained to the module as you point >>> out. >>> >>> However, if we're going to share it with the unit test through extern >>> as proposed below I suggest we keep the long name to avoid potential >>> symbol clashes? >> >> I'm not concerned much about symbol clashes, since the header must >> not be included anywhere, except for the module itself and the test >> code. The test already knows too much about internals of the module, >> so we can expect it to be aware of the internal naming. But it is >> up to you. > > Hum, I'm sure I'm doing something different than what you intended > with your comment here then, with: > lib/cooperative-multitasking.c: > struct hmap cooperative_multitasking_callbacks = HMAP_INITIALIZER( > &cooperative_multitasking_callbacks); > > lib/cooperative-multitasking-private.h > extern struct hmap cooperative_multitasking_callbacks; > > The linker will see `cooperative_multitasking_callbacks` as a global symbol: > $ nm ovsdb/ovsdb-server|grep callbacks > 0000000000245598 D cooperative_multitasking_callbacks > > and subsequently it will not be available for use in other modules, > regardless of which header is included. >
Ah, I see what you mean. I missed that it will become part of a global namespace since it's not static anymore. OK then. Still up to you. We may keep it as-is. But if there are some suggestions on how to shorten it, that would be nice. We could potentially shorten it later, when someone has a sudden epiphany on better naming. :D Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
