On Wed, Oct 25, 2023 at 1:35 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
> 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
>
>
Thank you for the review, all should be addressed in v6.
Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
