On Tue, Nov 28, 2023 at 2:34 PM Ilya Maximets <[email protected]> wrote:
> On 11/2/23 13:00, 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 <[email protected]> > > --- > > v6: Rebase on top of current master. > > Address comments from Ilya: > > - Add assert to conntrack.h for the zone numbers. > > - Some minot cosmetic changes. > > 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 | 7 +++++-- > > lib/ct-dpif.c | 28 +++++++++++++++------------- > > lib/ct-dpif.h | 14 ++++++-------- > > lib/dpctl.c | 15 ++++++++------- > > lib/dpif-netdev.c | 21 ++++++--------------- > > lib/dpif-netlink.c | 29 ++++++----------------------- > > lib/dpif-provider.h | 24 +++++++++++------------- > > 8 files changed, 58 insertions(+), 82 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..18c182f85 100644 > > --- a/lib/conntrack.h > > +++ b/lib/conntrack.h > > @@ -122,11 +122,14 @@ 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, > > }; > > > > +BUILD_ASSERT_DECL(DEFAULT_ZONE > INVALID_ZONE && DEFAULT_ZONE < > MIN_ZONE); > > + > > struct ct_dpif_entry; > > struct ct_dpif_tuple; > > > > @@ -154,6 +157,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 b8f065d1d..7ce99dcec 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -9450,17 +9450,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) { > > @@ -9475,20 +9468,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) { > > @@ -9502,6 +9487,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)); > > Nit: the count is not meaningful for the default limit, > because it's not an actual zone. We do not print it > to the user, but we shouldn't really read it either. > > > + } > > + > > 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..5f92a2b65 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,11 +3430,8 @@ 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 || > > - zone_limit->zone_id > UINT16_MAX) { > > - } else { > > + if (zone_limit->zone_id >= OVS_ZONE_LIMIT_DEFAULT_ZONE && > > + zone_limit->zone_id <= UINT16_MAX) { > > ct_dpif_push_zone_limit(zone_limits, zone_limit->zone_id, > > zone_limit->limit, > zone_limit->count); > > } > > @@ -3456,7 +3444,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 +3464,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 +3481,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..c9f6fffe6 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); > > - > > - /* Looks up the default per zone limit and stores that in > > - * 'default_limit'. Look up the per zone limits for all zones in > > - * 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, > > + * is not used when setting limits). */ > > + int (*ct_set_limits)(struct dpif *, const struct ovs_list > *zone_limits); > > + > > + /* Looks up the per zone limits for all zones in 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. If the 'zone_limits_in' list is empty > the > > + * report will contain all previously set zone limits and the > default > > + * limit. */ > > Maybe mention that the 'count' is not used for the output for the defualt > limit as well. > > > + 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, everything should be addressed in v7. Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
