[ovs-dev,v4] OpenFlow: Add extn to set conntrack entries limit per zone.

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 <naveen.yerramneni@nutanix.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
---
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(+)

On Thu, Mar 30, 2023 at 08:17:18AM +0000, Naveen Yerramneni 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 <naveen.yerramneni@nutanix.com>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> ---
> Notes:
>   v1 -> v2
>   - Fix memory leak and added logs
>   v2 -> v3
>   - Addressed nits
>   v3 -> v4
>   - Updated change description

Thanks for the update.
This looks good to me.

Hi Team,

Could someone please look into this and merge if everything looks good ?

Thanks,
Naveen

> On 30-Mar-2023, at 1:47 PM, Naveen Yerramneni <naveen.yerramneni@nutanix.com> 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 <naveen.yerramneni@nutanix.com>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> ---
> 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
>

On Thu, Mar 30, 2023 at 10:17 AM Naveen Yerramneni <
naveen.yerramneni@nutanix.com> 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 <naveen.yerramneni@nutanix.com>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> ---
> 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
> dev@openvswitch.org
> 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.

Thanks,
Ales

On Wed, Apr 26, 2023 at 3:54 PM Ales Musil <amusil@redhat.com> wrote:

>
>
> On Thu, Mar 30, 2023 at 10:17 AM Naveen Yerramneni <
> naveen.yerramneni@nutanix.com> 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 <naveen.yerramneni@nutanix.com>
>> Reviewed-by: Simon Horman <simon.horman@corigine.com>
>> ---
>> 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
>> dev@openvswitch.org
>> 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>
>
> amusil@redhat.com    IM: amusil
> <https://red.ht/sig>
>

On 3/30/23 10:17, Naveen Yerramneni 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 <naveen.yerramneni@nutanix.com>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> ---
> 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.

Hi, Naveen.  Sorry for the late reply, but I don't think this functionality
should be implemented within OpenFlow interface.

OpenFlow is not great for configurations that should preserve the state on
re-start, for example.  CT_FLUSH is reasonable to implement via OpenFlow,
because it is a one-shot stateless command.  But limits are stateful.  We
should be able to request a current value and save and restore the
configuration on re-start with ovs-save script.  This is not ideal.

What we should do instead is to naturally extend the existing database
configuration that already supports per-zone configuration of conntrack
timeouts.  That will be more organic within OVS and will solve the problem
with configuration persistence.

See the CT_Zone and CT_Timeout_Policy database columns.

Best regards, Ilya Maximets.

Hi IIya,

Thanks for the review.
One question, once vswitchd DB schema is updated to store zone level limits. OVN controller should directly update the configuration in the database , right ?


Thanks,
Naveen


On 04-May-2023, at 3:33 PM, Ilya Maximets <i.maximets@ovn.org<mailto:i.maximets@ovn.org>> wrote:

On 3/30/23 10:17, Naveen Yerramneni 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 <naveen.yerramneni@nutanix.com<mailto:naveen.yerramneni@nutanix.com>>
Reviewed-by: Simon Horman <simon.horman@corigine.com<mailto:simon.horman@corigine.com>>
---
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<http://ofp-print.at>             | 10 ++++++++++
tests/ovs-ofctl.at<http://ovs-ofctl.at>             | 12 ++++++++++++
utilities/ovs-ofctl.8.in<http://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.

Hi, Naveen.  Sorry for the late reply, but I don't think this functionality
should be implemented within OpenFlow interface.

OpenFlow is not great for configurations that should preserve the state on
re-start, for example.  CT_FLUSH is reasonable to implement via OpenFlow,
because it is a one-shot stateless command.  But limits are stateful.  We
should be able to request a current value and save and restore the
configuration on re-start with ovs-save script.  This is not ideal.

What we should do instead is to naturally extend the existing database
configuration that already supports per-zone configuration of conntrack
timeouts.  That will be more organic within OVS and will solve the problem
with configuration persistence.

See the CT_Zone and CT_Timeout_Policy database columns.

Best regards, Ilya Maximets.

