Hi, Ales. Thanks for the new version!
See some comments inline.
Best regards, Ilya Maximets.
On 10/10/23 16:12, Ales Musil wrote:
> Internally handle default CT zone limit as other limits that
> can be passed via the list with special value -1. Curently
* Currently
And there is seem to be an extra 'as' in the patch subject.
> 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>
> ---
> lib/conntrack.c | 2 +-
> lib/conntrack.h | 4 +++-
> lib/ct-dpif.c | 28 +++++++++++++++-------------
> lib/ct-dpif.h | 16 ++++++++--------
> lib/dpctl.c | 14 +++++++-------
> lib/dpif-netdev.c | 17 +----------------
> lib/dpif-netlink.c | 38 +++++++++++++-------------------------
> lib/dpif-provider.h | 9 +++------
> 8 files changed, 51 insertions(+), 77 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..4c3c4aaf8 100644
> --- a/lib/conntrack.h
> +++ b/lib/conntrack.h
> @@ -127,6 +127,8 @@ enum {
> MAX_ZONE = 0xFFFF,
> };
>
> +BUILD_ASSERT_DECL(CT_DPIF_DEFAULT_ZONE == DEFAULT_ZONE);
> +
> struct ct_dpif_entry;
> struct ct_dpif_tuple;
>
> @@ -154,6 +156,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..686e95c92 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 == CT_DPIF_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 == CT_DPIF_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..c90dc9476 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -232,12 +232,14 @@ struct dpif;
> struct dpif_ipf_status;
> struct ipf_dump_ctx;
>
> +#define CT_DPIF_DEFAULT_ZONE -1
It looks a little strange to have 3 different constants in the same
codebase that mean the same thing and must all have the same value.
Also, CT_DPIF_DEFAULT_ZONE is not a very informative name, it's not
a general default zone, it's only a default zone when we talk about
zone limits.
Can we just use OVS_ZONE_LIMIT_DEFAULT_ZONE here instead?
> +
> struct ct_dpif_dump_state {
> struct dpif *dpif;
> };
>
> 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 +309,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 +330,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..ad104372e 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, CT_DPIF_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,16 @@ dpctl_ct_get_limits(int argc, const char *argv[],
> }
>
> if (argc > i) {
> + ct_dpif_push_zone_limit(&list_query, CT_DPIF_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..636a09f1a 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,7 +9483,7 @@ dpif_netdev_ct_get_limits(struct dpif *dpif,
> }
> }
> } else {
> - for (int z = MIN_ZONE; z <= MAX_ZONE; z++) {
> + for (int z = DEFAULT_ZONE; z <= MAX_ZONE; z++) {
This looks dangerous. Currently nothing says that DEFAULT_ZONE
has to be smaller than MIN_ZONE by exaclty one.
> czl = zone_limit_get(dp->conntrack, z);
> if (czl.zone == z) {
> ct_dpif_push_zone_limit(zone_limits_reply, z, czl.limit,
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 9194971d3..3f12d0c9d 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -3358,9 +3358,10 @@ dpif_netlink_ct_flush(struct dpif *dpif OVS_UNUSED,
> const uint16_t *zone,
> }
> }
>
> +BUILD_ASSERT_DECL(CT_DPIF_DEFAULT_ZONE == OVS_ZONE_LIMIT_DEFAULT_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) {
> @@ -3376,17 +3377,11 @@ dpif_netlink_ct_set_limits(struct dpif *dpif
> OVS_UNUSED,
> ovs_header = ofpbuf_put_uninit(request, sizeof *ovs_header);
> ovs_header->dp_ifindex = 0;
>
> - 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)) {
> + size_t opt_offset;
> + opt_offset = nl_msg_start_nested(request,
> + OVS_CT_LIMIT_ATTR_ZONE_LIMIT);
> +
> struct ct_dpif_zone_limit *zone_limit;
>
> LIST_FOR_EACH (zone_limit, node, zone_limits) {
> @@ -3396,8 +3391,9 @@ dpif_netlink_ct_set_limits(struct dpif *dpif OVS_UNUSED,
> };
> nl_msg_put(request, &req_zone_limit, sizeof req_zone_limit);
> }
> +
> + nl_msg_end_nested(request, opt_offset);
> }
> - nl_msg_end_nested(request, opt_offset);
>
> int err = nl_transact(NETLINK_GENERIC, request, NULL);
If the list is empty, previously we sent a request with an empty
OVS_CT_LIMIT_ATTR_ZONE_LIMIT, which is fine. Now we will send
a request with no attributes, which is an error.
> ofpbuf_delete(request);
> @@ -3406,7 +3402,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 +3434,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) {
> } else {
> ct_dpif_push_zone_limit(zone_limits, zone_limit->zone_id,
> @@ -3456,7 +3449,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 +3469,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 +3486,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..3ccf018e7 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -520,10 +520,8 @@ 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
> @@ -531,8 +529,7 @@ struct dpif_class {
> * (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. */
The comment here needs an update as well. We should also, probably,
add that this API reports a default limit as well as limits for all
previously configured zones if the list is empty. It is not obvious.
> - int (*ct_get_limits)(struct dpif *, uint32_t *default_limit,
> - const struct ovs_list *zone_limits_in,
> + 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
