Adding ML back :) On Fri, Apr 6, 2018 at 2:51 PM, Mark Michelson <[email protected]> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
