On Mon, Oct 16, 2023 at 9:08 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
> 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
>
>
Thank you for the review, all should be addressed in v5.
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
