On 16/09/2021 23:16, Lorenzo Bianconi wrote: > Dump datapath meter capabilities before configuring meters in > ovn-controller
Thanks. Generally OK with this with one suggestion below. I'm not sure if it will work but I think it follows the typical ovn/ovn pattern which uses _run() functions in these loops. > > Signed-off-by: Lorenzo Bianconi <[email protected]> > --- > Changes since v3: > - add missing rconn_run_wait() and rconn_recv_wait() at the end of swconn for > loop > - cosmetics > Changes since v2: > - move meter capability logic in lib/features.c > Changes since v1: > - move rconn in ovn-controller to avoid concurrency issues > --- > controller/ovn-controller.c | 4 ++ > include/ovn/features.h | 4 ++ > lib/actions.c | 3 ++ > lib/automake.mk | 1 + > lib/features.c | 90 +++++++++++++++++++++++++++++++++++++ > tests/ovn.at | 4 +- > 6 files changed, 104 insertions(+), 2 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 0031a1035..22cb4b956 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -3523,6 +3523,9 @@ main(int argc, char *argv[]) > ovsrec_server_has_datapath_table(ovs_idl_loop.idl) > ? &br_int_dp > : NULL); > + if (br_int) { > + ovs_feature_support_init(br_int->name); > + } > > /* Enable ACL matching for double tagged traffic. */ > if (ovs_idl_txn) { > @@ -3898,6 +3901,7 @@ loop_done: > ovsdb_idl_loop_destroy(&ovs_idl_loop); > ovsdb_idl_loop_destroy(&ovnsb_idl_loop); > > + ovs_feature_support_deinit(); > free(ovs_remote); > service_stop(); > > diff --git a/include/ovn/features.h b/include/ovn/features.h > index c35d59b14..5259f2c69 100644 > --- a/include/ovn/features.h > +++ b/include/ovn/features.h > @@ -28,12 +28,16 @@ > */ > enum ovs_feature_support_bits { > OVS_CT_ZERO_SNAT_SUPPORT_BIT, > + OVS_DP_METER_SUPPORT_BIT, > }; > > enum ovs_feature_value { > OVS_CT_ZERO_SNAT_SUPPORT = (1 << OVS_CT_ZERO_SNAT_SUPPORT_BIT), > + OVS_DP_METER_SUPPORT = (1 << OVS_DP_METER_SUPPORT_BIT), > }; > > +void ovs_feature_support_init(const char *br_name); > +void ovs_feature_support_deinit(void); > bool ovs_feature_is_supported(enum ovs_feature_value feature); > bool ovs_feature_support_update(const struct smap *ovs_capabilities); > > diff --git a/lib/actions.c b/lib/actions.c > index c572e88ae..7cf6be308 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -87,6 +87,9 @@ encode_start_controller_op(enum action_opcode opcode, bool > pause, > oc->max_len = UINT16_MAX; > oc->reason = OFPR_ACTION; > oc->pause = pause; > + if (!ovs_feature_is_supported(OVS_DP_METER_SUPPORT)) { > + meter_id = NX_CTLR_NO_METER; > + } > oc->meter_id = meter_id; > > struct action_header ah = { .opcode = htonl(opcode) }; > diff --git a/lib/automake.mk b/lib/automake.mk > index ddfe33948..9f9f447d5 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > @@ -2,6 +2,7 @@ lib_LTLIBRARIES += lib/libovn.la > lib_libovn_la_LDFLAGS = \ > $(OVS_LTINFO) \ > -Wl,--version-script=$(top_builddir)/lib/libovn.sym \ > + $(OVS_LIBDIR)/libopenvswitch.la \ > $(AM_LDFLAGS) > lib_libovn_la_SOURCES = \ > lib/acl-log.c \ > diff --git a/lib/features.c b/lib/features.c > index fddf4c450..decb4d24c 100644 > --- a/lib/features.c > +++ b/lib/features.c > @@ -18,7 +18,14 @@ > #include <stdlib.h> > > #include "lib/util.h" > +#include "lib/dirs.h" > +#include "socket-util.h" > +#include "lib/vswitch-idl.h" > #include "openvswitch/vlog.h" > +#include "openvswitch/ofpbuf.h" > +#include "openvswitch/rconn.h" > +#include "openvswitch/ofp-msgs.h" > +#include "openvswitch/ofp-meter.h" > #include "ovn/features.h" > > VLOG_DEFINE_THIS_MODULE(features); > @@ -40,11 +47,15 @@ static uint32_t supported_ovs_features; > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); > > +/* ovs-vswitchd connection. */ > +static struct rconn *swconn; > + > static bool > ovs_feature_is_valid(enum ovs_feature_value feature) > { > switch (feature) { > case OVS_CT_ZERO_SNAT_SUPPORT: > + case OVS_DP_METER_SUPPORT: > return true; > default: > return false; > @@ -58,6 +69,81 @@ ovs_feature_is_supported(enum ovs_feature_value feature) > return supported_ovs_features & feature; > } > > +static bool > +ovs_feature_get_openflow_cap(void) > +{ > + if (!swconn) { > + return false; > + } > + > + rconn_run(swconn); > + if (!rconn_is_connected(swconn)) { > + return false; > + } > + > + bool ret = false; > + /* dump datapath meter capabilities. */ > + struct ofpbuf *msg = ofpraw_alloc(OFPRAW_OFPST13_METER_FEATURES_REQUEST, > + rconn_get_version(swconn), 0); > + rconn_send(swconn, msg, NULL); > + for (int i = 0; i < 50; i++) { > + msg = rconn_recv(swconn); > + if (!msg) { > + break; > + } > + > + const struct ofp_header *oh = msg->data; > + enum ofptype type; > + ofptype_decode(&type, oh); > + > + if (type == OFPTYPE_METER_FEATURES_STATS_REPLY) { > + struct ofputil_meter_features mf; > + ofputil_decode_meter_features(oh, &mf); > + > + bool old_state = supported_ovs_features & OVS_DP_METER_SUPPORT; > + bool new_state = mf.max_meters > 0; > + > + if (old_state != new_state) { > + ret = true; > + if (new_state) { > + supported_ovs_features |= OVS_DP_METER_SUPPORT; > + } else { > + supported_ovs_features &= ~OVS_DP_METER_SUPPORT; > + } > + } > + } > + ofpbuf_delete(msg); > + } > + rconn_run_wait(swconn); > + rconn_recv_wait(swconn); > + > + return ret; > +} > + > +void > +ovs_feature_support_init(const char *br_name) I think the name is misleading because we wouldn't call an init function multiple times. The general convention in OVS/OVN for functions that need to be called regularly in these loops is X_run() e.g. ovs_feature_support_run(). I have a suggestion below. > +{ > + if (!swconn) { > + swconn = rconn_create(5, 0, DSCP_DEFAULT, 1 << OFP15_VERSION); > + } > + > + if (swconn && !rconn_is_connected(swconn)) { > + char *target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(), br_name); > + if (strcmp(target, rconn_get_target(swconn))) { > + VLOG_INFO("%s: connecting to switch", target); > + rconn_connect(swconn, target, target); > + } > + free(target); > + } > +} > + > +void > +ovs_feature_support_deinit(void) > +{ Also, maybe the deinit() function could be _destroy()? > + rconn_destroy(swconn); > + swconn = NULL; > +} > + > /* Returns 'true' if the set of tracked OVS features has been updated. */ > bool > ovs_feature_support_update(const struct smap *ovs_capabilities) Could this be changed so that it calls the init() function above as well? Then on each iteration of the loop, only one function attempts to update all these features: bool ovs_feature_support_run(const struct smap *ovs_capabilities, const char *br_name) This function already gets called on every iteration of ovn-controller. It also "recomputes" on changes of the features, which maybe you want to do also? I might be missing something that would make this suggestion to not work? > @@ -69,6 +155,10 @@ ovs_feature_support_update(const struct smap > *ovs_capabilities) > ovs_capabilities = &empty_caps; > } > > + if (ovs_feature_get_openflow_cap()) { > + updated = true; > + } > + > for (size_t i = 0; i < ARRAY_SIZE(all_ovs_features); i++) { > enum ovs_feature_value value = all_ovs_features[i].value; > const char *name = all_ovs_features[i].name; > diff --git a/tests/ovn.at b/tests/ovn.at > index 30625ec37..9e55c0d57 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -1529,9 +1529,9 @@ log(verdict=allow, severity=warning); > log(name="test1", verdict=drop, severity=info); > encodes as > controller(userdata=00.00.00.07.00.00.00.00.01.06.74.65.73.74.31) > log(verdict=drop, severity=info, meter="meter1"); > - encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06,meter_id=4) > + encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06) > log(name="test1", verdict=drop, severity=info, meter="meter1"); > - encodes as > controller(userdata=00.00.00.07.00.00.00.00.01.06.74.65.73.74.31,meter_id=4) > + encodes as > controller(userdata=00.00.00.07.00.00.00.00.01.06.74.65.73.74.31) > log(verdict=drop); > formats as log(verdict=drop, severity=info); > encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06) > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
