[ovs-dev] [PATCH v9 06/10] Add incremental proessing to lflow_run
Ryan Moats
rmoats at us.ibm.com
Wed Mar 23 19:28:19 UTC 2016
Han Zhou <zhouhan at gmail.com> wrote on 03/23/2016 01:39:04 PM:
> From: Han Zhou <zhouhan at gmail.com>
> To: Ryan Moats/Omaha/IBM at IBMUS
> Cc: "dev at openvswitch.org" <dev at openvswitch.org>, Russell Bryant
> <russell at ovn.org>
> Date: 03/23/2016 01:39 PM
> Subject: Re: [ovs-dev] [PATCH v9 06/10] Add incremental proessing
tolflow_run
>
>
>
> On Tue, Mar 22, 2016 at 6:17 PM, Han Zhou <zhouhan at gmail.com> wrote:
> >
> >
> >
> > On Fri, Mar 11, 2016 at 1:06 PM, Ryan Moats <rmoats at us.ibm.com> wrote:
> > >
> > > From: RYAN D. MOATS <rmoats at 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 <rmoats at 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 at 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?
> >
> Marking the lflows that we ignored requires maintaining a lflow
> cache. A simpler (but less optimized) approach would be resetting
> the seqno when a new datapath is added to local_datapath so that we
> can fall back to reprocess all lflows. This is just the idea, but I
> haven't worked out all details. Ryan, what do you think?
Sorry, I missed this yesterday (still fighting with this rebase) - right
now the
code should do precisely what you suggest - reset the logical flow
processing
whenever anything that is an input to it changes...
Ryan
More information about the dev
mailing list