| Message ID | 20231010141224.638166-2-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | Expose CT limit via DB | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
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 <amusil@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
On Mon, Oct 16, 2023 at 9:08 PM Ilya Maximets <i.maximets@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 <amusil@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
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 + 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++) { 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); 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. */ - 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
Internally handle default CT zone limit as other limits that can be passed via the list with special value -1. Curently 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 <amusil@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(-)