On Wed, Mar 23, 2016 at 12:28 PM, Ryan Moats <rmo...@us.ibm.com> wrote: > > Han Zhou <zhou...@gmail.com> wrote on 03/23/2016 01:39:04 PM: > > > From: Han Zhou <zhou...@gmail.com> > > To: Ryan Moats/Omaha/IBM@IBMUS > > Cc: "dev@openvswitch.org" <dev@openvswitch.org>, Russell Bryant > > <russ...@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 <zhou...@gmail.com> wrote: > > > > > > > > > > > > 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? > > > > > 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... >
Indeed!! I saw it when putting on my new glasses. Sorry that I missed this part at the first glance. Thanks for the great work, and we can't wait for the rebase and trying it out. -- Best regards, Han _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev