On 16/09/2021 08:08, Han Zhou wrote: > On Fri, Sep 3, 2021 at 5:22 AM Mark Gray <mark.d.g...@redhat.com> wrote: >>
Also, thanks to Han and Mark for reviewing! >> Update the runtime node to initialize and destroy some common data >> used by multiple functions in northd. > > Hi Mark, > > It looks good overall as an initiate step to employ the inc-proc engine for > ovn-northd. It is great that it achieves the no-input-change-no-compute > outcome with this series. > However, for the en-runtime node it doesn't look right. I understand it is > only for demonstration purposes and is supposed to evolve, but it may look > misleading in how would we expand it further for I-P: > > 1) I think the I-P engine should focus on data dependency. Each node should > define its data (output data computed by the node's run()/change-handlers). > The en-runtime node defines its own data as lr_list, datapaths and ports, > but the run() function merely initiates empty data structures that are > later filled by en-northd, which is wrong. Instead, en-runtime should have > its own data computed according to its input (all those NB/SB tables), and > then en-northd should use en-runtime data in a read-only fashion. It is > better not adding this node if we don't have this data dependency defined. > > 2) en-northd run() calls ovn_db_run(), which accesses the inputs (DB > tables) using the northd_context instead of the data from its input nodes. > If we really want to use the engine for incremental processing, then I > think the first step is to make sure we never access data through any > global context in engine functions, and only use input data from the engine > nodes it depends on. The engine context should only contain idl_txn for > writing to DBs (even this could be improved so that we don't have any > global ctx in engines at all, but at least that's how we are living with in > ovn-controller). Otherwise it would be hard to ensure correctness of I-P. > For example, what if in ovn_db_run() it depends on some data from the > context that is not in any of the input nodes, and now if that data > changes, the ovn_db_run() won't be triggered any more because engine inputs > not changed, and this would be a bug. > > Thanks, > Han > >> >> Signed-off-by: Mark Gray <mark.d.g...@redhat.com> >> --- >> northd/en-northd.c | 9 ++++++++- >> northd/en-runtime.c | 30 ++++++++++++++++++++++++++++-- >> northd/en-runtime.h | 8 ++++++++ >> northd/northd.c | 15 +++++---------- >> northd/northd.h | 5 ++++- >> 5 files changed, 53 insertions(+), 14 deletions(-) >> >> diff --git a/northd/en-northd.c b/northd/en-northd.c >> index d310fa4dd31f..2a3250f3d57a 100644 >> --- a/northd/en-northd.c >> +++ b/northd/en-northd.c >> @@ -19,6 +19,7 @@ >> #include <stdio.h> >> >> #include "en-northd.h" >> +#include "en-runtime.h" >> #include "lib/inc-proc-eng.h" >> #include "northd.h" >> #include "openvswitch/vlog.h" >> @@ -29,7 +30,13 @@ void en_northd_run(struct engine_node *node, void > *data OVS_UNUSED) >> { >> const struct engine_context *eng_ctx = engine_get_context(); >> struct northd_context *ctx = eng_ctx->client_ctx; >> - ovn_db_run(ctx); >> + >> + struct ed_type_runtime *runtime_data = >> + engine_get_input_data("runtime", node); >> + >> + ovn_db_run(ctx, &runtime_data->lr_list, >> + &runtime_data->datapaths, >> + &runtime_data->ports); >> >> engine_set_node_state(node, EN_UPDATED); >> >> diff --git a/northd/en-runtime.c b/northd/en-runtime.c >> index aac01cd0351f..b8e5766823bf 100644 >> --- a/northd/en-runtime.c >> +++ b/northd/en-runtime.c >> @@ -19,7 +19,9 @@ >> #include <stdio.h> >> >> #include "en-runtime.h" >> +#include "openvswitch/hmap.h" >> #include "lib/inc-proc-eng.h" >> +#include "openvswitch/list.h" >> #include "northd.h" >> #include "openvswitch/vlog.h" >> >> @@ -27,14 +29,38 @@ VLOG_DEFINE_THIS_MODULE(en_runtime); >> >> void en_runtime_run(struct engine_node *node, void *data OVS_UNUSED) >> { >> + struct ed_type_runtime *runtime_data = data; >> + >> + struct ovs_list *lr_list = &runtime_data->lr_list; >> + struct hmap *datapaths = &runtime_data->datapaths; >> + struct hmap *ports = &runtime_data->ports; >> + >> + destroy_datapaths_and_ports(datapaths, ports, lr_list); >> + >> + ovs_list_init(lr_list); >> + hmap_init(datapaths); >> + hmap_init(ports); >> + >> engine_set_node_state(node, EN_UPDATED); >> } >> void *en_runtime_init(struct engine_node *node OVS_UNUSED, >> struct engine_arg *arg OVS_UNUSED) >> { >> - return NULL; >> + struct ed_type_runtime *data = xzalloc(sizeof *data); >> + ovs_list_init(&data->lr_list); >> + hmap_init(&data->datapaths); >> + hmap_init(&data->ports); >> + >> + return data; >> } >> >> -void en_runtime_cleanup(void *data OVS_UNUSED) >> +void en_runtime_cleanup(void *data) >> { >> + struct ed_type_runtime *runtime_data = data; >> + >> + struct ovs_list *lr_list = &runtime_data->lr_list; >> + struct hmap *datapaths = &runtime_data->datapaths; >> + struct hmap *ports = &runtime_data->ports; >> + >> + destroy_datapaths_and_ports(datapaths, ports, lr_list); >> } >> diff --git a/northd/en-runtime.h b/northd/en-runtime.h >> index 2547c9ec470a..7a1b2f6873e5 100644 >> --- a/northd/en-runtime.h >> +++ b/northd/en-runtime.h >> @@ -7,7 +7,15 @@ >> #include <stdlib.h> >> #include <stdio.h> >> >> +#include "openvswitch/hmap.h" >> #include "lib/inc-proc-eng.h" >> +#include "openvswitch/list.h" >> + >> +struct ed_type_runtime { >> + struct ovs_list lr_list; >> + struct hmap datapaths; >> + struct hmap ports; >> +}; >> >> void en_runtime_run(struct engine_node *node, void *data); >> void *en_runtime_init(struct engine_node *node, >> diff --git a/northd/northd.c b/northd/northd.c >> index 829c4479f14b..43792f0d7ff7 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -13838,7 +13838,7 @@ sync_dns_entries(struct northd_context *ctx, > struct hmap *datapaths) >> hmap_destroy(&dns_map); >> } >> >> -static void >> +void >> destroy_datapaths_and_ports(struct hmap *datapaths, struct hmap *ports, >> struct ovs_list *lr_list) >> { >> @@ -14548,13 +14548,9 @@ ovnsb_db_run(struct northd_context *ctx, >> } >> >> void >> -ovn_db_run(struct northd_context *ctx) >> +ovn_db_run(struct northd_context *ctx, struct ovs_list *lr_list, >> + struct hmap *datapaths, struct hmap *ports) >> { >> - struct hmap datapaths, ports; >> - struct ovs_list lr_list; >> - ovs_list_init(&lr_list); >> - hmap_init(&datapaths); >> - hmap_init(&ports); >> use_parallel_build = ctx->use_parallel_build; >> lflow_locks = ctx->lflow_locks; >> >> @@ -14562,12 +14558,11 @@ ovn_db_run(struct northd_context *ctx) >> >> stopwatch_start(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec()); >> ovnnb_db_run(ctx, ctx->sbrec_chassis_by_name, ctx->ovnsb_idl_loop, >> - &datapaths, &ports, &lr_list, start_time, >> + datapaths, ports, lr_list, start_time, >> ctx->ovn_internal_version); >> stopwatch_stop(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec()); >> stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec()); >> - ovnsb_db_run(ctx, ctx->ovnsb_idl_loop, &ports, start_time); >> + ovnsb_db_run(ctx, ctx->ovnsb_idl_loop, ports, start_time); >> stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec()); >> - destroy_datapaths_and_ports(&datapaths, &ports, &lr_list); >> } >> >> diff --git a/northd/northd.h b/northd/northd.h >> index fa941d8ec83b..45a153ba2aa4 100644 >> --- a/northd/northd.h >> +++ b/northd/northd.h >> @@ -37,6 +37,9 @@ struct northd_context { >> const char *ovn_internal_version; >> }; >> >> -void ovn_db_run(struct northd_context *ctx); >> +void ovn_db_run(struct northd_context *ctx, struct ovs_list *, >> + struct hmap *, struct hmap *); >> +void destroy_datapaths_and_ports(struct hmap *datapaths, struct hmap > *ports, >> + struct ovs_list *lr_list); >> >> #endif /* NORTHD_H */ >> -- >> 2.27.0 >> >> _______________________________________________ >> dev mailing list >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev