Message ID | 20210903122148.826196-8-mark.d.gray@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | northd: Split northd and northd incremental processing framework | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Fri, Sep 3, 2021 at 5:22 AM Mark Gray <mark.d.gray@redhat.com> wrote: > > 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.gray@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 16/09/2021 08:08, Han Zhou wrote: > On Fri, Sep 3, 2021 at 5:22 AM Mark Gray <mark.d.gray@redhat.com> wrote: >> >> 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: Specific comments below. However, I think the rework would be significant enough that it will be difficult to include before code freeze. Therefore, if the maintainers are OK with it, I request only that the first two patches in the series get merged - the ones that refactor northd in preparation for this. I will continue working on I-P in the interim. If this is OK, I will send out a v5 that rebases this series only includes these? > > 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. Yes, I completely agree with this and this was my longer-term intention. > > 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. Yes, I agree with this. I think in the case of this "northd" node, the northd_context can be initialized by the "northd" node and used by ovn_db_run as all it really contains are references to the IDL. However, I will confirm all this. > > Thanks, > Han > >> >> Signed-off-by: Mark Gray <mark.d.gray@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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 16/09/2021 08:08, Han Zhou wrote: > On Fri, Sep 3, 2021 at 5:22 AM Mark Gray <mark.d.gray@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.gray@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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 16/09/2021 08:08, Han Zhou wrote: > On Fri, Sep 3, 2021 at 5:22 AM Mark Gray <mark.d.gray@redhat.com> wrote: >> >> 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 for this feedback. I have posted a new series. I split it differently into two nodes - en_northd, and en_flow. en_northd does everything but generates the lflows and, crucially (as you pointed out), only depends on the output data from en_northd. > > Thanks, > Han > >> >> Signed-off-by: Mark Gray <mark.d.gray@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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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 */
Update the runtime node to initialize and destroy some common data used by multiple functions in northd. Signed-off-by: Mark Gray <mark.d.gray@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(-)