On Fri, Mar 11, 2016 at 1:06 PM, Ryan Moats <rmo...@us.ibm.com> wrote:
>
> From: RYAN D. MOATS <rmo...@us.ibm.com>
>
> This code changes lflow_run to do incremental process of the
> logical flow table rather than processing the full table each run.
>
> Signed-off-by: RYAN D. MOATS <rmo...@us.ibm.com>
> ---
> ovn/controller/binding.c | 3 ++
> ovn/controller/lflow.c | 53
+++++++++++++++++++++++++++++++++------
> ovn/controller/lflow.h | 4 +-
> ovn/controller/ofctrl.c | 4 +-
> ovn/controller/ofctrl.h | 2 +
> ovn/controller/ovn-controller.c | 5 +++-
> 6 files changed, 58 insertions(+), 13 deletions(-)
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index 602a8fe..87cae99 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -15,6 +15,7 @@
>
> #include <config.h>
> #include "binding.h"
> +#include "lflow.h"
>
> #include "lib/bitmap.h"
> #include "lib/hmap.h"
> @@ -139,6 +140,7 @@ remove_local_datapath(struct hmap *local_datapaths,
unsigned int ins_seqno)
> if (ld) {
> hmap_remove(local_datapaths, &ld->hmap_node);
> hmap_remove(&local_datapaths_by_seqno, &ld->seqno_hmap_node);
> + reset_flow_processing();
> }
> }
>
> @@ -156,6 +158,7 @@ add_local_datapath(struct hmap *local_datapaths,
> hmap_insert(local_datapaths, &ld->hmap_node,
> binding_rec->datapath->tunnel_key);
> hmap_insert(&local_datapaths_by_seqno, &ld->seqno_hmap_node,
ins_seqno);
> + reset_flow_processing();
> }
>
> static void
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index 4856362..6d0d417 100644
> --- a/ovn/controller/lflow.c
> +++ b/ovn/controller/lflow.c
> @@ -176,6 +176,20 @@ struct logical_datapath {
> enum ldp_type type; /* Type of logical datapath */
> };
>
> +void reset_flow_processing(void);
> +void ldp_port_create(uint32_t ins_seqno, char *name,
> + struct logical_datapath *ldp);
> +void ldp_port_update(uint32_t ins_seqno, char *name,
> + struct logical_datapath *ldp);
> +
> +bool restart_flow_processing = false;
> +
> +void
> +reset_flow_processing(void)
> +{
> + restart_flow_processing = true;
> +}
> +
> /* Contains "struct logical_datapath"s. */
> static struct hmap logical_datapaths =
HMAP_INITIALIZER(&logical_datapaths);
>
> @@ -208,6 +222,7 @@ ldp_create(const struct sbrec_datapath_binding
*binding)
> const char *ls = smap_get(&binding->external_ids, "logical-switch");
> ldp->type = ls ? LDP_TYPE_SWITCH : LDP_TYPE_ROUTER;
> simap_init(&ldp->ports);
> + reset_flow_processing();
> return ldp;
> }
>
> @@ -224,6 +239,7 @@ ldp_free(struct logical_datapath *ldp)
> simap_destroy(&ldp->ports);
> hmap_remove(&logical_datapaths, &ldp->hmap_node);
> free(ldp);
> + reset_flow_processing();
> }
>
> /* Whether a particular port has been seen or not
> @@ -319,6 +335,7 @@ ldp_run(struct controller_ctx *ctx)
> binding->logical_port);
> if (!old || old->data != binding->tunnel_key) {
> simap_put(&ldp->ports, binding->logical_port,
binding->tunnel_key);
> + reset_flow_processing();
> }
>
> ldp_port_update(ins_seqno, binding->logical_port, ldp);
> @@ -380,13 +397,21 @@ lflow_init(void)
>
> /* Translates logical flows in the Logical_Flow table in the OVN_SB
database
> * into OpenFlow flows. See ovn-architecture(7) for more information. */
> -void
> +unsigned int
> lflow_run(struct controller_ctx *ctx,
> const struct simap *ct_zones,
> - struct hmap *local_datapaths)
> + struct hmap *local_datapaths,
> + unsigned int seqno)
> {
> struct hmap flows = HMAP_INITIALIZER(&flows);
> uint32_t conj_id_ofs = 1;
> + unsigned int processed_seqno = seqno;
> +
> + if (restart_flow_processing) {
> + seqno = 0;
> + ovn_flow_table_clear();
> + restart_flow_processing = false;
> + }
>
> ldp_run(ctx);
>
> @@ -398,17 +423,29 @@ lflow_run(struct controller_ctx *ctx,
> OVSDB_IDL_CHANGE_MODIFY);
> unsigned int ins_seqno = sbrec_logical_flow_row_get_seqno(lflow,
> OVSDB_IDL_CHANGE_INSERT);
> - // this offset is to protect the hard coded rules in physical.c
> - ins_seqno += 4;
> -
> + if (del_seqno <= seqno && mod_seqno <= seqno && ins_seqno <=
seqno) {
> + continue;
> + }
> /* if the row has a del_seqno > 0, then trying to process the
> * row isn't going to work (as it has already been freed).
> - * Therefore all we can do is to pass the ins_seqno to
> + * Therefore all we can do is to pass the offset ins_seqno to
> * ofctrl_remove_flow() to remove the flow */
> if (del_seqno > 0) {
> - ofctrl_remove_flow(ins_seqno);
> + ofctrl_remove_flow(ins_seqno+4);
> + if (del_seqno > processed_seqno) {
> + processed_seqno = del_seqno;
> + }
> continue;
> }
> + if (mod_seqno > processed_seqno) {
> + processed_seqno = mod_seqno;
> + }
> + if (ins_seqno > processed_seqno) {
> + processed_seqno = ins_seqno;
> + }
> +
> + // this offset is to protect the hard coded rules in physical.c
> + ins_seqno += 4;
>
> /* Find the "struct logical_datapath" associated with this
> * Logical_Flow row. If there's no such struct, that must be
because
> @@ -544,12 +581,12 @@ lflow_run(struct controller_ctx *ctx,
> ofpbuf_uninit(&conj);
> }
> }
> -
> /* Clean up. */
> expr_matches_destroy(&matches);
> ofpbuf_uninit(&ofpacts);
> conj_id_ofs += n_conjs;
> }
> + return processed_seqno;
> }
>
> void
> diff --git a/ovn/controller/lflow.h b/ovn/controller/lflow.h
> index e0e902c..31b187a 100644
> --- a/ovn/controller/lflow.h
> +++ b/ovn/controller/lflow.h
> @@ -56,8 +56,8 @@ struct uuid;
> #define LOG_PIPELINE_LEN 16
>
> void lflow_init(void);
> -void lflow_run(struct controller_ctx *, const struct simap *ct_zones,
> - struct hmap *local_datapaths);
> +unsigned int lflow_run(struct controller_ctx *, const struct simap
*ct_zones,
> + struct hmap *local_datapaths, unsigned int seqno);
> void lflow_destroy(void);
>
> #endif /* ovn/lflow.h */
> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> index 2479ca1..5aa7044 100644
> --- a/ovn/controller/ofctrl.c
> +++ b/ovn/controller/ofctrl.c
> @@ -106,7 +106,7 @@ static struct hmap installed_flows;
> * S_CLEAR_FLOWS or S_UPDATE_FLOWS, this is really the option we have. */
> static enum mf_field_id mff_ovn_geneve;
>
> -static void ovn_flow_table_clear(void);
> +void ovn_flow_table_clear(void);
> static void ovn_flow_table_destroy(void);
>
> static void ofctrl_recv(const struct ofp_header *, enum ofptype);
> @@ -728,7 +728,7 @@ ovn_flow_destroy(struct ovn_flow *f)
>
> /* Flow tables of struct ovn_flow. */
>
> -static void
> +void
> ovn_flow_table_clear(void)
> {
> struct ovn_flow *f, *next;
> diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
> index 4ae0d42..ff870e6 100644
> --- a/ovn/controller/ofctrl.h
> +++ b/ovn/controller/ofctrl.h
> @@ -43,4 +43,6 @@ void ofctrl_add_flow(uint8_t table_id, uint16_t
priority,
>
> void ofctrl_remove_flow(unsigned int ins_seqno);
>
> +void ovn_flow_table_clear(void);
> +
> #endif /* ovn/ofctrl.h */
> diff --git a/ovn/controller/ovn-controller.c
b/ovn/controller/ovn-controller.c
> index cb8536b..258d83e 100644
> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
> @@ -208,6 +208,7 @@ main(int argc, char *argv[])
> struct unixctl_server *unixctl;
> bool exiting;
> int retval;
> + unsigned int ovnsb_last_lflow_seqno = 0;
>
> ovs_cmdl_proctitle_init(argc, argv);
> set_program_name(argv[0]);
> @@ -303,7 +304,9 @@ main(int argc, char *argv[])
>
> pinctrl_run(&ctx, br_int);
>
> - lflow_run(&ctx, &ct_zones, &local_datapaths);
> + ovnsb_last_lflow_seqno = lflow_run(&ctx, &ct_zones,
> + &local_datapaths,
> + ovnsb_last_lflow_seqno);
> if (chassis_id) {
> physical_run(&ctx, mff_ovn_geneve,
> br_int, chassis_id, &ct_zones,
> --
> 1.7.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
I just realized a problem here. We have an optimization already in
lflow_run to process egress flows only if it belongs to local-datapath,
which was added by Russell some time ago.
With incremental lflow_run this can be a problem. Considering below
scenario:
- HV-1 has lports for lswitch-A, so local-datapath contains only lswitch-A
- Egress lflows for lswtich-B is added in SB, but ignored by HV-1 because
lswitch-B is not local.
- A lport of lswitch-B attached to HV-1. Now lswitch-B becomes local for
HV-1. But there is no egress lflow updates for lswitch-B, so HV-1 still
will not generate physical flows for those egress lflows of lswitch-B,
because of the incremental processing logic.
I don't think we should drop the local-datapath optimization. So the only
way I am thinking right now is to mark the lflows that we ignored, and when
a new lswitch becomes local, we need to recheck those lflows to see if it
belong to the newly added local-datapath. Any better ideas?
--
Best regards,
Han
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
