On Wed, Apr 26, 2023 at 3:54 PM Ales Musil <[email protected]> wrote:
> > > On Thu, Mar 30, 2023 at 10:17 AM Naveen Yerramneni < > [email protected]> wrote: > >> Add OpenFlow extn to set conntrack entries limit per zone. >> This extn will be used in future to set the zone level limit for >> drop zones used by OVN. >> >> Signed-off-by: Naveen Yerramneni <[email protected]> >> Reviewed-by: Simon Horman <[email protected]> >> --- >> Notes: >> v1 -> v2 >> - Fix memory leak and added logs >> v2 -> v3 >> - Addressed nits >> v3 -> v4 >> - Updated change description >> >> NEWS | 2 ++ >> include/openflow/nicira-ext.h | 10 ++++++++++ >> include/openvswitch/ofp-msgs.h | 4 ++++ >> lib/ofp-bundle.c | 1 + >> lib/ofp-print.c | 11 +++++++++++ >> lib/rconn.c | 1 + >> ofproto/ofproto-dpif.c | 21 +++++++++++++++++++++ >> ofproto/ofproto-provider.h | 4 ++++ >> ofproto/ofproto.c | 25 +++++++++++++++++++++++++ >> tests/ofp-print.at | 10 ++++++++++ >> tests/ovs-ofctl.at | 12 ++++++++++++ >> utilities/ovs-ofctl.8.in | 5 +++++ >> utilities/ovs-ofctl.c | 34 ++++++++++++++++++++++++++++++++++ >> 13 files changed, 140 insertions(+) >> >> diff --git a/NEWS b/NEWS >> index fe6055a27..f6ae60856 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -32,6 +32,8 @@ v3.1.0 - xx xxx xxxx >> - OpenFlow: >> * New OpenFlow extension NXT_CT_FLUSH to flush connections matching >> the specified fields. >> + * New OpenFlow extension NXT_CT_SET_ZONE_LIMIT to set conntrack >> table >> + limit at zone level. >> - ovs-ctl: >> * New option '--dump-hugepages' to include hugepages in core dumps. >> This >> can assist with postmortem analysis involving DPDK, but may also >> produce >> diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h >> index 768775898..0f93ea21c 100644 >> --- a/include/openflow/nicira-ext.h >> +++ b/include/openflow/nicira-ext.h >> @@ -1101,4 +1101,14 @@ struct nx_ct_flush { >> }; >> OFP_ASSERT(sizeof(struct nx_ct_flush) == 8); >> >> +/* NXT_CT_SET_ZONE_LIMIT. >> + * >> + * Sets connection tracking table zone limit. */ >> +struct nx_ct_zone_limit { >> + uint8_t zero[2]; /* Must be zero. */ >> + ovs_be16 zone_id; /* Connection tracking zone. */ >> + ovs_be32 limit; /* Drop limit. */ >> +}; >> +OFP_ASSERT(sizeof(struct nx_ct_zone_limit) == 8); >> + >> #endif /* openflow/nicira-ext.h */ >> diff --git a/include/openvswitch/ofp-msgs.h >> b/include/openvswitch/ofp-msgs.h >> index 708427fc0..a9518557e 100644 >> --- a/include/openvswitch/ofp-msgs.h >> +++ b/include/openvswitch/ofp-msgs.h >> @@ -518,6 +518,9 @@ enum ofpraw { >> /* NXT 1.0+ (32): struct nx_ct_flush, uint8_t[8][]. */ >> OFPRAW_NXT_CT_FLUSH, >> >> + /* NXT 1.0+ (35): struct nx_ct_zone_limit. */ >> + OFPRAW_NXT_CT_SET_ZONE_LIMIT, >> + >> /* NXST 1.0+ (3): void. */ >> OFPRAW_NXST_IPFIX_BRIDGE_REQUEST, >> >> @@ -776,6 +779,7 @@ enum ofptype { >> OFPTYPE_IPFIX_FLOW_STATS_REPLY, /* OFPRAW_NXST_IPFIX_FLOW_REPLY */ >> OFPTYPE_CT_FLUSH_ZONE, /* OFPRAW_NXT_CT_FLUSH_ZONE. */ >> OFPTYPE_CT_FLUSH, /* OFPRAW_NXT_CT_FLUSH. */ >> + OFPTYPE_CT_SET_ZONE_LIMIT, /* OFPRAW_NXT_CT_SET_ZONE_LIMIT. */ >> >> /* Flow monitor extension. */ >> OFPTYPE_FLOW_MONITOR_CANCEL, /* OFPRAW_NXT_FLOW_MONITOR_CANCEL. >> diff --git a/lib/ofp-bundle.c b/lib/ofp-bundle.c >> index 941a8370e..3ed1f30d8 100644 >> --- a/lib/ofp-bundle.c >> +++ b/lib/ofp-bundle.c >> @@ -293,6 +293,7 @@ ofputil_is_bundlable(enum ofptype type) >> case OFPTYPE_IPFIX_FLOW_STATS_REPLY: >> case OFPTYPE_CT_FLUSH_ZONE: >> case OFPTYPE_CT_FLUSH: >> + case OFPTYPE_CT_SET_ZONE_LIMIT: >> break; >> } >> >> diff --git a/lib/ofp-print.c b/lib/ofp-print.c >> index 874079b84..8a64b72c0 100644 >> --- a/lib/ofp-print.c >> +++ b/lib/ofp-print.c >> @@ -967,6 +967,15 @@ ofp_print_nxt_ct_flush(struct ds *string, const >> struct ofp_header *oh) >> return 0; >> } >> >> +static enum ofperr >> +ofp_print_nxt_ct_set_zone_limit(struct ds *string, >> + const struct nx_ct_zone_limit *nzl) >> +{ >> + ds_put_format(string, " zone_id=%"PRIu16, ntohs(nzl->zone_id)); >> + ds_put_format(string, " limit=%"PRIu32, ntohl(nzl->limit)); >> + return 0; >> +} >> + >> static enum ofperr >> ofp_to_string__(const struct ofp_header *oh, >> const struct ofputil_port_map *port_map, >> @@ -1204,6 +1213,8 @@ ofp_to_string__(const struct ofp_header *oh, >> return ofp_print_nxt_ct_flush_zone(string, ofpmsg_body(oh)); >> case OFPTYPE_CT_FLUSH: >> return ofp_print_nxt_ct_flush(string, oh); >> + case OFPTYPE_CT_SET_ZONE_LIMIT: >> + return ofp_print_nxt_ct_set_zone_limit(string, ofpmsg_body(oh)); >> } >> >> return 0; >> diff --git a/lib/rconn.c b/lib/rconn.c >> index 4afa21515..91c982d98 100644 >> --- a/lib/rconn.c >> +++ b/lib/rconn.c >> @@ -1427,6 +1427,7 @@ is_admitted_msg(const struct ofpbuf *b) >> case OFPTYPE_IPFIX_FLOW_STATS_REPLY: >> case OFPTYPE_CT_FLUSH_ZONE: >> case OFPTYPE_CT_FLUSH: >> + case OFPTYPE_CT_SET_ZONE_LIMIT: >> default: >> return true; >> } >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index f87e27a8c..b0a66ef10 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -5631,6 +5631,26 @@ ct_del_zone_timeout_policy(const char >> *datapath_type, uint16_t zone_id) >> } >> } >> >> +static void >> +ct_set_zone_limit(const struct ofproto *ofproto_, const uint16_t zone_id, >> + const uint32_t limit) >> +{ >> + struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); >> + struct ovs_list zone_limits = OVS_LIST_INITIALIZER(&zone_limits); >> + >> + ct_dpif_push_zone_limit(&zone_limits, zone_id, limit, 0); >> + int err = ct_dpif_set_limits(ofproto->backer->dpif, NULL, >> &zone_limits); >> + if (err) { >> + VLOG_ERR_RL(&rl, "failed to set zone limit id=%"PRIu16", " >> + "limit=%"PRIu32" (%s)", zone_id, limit, >> + ovs_strerror(err)); >> + } else { >> + VLOG_DBG("configured zone limit for zone=%"PRIu16", >> limit=%"PRIu32"", >> + zone_id, limit); >> + } >> + ct_dpif_free_zone_limits(&zone_limits); >> +} >> + >> static void >> get_datapath_cap(const char *datapath_type, struct smap *cap) >> { >> @@ -6920,4 +6940,5 @@ const struct ofproto_class ofproto_dpif_class = { >> ct_flush, /* ct_flush */ >> ct_set_zone_timeout_policy, >> ct_del_zone_timeout_policy, >> + ct_set_zone_limit, /* ct_set_zone_limit */ >> }; >> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h >> index a84ddc1d0..c66623637 100644 >> --- a/ofproto/ofproto-provider.h >> +++ b/ofproto/ofproto-provider.h >> @@ -1916,6 +1916,10 @@ struct ofproto_class { >> /* Deletes the timeout policy associated with 'zone' in datapath type >> * 'dp_type'. */ >> void (*ct_del_zone_timeout_policy)(const char *dp_type, uint16_t >> zone); >> + >> + /* Sets conntrack zone limit */ >> + void (*ct_set_zone_limit)(const struct ofproto *, const uint16_t >> zone, >> + const uint32_t limit); >> }; >> >> extern const struct ofproto_class ofproto_dpif_class; >> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c >> index e4a1bee76..e8e884937 100644 >> --- a/ofproto/ofproto.c >> +++ b/ofproto/ofproto.c >> @@ -966,6 +966,28 @@ handle_nxt_ct_flush(struct ofconn *ofconn, const >> struct ofp_header *oh) >> return 0; >> } >> >> +static enum ofperr >> +handle_nxt_ct_set_zone_limit(struct ofconn *ofconn, >> + const struct ofp_header *oh) >> +{ >> + struct ofproto *ofproto = ofconn_get_ofproto(ofconn); >> + const struct nx_ct_zone_limit *nzl = ofpmsg_body(oh); >> + >> + if (!is_all_zeros(nzl->zero, sizeof nzl->zero)) { >> + return OFPERR_NXBRC_MUST_BE_ZERO; >> + } >> + >> + uint16_t zone_id = ntohs(nzl->zone_id); >> + uint32_t limit = ntohl(nzl->limit); >> + if (ofproto->ofproto_class->ct_set_zone_limit) { >> + ofproto->ofproto_class->ct_set_zone_limit(ofproto, zone_id, >> limit); >> + } else { >> + return EOPNOTSUPP; >> + } >> + >> + return 0; >> +} >> + >> void >> ofproto_set_flow_restore_wait(bool flow_restore_wait_db) >> { >> @@ -8814,6 +8836,9 @@ handle_single_part_openflow(struct ofconn *ofconn, >> const struct ofp_header *oh, >> case OFPTYPE_CT_FLUSH: >> return handle_nxt_ct_flush(ofconn, oh); >> >> + case OFPTYPE_CT_SET_ZONE_LIMIT: >> + return handle_nxt_ct_set_zone_limit(ofconn, oh); >> + >> case OFPTYPE_HELLO: >> case OFPTYPE_ERROR: >> case OFPTYPE_FEATURES_REPLY: >> diff --git a/tests/ofp-print.at b/tests/ofp-print.at >> index 14aa55416..5c45e1ec6 100644 >> --- a/tests/ofp-print.at >> +++ b/tests/ofp-print.at >> @@ -4181,3 +4181,13 @@ AT_CHECK([ovs-ofctl ofp-print "\ >> 00 00 00 14 00 00 00 00 00 00 00 00 00 00 ff ff 0a 0a 00 02 00 00 00 00 \ >> " | grep -q OFPBPC_BAD_VALUE], [0]) >> AT_CLEANUP >> + >> +AT_SETUP([NXT_CT_SET_ZONE_LIMIT]) >> +AT_KEYWORDS([ofp-print]) >> +AT_CHECK([ovs-ofctl ofp-print "\ >> +01 04 00 18 00 00 00 03 00 00 23 20 00 00 00 23 \ >> +00 00 00 12 00 01 86 a0 \ >> +"], [0], [dnl >> +NXT_CT_SET_ZONE_LIMIT (xid=0x3): zone_id=18 limit=100000 >> +]) >> +AT_CLEANUP >> diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at >> index 8531b2e2e..8a17d3609 100644 >> --- a/tests/ovs-ofctl.at >> +++ b/tests/ovs-ofctl.at >> @@ -3309,3 +3309,15 @@ AT_CHECK([grep -q "ct_dpif|DBG|.*ct_flush: <all>" >> ovs-vswitchd.log]) >> >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> + >> + >> +AT_SETUP([ovs-ofctl ct-set-zone-limit]) >> +OVS_VSWITCHD_START >> + >> +AT_CHECK([ovs-appctl vlog/set ct_dpif:dbg]) >> +AT_CHECK([ovs-ofctl ct-set-zone-limit br0 1 200000]) >> + >> +OVS_WAIT_UNTIL([grep -q "ofproto_dpif|DBG|.*zone=1.*limit=200000" >> ovs-vswitchd.log]) >> +AT_CHECK([grep -q "ofproto_dpif|DBG|.*zone=1.*limit=200000" >> ovs-vswitchd.log]) >> +OVS_VSWITCHD_STOP >> +AT_CLEANUP >> diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in >> index 0a611b2ee..8a6d5a3db 100644 >> --- a/utilities/ovs-ofctl.8.in >> +++ b/utilities/ovs-ofctl.8.in >> @@ -326,6 +326,11 @@ An example of an IPv6 TCP >> \fIct-[orig|reply]-tuple\fR: >> .IP >> This command uses an Open vSwitch extension that is only in Open vSwitch >> 3.1 >> and later. >> +.IP "\fBct\-set\-zone\-limit \fIswitch zone limit\fR >> +Set the connection tracking entries limit in \fIzone\fR on \fIswitch\fR. >> +.IP >> +This command uses an Open vSwitch extension that is only in Open >> +vSwitch 3.1 and later. >> . >> .SS "OpenFlow Switch Flow Table Commands" >> . >> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c >> index eabec18a3..1464827bb 100644 >> --- a/utilities/ovs-ofctl.c >> +++ b/utilities/ovs-ofctl.c >> @@ -489,6 +489,8 @@ usage(void) >> " ct-flush SWITCH [ZONE] [CT_ORIG_TUPLE [CT_REPLY_TUPLE]]\n" >> " flush conntrack entries >> specified\n" >> " by CT_ORIG/REPLY_TUPLE and >> ZONE\n" >> + " ct-set-zone-limit SWITCH ZONE LIMIT set conntrack >> entries\n" >> + " limit for the ZONE\n" >> "\nFor OpenFlow switches and controllers:\n" >> " probe TARGET probe whether TARGET is up\n" >> " ping TARGET [N] latency of N-byte echos\n" >> @@ -3098,6 +3100,35 @@ ofctl_ct_flush(struct ovs_cmdl_context *ctx) >> vconn_close(vconn); >> } >> >> +static void >> +ofctl_ct_set_zone_limit(struct ovs_cmdl_context *ctx) >> +{ >> + uint16_t zone_id; >> + uint32_t limit; >> + >> + char *error = str_to_u16(ctx->argv[2], "zone_id", &zone_id); >> + if (error) { >> + ovs_fatal(0, "%s", error); >> + } >> + error = str_to_u32(ctx->argv[3], &limit); >> + if (error) { >> + ovs_fatal(0, "%s", error); >> + } >> + >> + struct vconn *vconn; >> + open_vconn(ctx->argv[1], &vconn); >> + enum ofp_version version = vconn_get_version(vconn); >> + >> + struct ofpbuf *msg = ofpraw_alloc(OFPRAW_NXT_CT_SET_ZONE_LIMIT, >> version, >> + 0); >> + struct nx_ct_zone_limit *nzl = ofpbuf_put_zeros(msg, sizeof *nzl); >> + nzl->zone_id = htons(zone_id); >> + nzl->limit = htonl(limit); >> + >> + transact_noreply(vconn, msg); >> + vconn_close(vconn); >> +} >> + >> static void >> ofctl_dump_ipfix_flow(struct ovs_cmdl_context *ctx) >> { >> @@ -5114,6 +5145,9 @@ static const struct ovs_cmdl_command all_commands[] >> = { >> { "ct-flush", "switch [zone=N] [ct-orig-tuple [ct-reply-tuple]]", >> 1, 4, ofctl_ct_flush, OVS_RO }, >> >> + { "ct-set-zone-limit", "switch zone limit", >> + 3, 3, ofctl_ct_set_zone_limit, OVS_RO }, >> + >> { "ofp-parse", "file", >> 1, 1, ofctl_ofp_parse, OVS_RW }, >> { "ofp-parse-pcap", "pcap", >> -- >> 2.22.3 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Hi, > > overall the change looks good to me, I think it would be helpful > to have a helper function to construct the extension message > e.g. ofp_ct_zone_limit_encode, especially if there is a plan to > use it in OVN. > And I forgot to mention, if it should be used in OVN we need some indication for OVN that this is supported e.g. some feature flag. > > Thanks, > Ales > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > [email protected] IM: amusil > <https://red.ht/sig> > -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
