Flaviof <fla...@flaviof.com> wrote on 07/28/2016 11:39:00 PM:
> From: Flaviof <fla...@flaviof.com>
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: ovs dev <dev@openvswitch.org>
> Date: 07/28/2016 11:39 PM
> Subject: Re: [ovs-dev] [PATCH v3] ovn-controller: Persist desired
> conntrack groups.
>
> On Thu, Jul 28, 2016 at 5:17 PM, Ryan Moats <rmo...@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 <rmo...@us.ibm.com>
> Reported-by: Guru Shetty <g...@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"
Whoever pushes this, please consider it removed :)
> 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) ?
The existing ones are picked up from the desired ones,
so that's not necessary.
Ryan
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
