[ovs-dev] [RFC v2 01/10] ovn-controller: Incremental processing engine
Han Zhou
zhouhan at gmail.com
Fri Apr 6 22:00:43 UTC 2018
Adding ML back :)
On Fri, Apr 6, 2018 at 2:51 PM, Mark Michelson <mmichels at 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
More information about the dev
mailing list