On 5/5/23 14:06, Naveen Yerramneni wrote:
> Hi IIya,
> 
> Thanks for the review.
> 
> One question, once vswitchd DB schema is updated to store zone level limits. OVN controller should directly update the configuration in the database , right ?

Right.  ovn-controller already has a database connection,
so that should not be a problem.

Best regards, Ilya Maximets.

> 
> Thanks,
> Naveen
> 
>     On 04-May-2023, at 3:33 PM, Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> wrote:
> 
>     On 3/30/23 10:17, Naveen Yerramneni 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 <naveen.yerramneni@nutanix.com <mailto:naveen.yerramneni@nutanix.com>>
>         Reviewed-by: Simon Horman <simon.horman@corigine.com <mailto:simon.horman@corigine.com>>
>         ---
>         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 <http://ofp-print.at>             | 10 ++++++++++
>         tests/ovs-ofctl.at <http://ovs-ofctl.at>             | 12 ++++++++++++
>         utilities/ovs-ofctl.8.in <http://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.
> 
> 
>     Hi, Naveen.  Sorry for the late reply, but I don't think this functionality
>     should be implemented within OpenFlow interface.
> 
>     OpenFlow is not great for configurations that should preserve the state on
>     re-start, for example.  CT_FLUSH is reasonable to implement via OpenFlow,
>     because it is a one-shot stateless command.  But limits are stateful.  We
>     should be able to request a current value and save and restore the
>     configuration on re-start with ovs-save script.  This is not ideal.
> 
>     What we should do instead is to naturally extend the existing database
>     configuration that already supports per-zone configuration of conntrack
>     timeouts.  That will be more organic within OVS and will solve the problem
>     with configuration persistence.
> 
>     See the CT_Zone and CT_Timeout_Policy database columns.
> 
>     Best regards, Ilya Maximets.
> 
>  
>

On Fri, May 5, 2023 at 3:20 PM Ilya Maximets <i.maximets@ovn.org> wrote:

> On 5/5/23 14:06, Naveen Yerramneni wrote:
> > Hi IIya,
> >
> > Thanks for the review.
> >
> > One question, once vswitchd DB schema is updated to store zone level
> limits. OVN controller should directly update the configuration in the
> database , right ?
>
> Right.  ovn-controller already has a database connection,
> so that should not be a problem.
>
> Best regards, Ilya Maximets.
>
> >
> > Thanks,
> > Naveen
> >
> >     On 04-May-2023, at 3:33 PM, Ilya Maximets <i.maximets@ovn.org
> <mailto:i.maximets@ovn.org>> wrote:
> >
> >     On 3/30/23 10:17, Naveen Yerramneni 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 <naveen.yerramneni@nutanix.com
> <mailto:naveen.yerramneni@nutanix.com>>
> >         Reviewed-by: Simon Horman <simon.horman@corigine.com <mailto:
> simon.horman@corigine.com>>
> >         ---
> >         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 <http://ofp-print.at>             | 10
> ++++++++++
> >         tests/ovs-ofctl.at <http://ovs-ofctl.at>             | 12
> ++++++++++++
> >         utilities/ovs-ofctl.8.in <http://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.
> >
> >
> >     Hi, Naveen.  Sorry for the late reply, but I don't think this
> functionality
> >     should be implemented within OpenFlow interface.
> >
> >     OpenFlow is not great for configurations that should preserve the
> state on
> >     re-start, for example.  CT_FLUSH is reasonable to implement via
> OpenFlow,
> >     because it is a one-shot stateless command.  But limits are
> stateful.  We
> >     should be able to request a current value and save and restore the
> >     configuration on re-start with ovs-save script.  This is not ideal.
> >
> >     What we should do instead is to naturally extend the existing
> database
> >     configuration that already supports per-zone configuration of
> conntrack
> >     timeouts.  That will be more organic within OVS and will solve the
> problem
> >     with configuration persistence.
> >
> >     See the CT_Zone and CT_Timeout_Policy database columns.
> >
> >     Best regards, Ilya Maximets.
> >
> >
> >
>
>
Hi Naveen,

it has been a while since the last update. Do you still plan to work on
this?
We have a request for this feature in OVN, please let me know if not and
I'll
continue the work myself.

Thank you.
Best regards,
Ales