Message ID | 1521744147-23451-2-git-send-email-hzhou8@ebay.com |
---|---|
State | Superseded |
Headers | show |
Series | ovn-controller Incremental Processing | expand |
Adding ML back :) On Fri, Apr 6, 2018 at 2:51 PM, Mark Michelson <mmichels@redhat.com> wrote: > > Hi Han, I'm slowly making my way through this patch series. So far, I have a pretty small finding. See below. > > > On 03/22/2018 01:42 PM, Han Zhou wrote: >> >> This patch implements the engine which will be used in future patches >> for ovn-controller incremental processing. >> --- >> ovn/lib/automake.mk | 4 +- >> ovn/lib/inc-proc-eng.c | 97 ++++++++++++++++++++++++++++++++++++++++ >> ovn/lib/inc-proc-eng.h | 118 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 218 insertions(+), 1 deletion(-) >> create mode 100644 ovn/lib/inc-proc-eng.c >> create mode 100644 ovn/lib/inc-proc-eng.h >> >> diff --git a/ovn/lib/automake.mk b/ovn/lib/automake.mk >> index 6178fc2..c1d37c5 100644 >> --- a/ovn/lib/automake.mk >> +++ b/ovn/lib/automake.mk >> @@ -17,7 +17,9 @@ ovn_lib_libovn_la_SOURCES = \ >> ovn/lib/ovn-util.c \ >> ovn/lib/ovn-util.h \ >> ovn/lib/logical-fields.c \ >> - ovn/lib/logical-fields.h >> + ovn/lib/logical-fields.h \ >> + ovn/lib/inc-proc-eng.c \ >> + ovn/lib/inc-proc-eng.h >> nodist_ovn_lib_libovn_la_SOURCES = \ >> ovn/lib/ovn-nb-idl.c \ >> ovn/lib/ovn-nb-idl.h \ >> diff --git a/ovn/lib/inc-proc-eng.c b/ovn/lib/inc-proc-eng.c >> new file mode 100644 >> index 0000000..c13a065 >> --- /dev/null >> +++ b/ovn/lib/inc-proc-eng.c >> @@ -0,0 +1,97 @@ >> +/* >> + * Copyright (c) 2018 eBay Inc. >> + * >> + * 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 <errno.h> >> +#include <getopt.h> >> +#include <signal.h> >> +#include <stdlib.h> >> +#include <string.h> >> + >> +#include "openvswitch/dynamic-string.h" >> +#include "openvswitch/hmap.h" >> +#include "openvswitch/vlog.h" >> +#include "inc-proc-eng.h" >> + >> +VLOG_DEFINE_THIS_MODULE(inc_proc_eng); >> + >> +bool engine_force_recompute = false; >> + >> +void >> +engine_run(struct engine_node *node, uint64_t run_id) >> +{ >> + if (node->run_id == run_id) { >> + return; >> + } >> + node->run_id = run_id; >> + >> + if (node->changed) { >> + node->changed = false; >> + } >> + if (!node->n_inputs) { >> + node->run(node); >> + VLOG_DBG("node: %s, changed: %d", node->name, node->changed); >> + return; >> + } >> + >> + size_t i; >> + >> + for (i = 0; i < node->n_inputs; i++) { >> + engine_run(node->inputs[i].node, run_id); >> + } >> + >> + bool need_compute = false; >> + bool need_recompute = false; >> + >> + if (engine_force_recompute) { >> + need_recompute = true; >> + } else { >> + for (i = 0; i < node->n_inputs; i++) { >> + if (node->inputs[i].node->changed) { >> + need_compute = true; >> + if (!node->inputs[i].change_handler) { >> + need_recompute = true; >> + break; >> + } >> + } >> + } >> + } >> + >> + if (need_recompute) { >> + VLOG_DBG("node: %s, recompute (%s)", node->name, >> + engine_force_recompute ? "forced" : "triggered"); >> + node->run(node); >> + } else if (need_compute) { >> + for (i = 0; i < node->n_inputs; i++) { >> + if (node->inputs[i].node->changed) { >> + VLOG_DBG("node: %s, handle change for input %s", >> + node->name, node->inputs[i].node->name); >> + if (!node->inputs[i].change_handler(node)) { >> + VLOG_DBG("node: %s, can't handle change for input %s, " >> + "fall back to recompute", >> + node->name, node->inputs[i].node->name); >> + node->run(node); >> + break; >> + } >> + } >> + } >> + } >> + >> + VLOG_DBG("node: %s, changed: %d", node->name, node->changed); >> + >> +} >> + >> diff --git a/ovn/lib/inc-proc-eng.h b/ovn/lib/inc-proc-eng.h >> new file mode 100644 >> index 0000000..99c61a1 >> --- /dev/null >> +++ b/ovn/lib/inc-proc-eng.h >> @@ -0,0 +1,118 @@ >> +/* >> + * Copyright (c) 2018 eBay Inc. >> + * >> + * 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 INC_PROC_ENG_H >> +#define INC_PROC_ENG_H 1 >> + >> +// TODO: add documentation of incremental processing engine. >> + >> +#define ENGINE_MAX_INPUT 256 >> + >> +struct engine_node; >> + >> +struct engine_node_input { >> + struct engine_node *node; >> + /* change_handler handles one input change against "old_data" of all >> + * other inputs, returns: >> + * - true: if change can be handled >> + * - false: if change cannot be handled (suggesting full recompute) >> + */ >> + bool (*change_handler)(struct engine_node *node); >> +}; >> + >> +struct engine_node { >> + uint64_t run_id; >> + char* name; >> + size_t n_inputs; >> + struct engine_node_input inputs[ENGINE_MAX_INPUT]; >> + void *data; >> + bool changed; >> + void *context; >> + void (*run)(struct engine_node *node); >> +}; >> + >> +void >> +engine_run(struct engine_node *node, uint64_t run_id); >> + >> +static inline struct engine_node * >> +engine_get_input(const char *input_name, struct engine_node *node) >> +{ >> + size_t i; >> + for (i = 0; i < node->n_inputs; i++) { >> + if (!strcmp(node->inputs[i].node->name, input_name)) { >> + return node->inputs[i].node; >> + } >> + } >> + return NULL; >> +} >> + >> +static inline void >> +engine_add_input(struct engine_node *node, struct engine_node *input, >> + bool (*change_handler)(struct engine_node *node)) >> +{ >> + node->inputs[node->n_inputs].node = input; >> + node->inputs[node->n_inputs].change_handler = change_handler; >> + node->n_inputs ++; > > > It's a bit nit-picky, but there should be bounds checking here. > Good point. I will fix it. Thanks for the review! Han
diff --git a/ovn/lib/automake.mk b/ovn/lib/automake.mk index 6178fc2..c1d37c5 100644 --- a/ovn/lib/automake.mk +++ b/ovn/lib/automake.mk @@ -17,7 +17,9 @@ ovn_lib_libovn_la_SOURCES = \ ovn/lib/ovn-util.c \ ovn/lib/ovn-util.h \ ovn/lib/logical-fields.c \ - ovn/lib/logical-fields.h + ovn/lib/logical-fields.h \ + ovn/lib/inc-proc-eng.c \ + ovn/lib/inc-proc-eng.h nodist_ovn_lib_libovn_la_SOURCES = \ ovn/lib/ovn-nb-idl.c \ ovn/lib/ovn-nb-idl.h \ diff --git a/ovn/lib/inc-proc-eng.c b/ovn/lib/inc-proc-eng.c new file mode 100644 index 0000000..c13a065 --- /dev/null +++ b/ovn/lib/inc-proc-eng.c @@ -0,0 +1,97 @@ +/* + * Copyright (c) 2018 eBay Inc. + * + * 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 <errno.h> +#include <getopt.h> +#include <signal.h> +#include <stdlib.h> +#include <string.h> + +#include "openvswitch/dynamic-string.h" +#include "openvswitch/hmap.h" +#include "openvswitch/vlog.h" +#include "inc-proc-eng.h" + +VLOG_DEFINE_THIS_MODULE(inc_proc_eng); + +bool engine_force_recompute = false; + +void +engine_run(struct engine_node *node, uint64_t run_id) +{ + if (node->run_id == run_id) { + return; + } + node->run_id = run_id; + + if (node->changed) { + node->changed = false; + } + if (!node->n_inputs) { + node->run(node); + VLOG_DBG("node: %s, changed: %d", node->name, node->changed); + return; + } + + size_t i; + + for (i = 0; i < node->n_inputs; i++) { + engine_run(node->inputs[i].node, run_id); + } + + bool need_compute = false; + bool need_recompute = false; + + if (engine_force_recompute) { + need_recompute = true; + } else { + for (i = 0; i < node->n_inputs; i++) { + if (node->inputs[i].node->changed) { + need_compute = true; + if (!node->inputs[i].change_handler) { + need_recompute = true; + break; + } + } + } + } + + if (need_recompute) { + VLOG_DBG("node: %s, recompute (%s)", node->name, + engine_force_recompute ? "forced" : "triggered"); + node->run(node); + } else if (need_compute) { + for (i = 0; i < node->n_inputs; i++) { + if (node->inputs[i].node->changed) { + VLOG_DBG("node: %s, handle change for input %s", + node->name, node->inputs[i].node->name); + if (!node->inputs[i].change_handler(node)) { + VLOG_DBG("node: %s, can't handle change for input %s, " + "fall back to recompute", + node->name, node->inputs[i].node->name); + node->run(node); + break; + } + } + } + } + + VLOG_DBG("node: %s, changed: %d", node->name, node->changed); + +} + diff --git a/ovn/lib/inc-proc-eng.h b/ovn/lib/inc-proc-eng.h new file mode 100644 index 0000000..99c61a1 --- /dev/null +++ b/ovn/lib/inc-proc-eng.h @@ -0,0 +1,118 @@ +/* + * Copyright (c) 2018 eBay Inc. + * + * 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 INC_PROC_ENG_H +#define INC_PROC_ENG_H 1 + +// TODO: add documentation of incremental processing engine. + +#define ENGINE_MAX_INPUT 256 + +struct engine_node; + +struct engine_node_input { + struct engine_node *node; + /* change_handler handles one input change against "old_data" of all + * other inputs, returns: + * - true: if change can be handled + * - false: if change cannot be handled (suggesting full recompute) + */ + bool (*change_handler)(struct engine_node *node); +}; + +struct engine_node { + uint64_t run_id; + char* name; + size_t n_inputs; + struct engine_node_input inputs[ENGINE_MAX_INPUT]; + void *data; + bool changed; + void *context; + void (*run)(struct engine_node *node); +}; + +void +engine_run(struct engine_node *node, uint64_t run_id); + +static inline struct engine_node * +engine_get_input(const char *input_name, struct engine_node *node) +{ + size_t i; + for (i = 0; i < node->n_inputs; i++) { + if (!strcmp(node->inputs[i].node->name, input_name)) { + return node->inputs[i].node; + } + } + return NULL; +} + +static inline void +engine_add_input(struct engine_node *node, struct engine_node *input, + bool (*change_handler)(struct engine_node *node)) +{ + node->inputs[node->n_inputs].node = input; + node->inputs[node->n_inputs].change_handler = change_handler; + node->n_inputs ++; +} + +extern bool engine_force_recompute; +static inline void +engine_set_force_recompute(bool val) +{ + engine_force_recompute = val; +} + +#define ENGINE_NODE(NAME, NAME_STR) \ + struct engine_node en_##NAME = { \ + .name = NAME_STR, \ + .data = &ed_##NAME, \ + .context = &ctx, \ + .run = NAME##_run, \ + }; + +#define ENGINE_FUNC_OVSDB(DB_NAME, TBL_NAME, IDL) \ +static void \ +DB_NAME##_##TBL_NAME##_run(struct engine_node *node) \ +{ \ + static bool first_run = true; \ + if (first_run) { \ + first_run = false; \ + node->changed = true; \ + return; \ + } \ + struct controller_ctx *ctx = (struct controller_ctx *)node->context; \ + if (DB_NAME##rec_##TBL_NAME##_track_get_first(IDL)) { \ + node->changed = true; \ + return; \ + } \ + node->changed = false; \ +} + +#define ENGINE_FUNC_SB(TBL_NAME) \ + ENGINE_FUNC_OVSDB(sb, TBL_NAME, ctx->ovnsb_idl) + +#define ENGINE_FUNC_OVS(TBL_NAME) \ + ENGINE_FUNC_OVSDB(ovs, TBL_NAME, ctx->ovs_idl) + +#define ENGINE_NODE_SB(TBL_NAME, TBL_NAME_STR) \ + void *ed_sb_##TBL_NAME; \ + ENGINE_NODE(sb_##TBL_NAME, TBL_NAME_STR) + +#define ENGINE_NODE_OVS(TBL_NAME, TBL_NAME_STR) \ + void *ed_ovs_##TBL_NAME; \ + ENGINE_NODE(ovs_##TBL_NAME, TBL_NAME_STR) + +#endif /* ovn/lib/inc-proc-eng.h */