[ovs-dev] [PATCH v3 ovn] controller: add datapath meter capability check
Lorenzo Bianconi
lorenzo.bianconi at redhat.com
Thu Sep 16 13:46:19 UTC 2021
> 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 <lorenzo.bianconi at redhat.com>
> >> ---
> >> 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
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
More information about the dev
mailing list