> On 16/09/2021 04:42, Mark Michelson wrote: > > Hi Lorenzo, I have some comments down below. > > > > Thanks for your changes so far Lorenzo.
Hi Mark, thx for the review > > > On 9/9/21 7:27 PM, Lorenzo Bianconi wrote: > >> Dump datapath meter capabilities before configuring meters in > >> ovn-controller > >> > >> Signed-off-by: Lorenzo Bianconi <[email protected]> > >> --- > >> 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 | 7 +++ > >> lib/actions.c | 3 ++ > >> lib/automake.mk | 1 + > >> lib/features.c | 89 +++++++++++++++++++++++++++++++++++++ > >> tests/ovn.at | 4 +- > >> 6 files changed, 106 insertions(+), 2 deletions(-) > >> > >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > >> index 0031a1035..8c6d4393b 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); > >> + } > >> > >> /* 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..4a6d45d88 100644 > >> --- a/include/ovn/features.h > >> +++ b/include/ovn/features.h > >> @@ -23,17 +23,24 @@ > >> /* ovn-controller supported feature names. */ > >> #define OVN_FEATURE_PORT_UP_NOTIF "port-up-notif" > >> > >> +struct ovsrec_bridge; > >> +struct rconn; > >> + > > > > The forward declaration of rconn is unnecessary here. > > > >> /* OVS datapath supported features. Based on availability OVN might > >> generate > >> * different types of openflows. > >> */ > >> 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 struct ovsrec_bridge *br_int); > >> +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..bd7ba79ef 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,80 @@ ovs_feature_is_supported(enum ovs_feature_value feature) > >> return supported_ovs_features & feature; > >> } > >> > >> +static bool > >> +ovn_controller_get_ofp_capa(void) > This is definitely a nit, but I think I prefer something like > > static bool > ovs_feature_get_openflow_cap() This will trigger the following warning: b/features.c: In function ‘ovn_controller_get_ofp_capa’: lib/features.c:73:1: warning: old-style function definition [-Wold-style-definition] > > >> +{ > >> + 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 < 10; i++) { > >> + msg = rconn_recv(swconn); > >> + if (!msg) { > >> + break; > >> + } > > > > This rconn usage seems like it could be unreliable. If ovs-vswitchd is > > super busy, it's possible that ovs-vswitchd will not be able to respond > > to the request before we call rconn_recv(). > > Maybe vconn would be more appropriate here? rconn does a lot of things > (queuing, reliability, etc) that I don't think are needed here. It is > also tricky to manage because you need to call rconn_run(), etc. As all > we need to do is quickly check the capabilities, I think this use case > is similar to calling `ovs-ofctrl show` (which also report the > capabilities) - this uses a vconn connection - > https://github.com/openvswitch/ovs/blob/849a40ccfb9c7c6bba635b517caac4f12ab63eee/utilities/ovs-ofctl.c#L833. I am fine with it, I have just reused the same approach used in ofctrl or pinctrl. > > > This would also mean you could remove the init() and deinit() function, > simplifying the code. If the connection fails for some reason, we can > try again on the next iteration. > > > > > I think a more reliable way of dealing with this would be to make use of > > the poll loop to ensure that we only try to read from the rconn when it > > has data ready. > > > > Also, why are there 10 iterations? I would have expected for this to > > loop continually until rconn_recv() no longer returns any data. > > > >> + > >> + 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); > >> + } > >> + > >> + return ret; > >> +} > >> + > >> +void > >> +ovs_feature_support_init(const struct ovsrec_bridge *br_int) > > I think you should remove the dependency on 'struct ovsrec_bridge' by > passing the bridge name to this function (i.e. a 'char *') ack, I will fix it Regards, Lorenzo > > >> +{ > >> + 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_int->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) > >> +{ > >> + 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) > >> @@ -69,6 +154,10 @@ ovs_feature_support_update(const struct smap > >> *ovs_capabilities) > >> ovs_capabilities = &empty_caps; > >> } > >> > >> + if (ovn_controller_get_ofp_capa()) { > > > > Can the openflow capabilities change while ovs-vswitchd is running? If > > they cannot, then should we only request OFP capabilities after a reconnect? > > > >> + 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 5104a6895..fdee26ac9 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 > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
