[ovs-dev] [PATCH v4 2/3] [ovn-controller] Add logical flow incremental processing

Ryan Moats rmoats at us.ibm.com
Tue Feb 16 22:54:27 UTC 2016


While this patch passes the ovn test suite, it is not working correctly, so
consider this is "Self-NACK"
and I'll work on getting it fixed tomorrow...

Ryan

Ryan Moats/Omaha/IBM at IBMUS wrote on 02/16/2016 02:30:20 PM:

> From: Ryan Moats/Omaha/IBM at IBMUS
> To: dev at openvswitch.org
> Cc: russell at ovn.org, Ryan Moats/Omaha/IBM at IBMUS
> Date: 02/16/2016 02:30 PM
> Subject: [PATCH v4 2/3] [ovn-controller] Add logical flow
> incremental processing
>
> From: RYAN D. MOATS <rmoats at us.ibm.com>
>
> Make flow table persistent in ovn controller
>
> This is a prerequisite for incremental processing.
>
> Signed-off-by: RYAN D. MOATS <rmoats at us.ibm.com>
> ---
>  ovn/controller/ofctrl.c         |   99 +++++++++++++++++++++++++++
> +-----------
>  ovn/controller/ofctrl.h         |    2 +
>  ovn/controller/ovn-controller.c |    4 +-
>  3 files changed, 75 insertions(+), 30 deletions(-)
>
> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> index 3297fb3..e5f4ebf 100644
> --- a/ovn/controller/ofctrl.c
> +++ b/ovn/controller/ofctrl.c
> @@ -50,6 +50,8 @@ struct ovn_flow {
>  static uint32_t ovn_flow_hash(const struct ovn_flow *);
>  static struct ovn_flow *ovn_flow_lookup(struct hmap *flow_table,
>                                          const struct ovn_flow *target);
> +static struct ovn_flow *ovn_flow_lookup2(struct hmap *flow_table,
> +                                         const struct ovn_flow *target);
>  static char *ovn_flow_to_string(const struct ovn_flow *);
>  static void ovn_flow_log(const struct ovn_flow *, const char *action);
>  static void ovn_flow_destroy(struct ovn_flow *);
> @@ -484,21 +486,41 @@ ofctrl_add_flow(struct hmap *desired_flows,
>      f->ofpacts_len = actions->size;
>      f->hmap_node.hash = ovn_flow_hash(f);
>
> -    if (ovn_flow_lookup(desired_flows, f)) {
> -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> -        if (!VLOG_DROP_INFO(&rl)) {
> -            char *s = ovn_flow_to_string(f);
> -            VLOG_INFO("dropping duplicate flow: %s", s);
> -            free(s);
> -        }
> +    struct ovn_flow *d = ovn_flow_lookup2(desired_flows, f);
> +    if (d) {
> +        if (!match_equal(&d->match, &f->match)) {
>
> -        ovn_flow_destroy(f);
> -        return;
> +            hmap_remove(desired_flows, &d->hmap_node);
> +            ovn_flow_destroy(d);
> +        } else {
> +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
5);
> +            if (!VLOG_DROP_INFO(&rl)) {
> +                char *s = ovn_flow_to_string(f);
> +                VLOG_INFO("dropping duplicate flow: %s", s);
> +                free(s);
> +            }
> +
> +            ovn_flow_destroy(f);
> +            return;
> +        }
>      }
>
>      hmap_insert(desired_flows, &f->hmap_node, f->hmap_node.hash);
>  }
>
> +/* duplicate an ovn_flow structure */
> +struct ovn_flow *ofctrl_dup_flow(struct ovn_flow *source)
> +{
> +    struct ovn_flow *answer = xmalloc(sizeof *answer);
> +    answer->table_id = source->table_id;
> +    answer->priority = source->priority;
> +    answer->match = source->match;
> +    answer->ofpacts = xmemdup(source->ofpacts, source->ofpacts_len);
> +    answer->ofpacts_len = source->ofpacts_len;
> +    answer->hmap_node.hash = ovn_flow_hash(answer);
> +    return answer;
> +}
> +
>  /* ovn_flow. */
>
>  /* Returns a hash of the key in 'f'. */
> @@ -528,6 +550,25 @@ ovn_flow_lookup(struct hmap *flow_table, const
> struct ovn_flow *target)
>      return NULL;
>  }
>
> +/* Finds and returns an ovn_flow in 'flow_table" whose table_id,
priority
> + * and actions are the same as the target, or NULL if there is none */
> +static struct ovn_flow *
> +ovn_flow_lookup2(struct hmap *flow_table, const struct ovn_flow *target)
> +{
> +    struct ovn_flow *f;
> +
> +    HMAP_FOR_EACH_WITH_HASH (f, hmap_node, target->hmap_node.hash,
> +                             flow_table) {
> +        if (f->table_id == target->table_id
> +            && f->priority == target->priority
> +            && ofpacts_equal(f->ofpacts, f->ofpacts_len,
> +                             target->ofpacts, target->ofpacts_len)) {
> +            return f;
> +        }
> +    }
> +    return NULL;
> +}
> +
>  static char *
>  ovn_flow_to_string(const struct ovn_flow *f)
>  {
> @@ -607,7 +648,6 @@ ofctrl_put(struct hmap *flow_table)
>       * to wake up and retry. */
>      if (state != S_UPDATE_FLOWS
>          || rconn_packet_counter_n_packets(tx_counter)) {
> -        ovn_flow_table_clear(flow_table);
>          return;
>      }
>
> @@ -659,25 +699,28 @@ ofctrl_put(struct hmap *flow_table)
>          }
>      }
>
> -    /* The previous loop removed from 'flow_table' all of the flows that
are
> -     * already installed.  Thus, any flows remaining in 'flow_table'
need to
> -     * be added to the flow table. */
> +    /* Since flow table has all flows, now we need to add the ones that
> +     * aren't in the installed flow table. */
>      struct ovn_flow *d;
>      HMAP_FOR_EACH_SAFE (d, next, hmap_node, flow_table) {
> -        /* Send flow_mod to add flow. */
> -        struct ofputil_flow_mod fm = {
> -            .match = d->match,
> -            .priority = d->priority,
> -            .table_id = d->table_id,
> -            .ofpacts = d->ofpacts,
> -            .ofpacts_len = d->ofpacts_len,
> -            .command = OFPFC_ADD,
> -        };
> -        queue_flow_mod(&fm);
> -        ovn_flow_log(d, "adding");
> -
> -        /* Move 'd' from 'flow_table' to installed_flows. */
> -        hmap_remove(flow_table, &d->hmap_node);
> -        hmap_insert(&installed_flows, &d->hmap_node, d->hmap_node.hash);
> +        struct ovn_flow *i = ovn_flow_lookup(&installed_flows, d);
> +        if (!i) {
> +            /* Send flow_mod to add flow. */
> +            struct ofputil_flow_mod fm = {
> +                .match = d->match,
> +                .priority = d->priority,
> +                .table_id = d->table_id,
> +                .ofpacts = d->ofpacts,
> +                .ofpacts_len = d->ofpacts_len,
> +                .command = OFPFC_ADD,
> +            };
> +            queue_flow_mod(&fm);
> +            ovn_flow_log(d, "adding");
> +
> +            /* Copy 'd' from 'flow_table' to installed_flows. */
> +            struct ovn_flow *new_node = ofctrl_dup_flow(d);
> +            hmap_insert(&installed_flows, &new_node->hmap_node,
> +                        new_node->hmap_node.hash);
> +        }
>      }
>  }
> diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
> index 93ef8ea..d3fabc0 100644
> --- a/ovn/controller/ofctrl.h
> +++ b/ovn/controller/ofctrl.h
> @@ -34,6 +34,8 @@ void ofctrl_put(struct hmap *flows);
>  void ofctrl_wait(void);
>  void ofctrl_destroy(void);
>
> +struct ovn_flow *ofctrl_dup_flow(struct ovn_flow *source);
> +
>  /* Flow table interface to the rest of ovn-controller. */
>  void ofctrl_add_flow(struct hmap *flows, uint8_t table_id,
uint16_tpriority,
>                       const struct match *, const struct ofpbuf
*ofpacts);
> diff --git a/ovn/controller/ovn-controller.c
b/ovn/controller/ovn-controller.c
> index 3638342..5a4174e 100644
> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
> @@ -205,6 +205,8 @@ main(int argc, char *argv[])
>      bool exiting;
>      int retval;
>
> +    struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
> +
>      ovs_cmdl_proctitle_init(argc, argv);
>      set_program_name(argv[0]);
>      service_start(&argc, &argv);
> @@ -299,14 +301,12 @@ main(int argc, char *argv[])
>
>              pinctrl_run(&ctx, br_int);
>
> -            struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
>              lflow_run(&ctx, &flow_table, &ct_zones, &local_datapaths);
>              if (chassis_id) {
>                  physical_run(&ctx, mff_ovn_geneve,
>                               br_int, chassis_id, &ct_zones,
&flow_table);
>              }
>              ofctrl_put(&flow_table);
> -            hmap_destroy(&flow_table);
>          }
>
>          /* local_datapaths contains bare hmap_node instances.
> --
> 1.7.1
>



More information about the dev mailing list