On 10/18/23 09:56, Ales Musil wrote:
> Internally handle default CT zone limit as other limits that
> can be passed via the list with special value -1. Currently,
> the -1 is treated by both datapaths as default, add static
> asserts to make sure that this remains the case in the future.
> This allows us to easily delete the default zone limit.
>
> Signed-off-by: Ales Musil <amu...@redhat.com>
> ---
> v5: Rebase on top of current master.
> Address comments from Ilya:
> - Fix some typos.
> - Use OVS_ZONE_LIMIT_DEFAULT_ZONE instead of special constant.
> - Do not relay on DEFAULT_ZONE being -1 for the limit list.
> - Fix wrong netlink message.
> ---
> lib/conntrack.c | 2 +-
> lib/conntrack.h | 5 +++--
> lib/ct-dpif.c | 28 +++++++++++++++-------------
> lib/ct-dpif.h | 14 ++++++--------
> lib/dpctl.c | 15 ++++++++-------
> lib/dpif-netdev.c | 21 ++++++---------------
> lib/dpif-netlink.c | 26 +++++---------------------
> lib/dpif-provider.h | 16 +++++++---------
> 8 files changed, 51 insertions(+), 76 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 47a443fba..31f00a127 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -398,7 +398,7 @@ zone_limit_clean(struct conntrack *ct, struct zone_limit
> *zl)
> }
>
> int
> -zone_limit_delete(struct conntrack *ct, uint16_t zone)
> +zone_limit_delete(struct conntrack *ct, int32_t zone)
> {
> ovs_mutex_lock(&ct->ct_lock);
> struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
> diff --git a/lib/conntrack.h b/lib/conntrack.h
> index 57d5159b6..a8df4f78b 100644
> --- a/lib/conntrack.h
> +++ b/lib/conntrack.h
> @@ -122,7 +122,8 @@ struct timeout_policy {
>
> enum {
> INVALID_ZONE = -2,
> - DEFAULT_ZONE = -1, /* Default zone for zone limit management. */
> + DEFAULT_ZONE = OVS_ZONE_LIMIT_DEFAULT_ZONE, /* Default zone for zone
> + * limit management. */
> MIN_ZONE = 0,
> MAX_ZONE = 0xFFFF,
> };
We may want to add an assertion here that DEFAULT_ZONE doesn't
overlap with other values.
> @@ -154,6 +155,6 @@ struct ipf *conntrack_ipf_ctx(struct conntrack *ct);
> struct conntrack_zone_limit zone_limit_get(struct conntrack *ct,
> int32_t zone);
> int zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit);
> -int zone_limit_delete(struct conntrack *ct, uint16_t zone);
> +int zone_limit_delete(struct conntrack *ct, int32_t zone);
>
> #endif /* conntrack.h */
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index f59c6e560..2ee045164 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -398,23 +398,19 @@ ct_dpif_get_tcp_seq_chk(struct dpif *dpif, bool
> *enabled)
> }
>
> int
> -ct_dpif_set_limits(struct dpif *dpif, const uint32_t *default_limit,
> - const struct ovs_list *zone_limits)
> +ct_dpif_set_limits(struct dpif *dpif, const struct ovs_list *zone_limits)
> {
> return (dpif->dpif_class->ct_set_limits
> - ? dpif->dpif_class->ct_set_limits(dpif, default_limit,
> - zone_limits)
> + ? dpif->dpif_class->ct_set_limits(dpif, zone_limits)
> : EOPNOTSUPP);
> }
>
> int
> -ct_dpif_get_limits(struct dpif *dpif, uint32_t *default_limit,
> - const struct ovs_list *zone_limits_in,
> +ct_dpif_get_limits(struct dpif *dpif, const struct ovs_list *zone_limits_in,
> struct ovs_list *zone_limits_out)
> {
> return (dpif->dpif_class->ct_get_limits
> - ? dpif->dpif_class->ct_get_limits(dpif, default_limit,
> - zone_limits_in,
> + ? dpif->dpif_class->ct_get_limits(dpif, zone_limits_in,
> zone_limits_out)
> : EOPNOTSUPP);
> }
> @@ -854,7 +850,7 @@ ct_dpif_format_tcp_stat(struct ds * ds, int tcp_state,
> int conn_per_state)
>
>
> void
> -ct_dpif_push_zone_limit(struct ovs_list *zone_limits, uint16_t zone,
> +ct_dpif_push_zone_limit(struct ovs_list *zone_limits, int32_t zone,
> uint32_t limit, uint32_t count)
> {
> struct ct_dpif_zone_limit *zone_limit = xmalloc(sizeof *zone_limit);
> @@ -928,15 +924,21 @@ error:
> }
>
> void
> -ct_dpif_format_zone_limits(uint32_t default_limit,
> - const struct ovs_list *zone_limits, struct ds *ds)
> +ct_dpif_format_zone_limits(const struct ovs_list *zone_limits, struct ds *ds)
> {
> struct ct_dpif_zone_limit *zone_limit;
>
> - ds_put_format(ds, "default limit=%"PRIu32, default_limit);
> + LIST_FOR_EACH (zone_limit, node, zone_limits) {
> + if (zone_limit->zone == OVS_ZONE_LIMIT_DEFAULT_ZONE) {
> + ds_put_format(ds, "default limit=%"PRIu32, zone_limit->limit);
> + }
> + }
>
> LIST_FOR_EACH (zone_limit, node, zone_limits) {
> - ds_put_format(ds, "\nzone=%"PRIu16, zone_limit->zone);
> + if (zone_limit->zone == OVS_ZONE_LIMIT_DEFAULT_ZONE) {
> + continue;
> + }
> + ds_put_format(ds, "\nzone=%"PRIu16, (uint16_t) zone_limit->zone);
> ds_put_format(ds, ",limit=%"PRIu32, zone_limit->limit);
> ds_put_format(ds, ",count=%"PRIu32, zone_limit->count);
> }
> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index 0b728b529..c8a7c155e 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -237,7 +237,7 @@ struct ct_dpif_dump_state {
> };
>
> struct ct_dpif_zone_limit {
> - uint16_t zone;
> + int32_t zone;
> uint32_t limit; /* Limit on number of entries. */
> uint32_t count; /* Current number of entries. */
> struct ovs_list node;
> @@ -307,10 +307,9 @@ int ct_dpif_get_maxconns(struct dpif *dpif, uint32_t
> *maxconns);
> int ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns);
> int ct_dpif_set_tcp_seq_chk(struct dpif *dpif, bool enabled);
> int ct_dpif_get_tcp_seq_chk(struct dpif *dpif, bool *enabled);
> -int ct_dpif_set_limits(struct dpif *dpif, const uint32_t *default_limit,
> - const struct ovs_list *);
> -int ct_dpif_get_limits(struct dpif *dpif, uint32_t *default_limit,
> - const struct ovs_list *, struct ovs_list *);
> +int ct_dpif_set_limits(struct dpif *dpif, const struct ovs_list *);
> +int ct_dpif_get_limits(struct dpif *dpif, const struct ovs_list *,
> + struct ovs_list *);
> int ct_dpif_del_limits(struct dpif *dpif, const struct ovs_list *);
> int ct_dpif_sweep(struct dpif *, uint32_t *ms);
> int ct_dpif_ipf_set_enabled(struct dpif *, bool v6, bool enable);
> @@ -329,13 +328,12 @@ void ct_dpif_format_ipproto(struct ds *ds, uint16_t
> ipproto);
> void ct_dpif_format_tuple(struct ds *, const struct ct_dpif_tuple *);
> uint8_t ct_dpif_coalesce_tcp_state(uint8_t state);
> void ct_dpif_format_tcp_stat(struct ds *, int, int);
> -void ct_dpif_push_zone_limit(struct ovs_list *, uint16_t zone, uint32_t
> limit,
> +void ct_dpif_push_zone_limit(struct ovs_list *, int32_t zone, uint32_t limit,
> uint32_t count);
> void ct_dpif_free_zone_limits(struct ovs_list *);
> bool ct_dpif_parse_zone_limit_tuple(const char *s, uint16_t *pzone,
> uint32_t *plimit, struct ds *);
> -void ct_dpif_format_zone_limits(uint32_t default_limit,
> - const struct ovs_list *, struct ds *);
> +void ct_dpif_format_zone_limits(const struct ovs_list *, struct ds *);
> bool ct_dpif_set_timeout_policy_attr_by_name(struct ct_dpif_timeout_policy
> *tp,
> const char *key, uint32_t
> value);
> bool ct_dpif_timeout_policy_support_ipproto(uint8_t ipproto);
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index cd12625a1..76f21a530 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -2202,7 +2202,7 @@ dpctl_ct_set_limits(int argc, const char *argv[],
> struct dpif *dpif;
> struct ds ds = DS_EMPTY_INITIALIZER;
> int i = dp_arg_exists(argc, argv) ? 2 : 1;
> - uint32_t default_limit, *p_default_limit = NULL;
> + uint32_t default_limit;
> struct ovs_list zone_limits = OVS_LIST_INITIALIZER(&zone_limits);
>
> int error = opt_dpif_open(argc, argv, dpctl_p, INT_MAX, &dpif);
> @@ -2213,7 +2213,8 @@ dpctl_ct_set_limits(int argc, const char *argv[],
> /* Parse default limit */
> if (!strncmp(argv[i], "default=", 8)) {
> if (ovs_scan(argv[i], "default=%"SCNu32, &default_limit)) {
> - p_default_limit = &default_limit;
> + ct_dpif_push_zone_limit(&zone_limits,
> OVS_ZONE_LIMIT_DEFAULT_ZONE,
> + default_limit, 0);
> i++;
> } else {
> ds_put_cstr(&ds, "invalid default limit");
> @@ -2233,7 +2234,7 @@ dpctl_ct_set_limits(int argc, const char *argv[],
> ct_dpif_push_zone_limit(&zone_limits, zone, limit, 0);
> }
>
> - error = ct_dpif_set_limits(dpif, p_default_limit, &zone_limits);
> + error = ct_dpif_set_limits(dpif, &zone_limits);
> if (!error) {
> ct_dpif_free_zone_limits(&zone_limits);
> dpif_close(dpif);
> @@ -2322,7 +2323,6 @@ dpctl_ct_get_limits(int argc, const char *argv[],
> {
> struct dpif *dpif;
> struct ds ds = DS_EMPTY_INITIALIZER;
> - uint32_t default_limit;
> int i = dp_arg_exists(argc, argv) ? 2 : 1;
> struct ovs_list list_query = OVS_LIST_INITIALIZER(&list_query);
> struct ovs_list list_reply = OVS_LIST_INITIALIZER(&list_reply);
> @@ -2333,16 +2333,17 @@ dpctl_ct_get_limits(int argc, const char *argv[],
> }
>
> if (argc > i) {
> + ct_dpif_push_zone_limit(&list_query, OVS_ZONE_LIMIT_DEFAULT_ZONE,
> + 0, 0);
> error = parse_ct_limit_zones(argv[i], &list_query, &ds);
> if (error) {
> goto error;
> }
> }
>
> - error = ct_dpif_get_limits(dpif, &default_limit, &list_query,
> - &list_reply);
> + error = ct_dpif_get_limits(dpif, &list_query, &list_reply);
> if (!error) {
> - ct_dpif_format_zone_limits(default_limit, &list_reply, &ds);
> + ct_dpif_format_zone_limits(&list_reply, &ds);
> dpctl_print(dpctl_p, "%s\n", ds_cstr(&ds));
> goto out;
> } else {
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 157694bcf..fc971849d 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -9446,17 +9446,10 @@ dpif_netdev_ct_get_sweep_interval(struct dpif *dpif,
> uint32_t *ms)
>
> static int
> dpif_netdev_ct_set_limits(struct dpif *dpif,
> - const uint32_t *default_limits,
> const struct ovs_list *zone_limits)
> {
> int err = 0;
> struct dp_netdev *dp = get_dp_netdev(dpif);
> - if (default_limits) {
> - err = zone_limit_update(dp->conntrack, DEFAULT_ZONE,
> *default_limits);
> - if (err != 0) {
> - return err;
> - }
> - }
>
> struct ct_dpif_zone_limit *zone_limit;
> LIST_FOR_EACH (zone_limit, node, zone_limits) {
> @@ -9471,20 +9464,12 @@ dpif_netdev_ct_set_limits(struct dpif *dpif,
>
> static int
> dpif_netdev_ct_get_limits(struct dpif *dpif,
> - uint32_t *default_limit,
> const struct ovs_list *zone_limits_request,
> struct ovs_list *zone_limits_reply)
> {
> struct dp_netdev *dp = get_dp_netdev(dpif);
> struct conntrack_zone_limit czl;
>
> - czl = zone_limit_get(dp->conntrack, DEFAULT_ZONE);
> - if (czl.zone == DEFAULT_ZONE) {
> - *default_limit = czl.limit;
> - } else {
> - return EINVAL;
> - }
> -
> if (!ovs_list_is_empty(zone_limits_request)) {
> struct ct_dpif_zone_limit *zone_limit;
> LIST_FOR_EACH (zone_limit, node, zone_limits_request) {
> @@ -9498,6 +9483,12 @@ dpif_netdev_ct_get_limits(struct dpif *dpif,
> }
> }
> } else {
> + czl = zone_limit_get(dp->conntrack, DEFAULT_ZONE);
> + if (czl.zone == DEFAULT_ZONE) {
> + ct_dpif_push_zone_limit(zone_limits_reply, DEFAULT_ZONE,
> czl.limit,
> + atomic_count_get(&czl.count));
> + }
> +
> for (int z = MIN_ZONE; z <= MAX_ZONE; z++) {
> czl = zone_limit_get(dp->conntrack, z);
> if (czl.zone == z) {
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 9194971d3..8ff42ff21 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -3360,7 +3360,6 @@ dpif_netlink_ct_flush(struct dpif *dpif OVS_UNUSED,
> const uint16_t *zone,
>
> static int
> dpif_netlink_ct_set_limits(struct dpif *dpif OVS_UNUSED,
> - const uint32_t *default_limits,
> const struct ovs_list *zone_limits)
> {
> if (ovs_ct_limit_family < 0) {
> @@ -3378,13 +3377,6 @@ dpif_netlink_ct_set_limits(struct dpif *dpif
> OVS_UNUSED,
>
> size_t opt_offset;
> opt_offset = nl_msg_start_nested(request, OVS_CT_LIMIT_ATTR_ZONE_LIMIT);
> - if (default_limits) {
> - struct ovs_zone_limit req_zone_limit = {
> - .zone_id = OVS_ZONE_LIMIT_DEFAULT_ZONE,
> - .limit = *default_limits,
> - };
> - nl_msg_put(request, &req_zone_limit, sizeof req_zone_limit);
> - }
>
> if (!ovs_list_is_empty(zone_limits)) {
> struct ct_dpif_zone_limit *zone_limit;
> @@ -3406,7 +3398,6 @@ dpif_netlink_ct_set_limits(struct dpif *dpif OVS_UNUSED,
>
> static int
> dpif_netlink_zone_limits_from_ofpbuf(const struct ofpbuf *buf,
> - uint32_t *default_limit,
> struct ovs_list *zone_limits)
> {
> static const struct nl_policy ovs_ct_limit_policy[] = {
> @@ -3439,9 +3430,7 @@ dpif_netlink_zone_limits_from_ofpbuf(const struct
> ofpbuf *buf,
> nl_attr_get(attr[OVS_CT_LIMIT_ATTR_ZONE_LIMIT]);
>
> while (rem >= sizeof *zone_limit) {
> - if (zone_limit->zone_id == OVS_ZONE_LIMIT_DEFAULT_ZONE) {
> - *default_limit = zone_limit->limit;
> - } else if (zone_limit->zone_id < OVS_ZONE_LIMIT_DEFAULT_ZONE ||
> + if (zone_limit->zone_id < OVS_ZONE_LIMIT_DEFAULT_ZONE ||
> zone_limit->zone_id > UINT16_MAX) {
Looks a little strange to have an empty primary 'if' block.
It made some sense as an 'else if', but should be adjusted now.
> } else {
> ct_dpif_push_zone_limit(zone_limits, zone_limit->zone_id,
> @@ -3456,7 +3445,6 @@ dpif_netlink_zone_limits_from_ofpbuf(const struct
> ofpbuf *buf,
>
> static int
> dpif_netlink_ct_get_limits(struct dpif *dpif OVS_UNUSED,
> - uint32_t *default_limit,
> const struct ovs_list *zone_limits_request,
> struct ovs_list *zone_limits_reply)
> {
> @@ -3477,14 +3465,11 @@ dpif_netlink_ct_get_limits(struct dpif *dpif
> OVS_UNUSED,
> size_t opt_offset = nl_msg_start_nested(request,
>
> OVS_CT_LIMIT_ATTR_ZONE_LIMIT);
>
> - struct ovs_zone_limit req_zone_limit = {
> - .zone_id = OVS_ZONE_LIMIT_DEFAULT_ZONE,
> - };
> - nl_msg_put(request, &req_zone_limit, sizeof req_zone_limit);
> -
> struct ct_dpif_zone_limit *zone_limit;
> LIST_FOR_EACH (zone_limit, node, zone_limits_request) {
> - req_zone_limit.zone_id = zone_limit->zone;
> + struct ovs_zone_limit req_zone_limit = {
> + .zone_id = zone_limit->zone,
> + };
> nl_msg_put(request, &req_zone_limit, sizeof req_zone_limit);
> }
>
> @@ -3497,8 +3482,7 @@ dpif_netlink_ct_get_limits(struct dpif *dpif OVS_UNUSED,
> goto out;
> }
>
> - err = dpif_netlink_zone_limits_from_ofpbuf(reply, default_limit,
> - zone_limits_reply);
> + err = dpif_netlink_zone_limits_from_ofpbuf(reply, zone_limits_reply);
>
> out:
> ofpbuf_delete(request);
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 1b822cb07..a3ffa27db 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -520,19 +520,17 @@ struct dpif_class {
>
> /* Sets the max connections allowed per zone according to 'zone_limits',
> * a list of 'struct ct_dpif_zone_limit' entries (the 'count' member
> - * is not used when setting limits). If 'default_limit' is not NULL,
> - * modifies the default limit to '*default_limit'. */
> - int (*ct_set_limits)(struct dpif *, const uint32_t *default_limit,
> - const struct ovs_list *zone_limits);
> + * is not used when setting limits). */
> + int (*ct_set_limits)(struct dpif *, const struct ovs_list *zone_limits);
>
> - /* Looks up the default per zone limit and stores that in
> - * 'default_limit'. Look up the per zone limits for all zones in
> + /* Look up the per zone limits for all zones in
* Looks up
Also, the line can fit more text.
> * the 'zone_limits_in' list of 'struct ct_dpif_zone_limit' entries
> * (the 'limit' and 'count' members are not used), and stores the
> * reply that includes the zone, the per zone limit, and the number
> - * of connections in the zone into 'zone_limits_out' list. */
> - int (*ct_get_limits)(struct dpif *, uint32_t *default_limit,
> - const struct ovs_list *zone_limits_in,
> + * of connections in the zone into 'zone_limits_out' list. If the
2 spaces between sentences.
> + * 'zone_limits_in' list is empty the report will contain all previously
> + * set zone limits and the default limit.*/
A space after a period.
> + int (*ct_get_limits)(struct dpif *, const struct ovs_list
> *zone_limits_in,
> struct ovs_list *zone_limits_out);
>
> /* Deletes per zone limit of all zones specified in 'zone_limits', a
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
