[ovs-dev] [PATCH v3] ovn-controller: Persist desired conntrack groups.
Flaviof
flavio at flaviof.com
Fri Jul 29 04:39:00 UTC 2016
On Thu, Jul 28, 2016 at 5:17 PM, Ryan Moats <rmoats at us.ibm.com> wrote:
> With incremental processing of logical flows desired conntrack groups
> are not being persisted. This patch adds this capability, with the
> side effect of adding a ds_clone method that this capability leverages.
>
> Signed-off-by: Ryan Moats <rmoats at us.ibm.com>
> Reported-by: Guru Shetty <guru at ovn.org>
> Reported-at: http://openvswitch.org/pipermail/dev/2016-July/076320.html
> Fixes: 70c7cfe ("ovn-controller: Add incremental processing to lflow_run
> and physical_run")
> ---
> include/openvswitch/dynamic-string.h | 1 +
> include/ovn/actions.h | 6 +++++
> lib/dynamic-string.c | 9 ++++++++
> ovn/controller/lflow.c | 2 ++
> ovn/controller/ofctrl.c | 43
> ++++++++++++++++++++++++------------
> ovn/controller/ofctrl.h | 5 ++++-
> ovn/controller/ovn-controller.c | 2 +-
> ovn/lib/actions.c | 1 +
> 8 files changed, 53 insertions(+), 16 deletions(-)
>
> diff --git a/include/openvswitch/dynamic-string.h
> b/include/openvswitch/dynamic-string.h
> index dfe2688..bf1f64a 100644
> --- a/include/openvswitch/dynamic-string.h
> +++ b/include/openvswitch/dynamic-string.h
> @@ -73,6 +73,7 @@ void ds_swap(struct ds *, struct ds *);
>
> int ds_last(const struct ds *);
> bool ds_chomp(struct ds *, int c);
> +void ds_clone(struct ds *, struct ds *);
>
> /* Inline functions. */
>
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 114c71e..55720ce 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -22,7 +22,9 @@
> #include "compiler.h"
> #include "openvswitch/hmap.h"
> #include "openvswitch/dynamic-string.h"
> +#include "openvswitch/uuid.h"
> #include "util.h"
> +#include "uuid.h"
>
Isn't #include "openvswitch/uuid.h" enough?
Consider not doing #include "uuid.h"
>
> struct expr;
> struct lexer;
> @@ -43,6 +45,7 @@ struct group_table {
> struct group_info {
> struct hmap_node hmap_node;
> struct ds group;
> + struct uuid lflow_uuid;
> uint32_t group_id;
> };
>
> @@ -107,6 +110,9 @@ struct action_params {
> /* A struct to figure out the group_id for group actions. */
> struct group_table *group_table;
>
> + /* The logical flow uuid that drove this action. */
> + struct uuid lflow_uuid;
> +
> /* OVN maps each logical flow table (ltable), one-to-one, onto a
> physical
> * OpenFlow flow table (ptable). A number of parameters describe this
> * mapping and data related to flow tables:
> diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c
> index 1f17a9f..6f7b610 100644
> --- a/lib/dynamic-string.c
> +++ b/lib/dynamic-string.c
> @@ -456,3 +456,12 @@ ds_chomp(struct ds *ds, int c)
> return false;
> }
> }
> +
> +void
> +ds_clone(struct ds *dst, struct ds *source)
> +{
> + dst->length = source->length;
> + dst->allocated = dst->length;
> + dst->string = xmalloc(dst->allocated + 1);
> + memcpy(dst->string, source->string, dst->allocated + 1);
> +}
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index a4f3322..e38c32a 100644
> --- a/ovn/controller/lflow.c
> +++ b/ovn/controller/lflow.c
> @@ -383,6 +383,7 @@ add_logical_flows(struct controller_ctx *ctx, const
> struct lport_index *lports,
>
> if (full_flow_processing) {
> ovn_flow_table_clear();
> + ovn_group_table_clear(group_table, false);
> full_logical_flow_processing = true;
> full_neighbor_flow_processing = true;
> full_flow_processing = false;
> @@ -515,6 +516,7 @@ consider_logical_flow(const struct lport_index *lports,
> .aux = &aux,
> .ct_zones = ct_zones,
> .group_table = group_table,
> + .lflow_uuid = lflow->header_.uuid,
>
> .n_tables = LOG_PIPELINE_LEN,
> .first_ptable = first_ptable,
> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> index dd9f5ec..54bea99 100644
> --- a/ovn/controller/ofctrl.c
> +++ b/ovn/controller/ofctrl.c
>
shouldn't we also need to call
ovn_group_table_clear(groups, true);
from existing function ovn_flow_table_destroy(void) ?
Besides these, lgtm....
Acked-by: Flavio Fernandes <flavio at flaviof.com>
> @@ -140,8 +140,6 @@ static ovs_be32 queue_msg(struct ofpbuf *);
> static void ovn_flow_table_destroy(void);
> static struct ofpbuf *encode_flow_mod(struct ofputil_flow_mod *);
>
> -static void ovn_group_table_clear(struct group_table *group_table,
> - bool existing);
> static struct ofpbuf *encode_group_mod(const struct ofputil_group_mod *);
>
> static void ofctrl_recv(const struct ofp_header *, enum ofptype);
> @@ -150,12 +148,15 @@ static struct hmap match_flow_table =
> HMAP_INITIALIZER(&match_flow_table);
> static struct hindex uuid_flow_table =
> HINDEX_INITIALIZER(&uuid_flow_table);
>
> void
> -ofctrl_init(void)
> +ofctrl_init(struct group_table *group_table)
> {
> swconn = rconn_create(5, 0, DSCP_DEFAULT, 1 << OFP13_VERSION);
> tx_counter = rconn_packet_counter_create();
> hmap_init(&installed_flows);
> ovs_list_init(&flow_updates);
> + if (!groups) {
> + groups = group_table;
> + }
> }
>
> /* S_NEW, for a new connection.
> @@ -680,6 +681,16 @@ ofctrl_remove_flows(const struct uuid *uuid)
> ovn_flow_destroy(f);
> }
> }
> +
> + /* Remove any group_info information created by this logical flow. */
> + struct group_info *g, *next_g;
> + HMAP_FOR_EACH_SAFE (g, next_g, hmap_node, &groups->desired_groups) {
> + if (uuid_equals(&g->lflow_uuid, uuid)) {
> + hmap_remove(&groups->desired_groups, &g->hmap_node);
> + ds_destroy(&g->group);
> + free(g);
> + }
> + }
> }
>
> /* Shortcut to remove all flows matching the supplied UUID and add this
> @@ -833,6 +844,17 @@ add_flow_mod(struct ofputil_flow_mod *fm, struct
> ovs_list *msgs)
>
> /* group_table. */
>
> +static struct group_info *
> +group_info_clone(struct group_info *source)
> +{
> + struct group_info *clone = xmalloc(sizeof *clone);
> + ds_clone(&clone->group, &source->group);
> + clone->group_id = source->group_id;
> + clone->lflow_uuid = source->lflow_uuid;
> + clone->hmap_node.hash = source->hmap_node.hash;
> + return clone;
> +}
> +
> /* Finds and returns a group_info in 'existing_groups' whose key is
> identical
> * to 'target''s key, or NULL if there is none. */
> static struct group_info *
> @@ -851,7 +873,7 @@ ovn_group_lookup(struct hmap *exisiting_groups,
> }
>
> /* Clear either desired_groups or existing_groups in group_table. */
> -static void
> +void
> ovn_group_table_clear(struct group_table *group_table, bool existing)
> {
> struct group_info *g, *next;
> @@ -894,10 +916,6 @@ add_group_mod(const struct ofputil_group_mod *gm,
> struct ovs_list *msgs)
> void
> ofctrl_put(struct group_table *group_table, int64_t nb_cfg)
> {
> - if (!groups) {
> - groups = group_table;
> - }
> -
> /* The flow table can be updated if the connection to the switch is
> up and
> * in the correct state and not backlogged with existing flow_mods.
> (Our
> * criteria for being backlogged appear very conservative, but the
> socket
> @@ -1066,13 +1084,10 @@ ofctrl_put(struct group_table *group_table,
> int64_t nb_cfg)
> /* Move the contents of desired_groups to existing_groups. */
> HMAP_FOR_EACH_SAFE(desired, next_group, hmap_node,
> &group_table->desired_groups) {
> - hmap_remove(&group_table->desired_groups, &desired->hmap_node);
> if (!ovn_group_lookup(&group_table->existing_groups, desired)) {
> - hmap_insert(&group_table->existing_groups,
> &desired->hmap_node,
> - desired->hmap_node.hash);
> - } else {
> - ds_destroy(&desired->group);
> - free(desired);
> + struct group_info *clone = group_info_clone(desired);
> + hmap_insert(&group_table->existing_groups, &clone->hmap_node,
> + clone->hmap_node.hash);
> }
> }
>
> diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
> index befae01..da4ebb4 100644
> --- a/ovn/controller/ofctrl.h
> +++ b/ovn/controller/ofctrl.h
> @@ -30,7 +30,7 @@ struct ovsrec_bridge;
> struct group_table;
>
> /* Interface for OVN main loop. */
> -void ofctrl_init(void);
> +void ofctrl_init(struct group_table *group_table);
> enum mf_field_id ofctrl_run(const struct ovsrec_bridge *br_int);
> void ofctrl_put(struct group_table *group_table, int64_t nb_cfg);
> void ofctrl_wait(void);
> @@ -54,4 +54,7 @@ void ofctrl_flow_table_clear(void);
>
> void ovn_flow_table_clear(void);
>
> +void ovn_group_table_clear(struct group_table *group_table,
> + bool existing);
> +
> #endif /* ovn/ofctrl.h */
> diff --git a/ovn/controller/ovn-controller.c
> b/ovn/controller/ovn-controller.c
> index 5c74186..3cdfcdf 100644
> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
> @@ -347,7 +347,7 @@ main(int argc, char *argv[])
> ovsrec_init();
> sbrec_init();
>
> - ofctrl_init();
> + ofctrl_init(&group_table);
> pinctrl_init();
> lflow_init();
>
> diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> index fd5a867..aef5c75 100644
> --- a/ovn/lib/actions.c
> +++ b/ovn/lib/actions.c
> @@ -761,6 +761,7 @@ parse_ct_lb_action(struct action_context *ctx)
> group_info = xmalloc(sizeof *group_info);
> group_info->group = ds;
> group_info->group_id = group_id;
> + group_info->lflow_uuid = ctx->ap->lflow_uuid;
> group_info->hmap_node.hash = hash;
>
> hmap_insert(&ctx->ap->group_table->desired_groups,
> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
More information about the dev
mailing list