Adding ML back :)

On Fri, Apr 6, 2018 at 2:51 PM, Mark Michelson <mmich...@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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to