On Thu, Jun 27, 2019 at 6:15 PM <[email protected]> wrote: > > From: Numan Siddique <[email protected]> > > If the ovn-controller main loop takes more than 5 seconds (if there are lots > of logical > flows) before it calls poll_block(), it causes the poll_block to wake up > immediately, > since rconn module has to send echo request. With the incremental processing, > this is > not an issue as ovn-controller will not recompute again. But for older > versions, this > is an issue as it causes flow recomputations and this would result in 100% > cpu all the > time. > > With this patch, CMS can configure a higher value depending the workload. > > The main intention of this patch is to fix this recompuation issue for older > versions > (there by requesting backport), it still would be beneficial with the > incremental processing engine. > > Signed-off-by: Numan Siddique <[email protected]>
Hi Numan, I tested it on my setup and it works fine. One minor comment inline otherwise the code LGTM. Tested-by: Dumitru Ceara <[email protected]> Thanks, Dumitru > --- > ovn/controller/ofctrl.c | 14 ++++++++++++-- > ovn/controller/ofctrl.h | 4 +++- > ovn/controller/ovn-controller.8.xml | 14 ++++++++++++++ > ovn/controller/ovn-controller.c | 12 +++++++++++- > 4 files changed, 40 insertions(+), 4 deletions(-) > > diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c > index 47a915aea..043abd69d 100644 > --- a/ovn/controller/ofctrl.c > +++ b/ovn/controller/ofctrl.c > @@ -169,9 +169,11 @@ static void ofctrl_recv(const struct ofp_header *, enum > ofptype); > > void > ofctrl_init(struct ovn_extend_table *group_table, > - struct ovn_extend_table *meter_table) > + struct ovn_extend_table *meter_table, > + int inactivity_probe_interval) > { > - swconn = rconn_create(5, 0, DSCP_DEFAULT, 1 << OFP13_VERSION); > + swconn = rconn_create(inactivity_probe_interval, 0, > + DSCP_DEFAULT, 1 << OFP13_VERSION); > tx_counter = rconn_packet_counter_create(); > hmap_init(&installed_flows); > ovs_list_init(&flow_updates); > @@ -1381,3 +1383,11 @@ ofctrl_is_connected(void) > { > return rconn_is_connected(swconn); > } > + > +void > +ofctrl_set_probe_interval(int probe_interval) > +{ > + if (swconn) { > + rconn_set_probe_interval(swconn, probe_interval); > + } > +} > diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h > index b39cdf88b..ed8918aae 100644 > --- a/ovn/controller/ofctrl.h > +++ b/ovn/controller/ofctrl.h > @@ -41,7 +41,8 @@ struct ovn_desired_flow_table { > > /* Interface for OVN main loop. */ > void ofctrl_init(struct ovn_extend_table *group_table, > - struct ovn_extend_table *meter_table); > + struct ovn_extend_table *meter_table, > + int inactivity_probe_interval); > void ofctrl_run(const struct ovsrec_bridge *br_int, > struct shash *pending_ct_zones); > enum mf_field_id ofctrl_get_mf_field_id(void); > @@ -81,5 +82,6 @@ void ofctrl_check_and_add_flow(struct > ovn_desired_flow_table *, > const struct uuid *, bool log_duplicate_flow); > > bool ofctrl_is_connected(void); > +void ofctrl_set_probe_interval(int probe_interval); > > #endif /* ovn/ofctrl.h */ > diff --git a/ovn/controller/ovn-controller.8.xml > b/ovn/controller/ovn-controller.8.xml > index 9721d9a5b..8f9c64838 100644 > --- a/ovn/controller/ovn-controller.8.xml > +++ b/ovn/controller/ovn-controller.8.xml > @@ -112,6 +112,20 @@ > </p> > </dd> > > + <dt><code>external_ids:ovn-openflow-probe-interval</code></dt> > + <dd> > + <p> > + The inactivity probe interval of the OpenFlow connection to the > + OpenvSwitch integration bridge, in seconds. > + If the value is zero, it disables the connection keepalive feature. > + </p> > + > + <p> > + If the value is nonzero, then it will be forced to a value of > + at least 5s. > + </p> > + </dd> > + > <dt><code>external_ids:ovn-encap-type</code></dt> > <dd> > <p> > diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c > index 60190161f..4e8b261e8 100644 > --- a/ovn/controller/ovn-controller.c > +++ b/ovn/controller/ovn-controller.c > @@ -390,6 +390,14 @@ update_ssl_config(const struct ovsrec_ssl_table > *ssl_table) > } > } > > +static int > +get_ofctrl_probe_interval(struct ovsdb_idl *ovs_idl) > +{ > + const struct ovsrec_open_vswitch *cfg = > ovsrec_open_vswitch_first(ovs_idl); > + return smap_get_int(&cfg->external_ids, > + "ovn-openflow-probe-interval", 5); Should we use a define for the default value? I see below that for the SB DB probes we use DEFAULT_PROBE_INTERVAL_MSEC which also suggests the unit. > +} > + > /* Retrieves the pointer to the OVN Southbound database from 'ovs_idl' and > * updates 'sbdb_idl' with that pointer. */ > static void > @@ -1817,7 +1825,8 @@ main(int argc, char *argv[]) > engine_init(&en_flow_output); > > ofctrl_init(&ed_flow_output.group_table, > - &ed_flow_output.meter_table); > + &ed_flow_output.meter_table, > + get_ofctrl_probe_interval(ovs_idl_loop.idl)); > > unixctl_command_register("group-table-list", "", 0, 0, > group_table_list, &ed_flow_output.group_table); > @@ -1844,6 +1853,7 @@ main(int argc, char *argv[]) > while (!exiting) { > update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl); > update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl)); > + > ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl)); > old_engine_run_id = engine_run_id; > > struct ovsdb_idl_txn *ovs_idl_txn = > ovsdb_idl_loop_run(&ovs_idl_loop); > -- > 2.21.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
