On 10/10/23 16:12, Ales Musil wrote:
> Propagate the CT limit that is present in the DB into
> datapath. The limit is currently only propagated on change
> and can be overwritten by the dpctl commands.
>
> Signed-off-by: Ales Musil <amu...@redhat.com>
> ---
> v4: Rebase on top of current master.
> Make sure that the values from DB are propagated only if set. That
> applies to both limit and policies.
> ---
> ofproto/ofproto-dpif.c | 41 ++++++++++++++++++++++++++
> ofproto/ofproto-dpif.h | 5 ++++
> ofproto/ofproto-provider.h | 4 +++
> ofproto/ofproto.c | 16 ++++++++--
> ofproto/ofproto.h | 4 +++
> tests/system-traffic.at | 54 ++++++++++++++++++++++++++++++++++
> vswitchd/bridge.c | 60 +++++++++++++++++++++++++++++++-------
> 7 files changed, 171 insertions(+), 13 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index ba5706f6a..4fdbf0ef0 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -220,6 +220,7 @@ static void ofproto_unixctl_init(void);
> static void ct_zone_config_init(struct dpif_backer *backer);
> static void ct_zone_config_uninit(struct dpif_backer *backer);
> static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
> +static void ct_zone_limits_commit(struct dpif_backer *backer);
>
> static inline struct ofproto_dpif *
> ofproto_dpif_cast(const struct ofproto *ofproto)
> @@ -513,6 +514,7 @@ type_run(const char *type)
>
> process_dpif_port_changes(backer);
> ct_zone_timeout_policy_sweep(backer);
> + ct_zone_limits_commit(backer);
>
> return 0;
> }
> @@ -5522,6 +5524,8 @@ ct_zone_config_init(struct dpif_backer *backer)
> cmap_init(&backer->ct_zones);
> hmap_init(&backer->ct_tps);
> ovs_list_init(&backer->ct_tp_kill_list);
> + ovs_list_init(&backer->ct_zone_limits_to_add);
> + ovs_list_init(&backer->ct_zone_limits_to_del);
> clear_existing_ct_timeout_policies(backer);
> }
>
> @@ -5545,6 +5549,8 @@ ct_zone_config_uninit(struct dpif_backer *backer)
> id_pool_destroy(backer->tp_ids);
> cmap_destroy(&backer->ct_zones);
> hmap_destroy(&backer->ct_tps);
> + ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_add);
> + ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_del);
> }
>
> static void
> @@ -5625,6 +5631,40 @@ ct_del_zone_timeout_policy(const char *datapath_type,
> uint16_t zone_id)
> }
> }
>
> +BUILD_ASSERT_DECL(OFPROTO_CT_DEFAULT_ZONE_ID == CT_DPIF_DEFAULT_ZONE);
Why this assertion added here?
> +
> +static void
> +ct_zone_limit_update(const char *datapath_type, int32_t zone_id,
> + int64_t *limit)
> +{
> + struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
> + datapath_type);
> + if (!backer) {
> + return;
> + }
> +
> + if (limit) {
> + ct_dpif_push_zone_limit(&backer->ct_zone_limits_to_add, zone_id,
> + *limit, 0);
> + } else {
> + ct_dpif_push_zone_limit(&backer->ct_zone_limits_to_del, zone_id, 0,
> 0);
> + }
> +}
> +
> +static void
> +ct_zone_limits_commit(struct dpif_backer *backer)
> +{
> + if (!ovs_list_is_empty(&backer->ct_zone_limits_to_add)) {
> + ct_dpif_set_limits(backer->dpif, &backer->ct_zone_limits_to_add);
> + ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_add);
> + }
> +
> + if (!ovs_list_is_empty(&backer->ct_zone_limits_to_del)) {
> + ct_dpif_del_limits(backer->dpif, &backer->ct_zone_limits_to_del);
> + ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_del);
> + }
> +}
> +
> static void
> get_datapath_cap(const char *datapath_type, struct smap *cap)
> {
> @@ -6914,4 +6954,5 @@ const struct ofproto_class ofproto_dpif_class = {
> ct_flush, /* ct_flush */
> ct_set_zone_timeout_policy,
> ct_del_zone_timeout_policy,
> + ct_zone_limit_update,
> };
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index d8e0cd37a..b863dd6fc 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -284,6 +284,11 @@ struct dpif_backer {
> feature than 'bt_support'. */
>
> struct atomic_count tnl_count;
> +
> + struct ovs_list ct_zone_limits_to_add; /* CT zone limits queued for
> + * addition into datapath. */
> + struct ovs_list ct_zone_limits_to_del; /* CT zone limt queued for
> + * deletion from datapath. */
> };
>
> /* All existing ofproto_backer instances, indexed by ofproto->up.type. */
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 9f7b8b6e8..33fb99280 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -1921,6 +1921,10 @@ struct ofproto_class {
> /* Deletes the timeout policy associated with 'zone' in datapath type
> * 'dp_type'. */
> void (*ct_del_zone_timeout_policy)(const char *dp_type, uint16_t zone);
> +
> + /* Updates the CT zone limit for specified zone. */
> + void (*ct_zone_limit_update)(const char *dp_type, int32_t zone,
> + int64_t *limit);
> };
>
> extern const struct ofproto_class ofproto_dpif_class;
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index e78c80d11..6df3f1b27 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -1026,6 +1026,18 @@ ofproto_ct_del_zone_timeout_policy(const char
> *datapath_type, uint16_t zone_id)
>
> }
>
> +void
> +ofproto_ct_zone_limit_update(const char *datapath_type, int32_t zone_id,
> + int64_t *limit)
> +{
> + datapath_type = ofproto_normalize_type(datapath_type);
> + const struct ofproto_class *class = ofproto_class_find__(datapath_type);
> +
> + if (class && class->ct_zone_limit_update) {
> + class->ct_zone_limit_update(datapath_type, zone_id, limit);
> + }
> +}
> +
>
> /* Spanning Tree Protocol (STP) configuration. */
>
> @@ -6366,7 +6378,7 @@ handle_flow_mod__(struct ofproto *ofproto, const struct
> ofputil_flow_mod *fm,
> error = ofproto_flow_mod_start(ofproto, &ofm);
> if (!error) {
> ofproto_bump_tables_version(ofproto);
> - error = ofproto_flow_mod_finish(ofproto, &ofm, req);
> + error = ofproto_flow_mod_finish(ofproto, &ofm, req);
The whitespace thing again.
> ofmonitor_flush(ofproto->connmgr);
> }
> ovs_mutex_unlock(&ofproto_mutex);
> @@ -8437,7 +8449,7 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id,
> uint16_t flags)
> /* Send error referring to the original message. */
> ofconn_send_error(ofconn, be->msg, error);
> error = OFPERR_OFPBFC_MSG_FAILED;
> -
> +
And here.
> /* 2. Revert. Undo all the changes made above. */
> LIST_FOR_EACH_REVERSE_CONTINUE(be, node, &bundle->msg_list) {
> if (be->type == OFPTYPE_FLOW_MOD) {
> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> index 8efdb20a0..bba4a9e0e 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -53,6 +53,8 @@ struct lldp_status;
> struct aa_settings;
> struct aa_mapping_settings;
>
> +#define OFPROTO_CT_DEFAULT_ZONE_ID -1
> +
> /* Needed for the lock annotations. */
> extern struct ovs_mutex ofproto_mutex;
>
> @@ -384,6 +386,8 @@ void ofproto_ct_set_zone_timeout_policy(const char
> *datapath_type,
> struct simap *timeout_policy);
> void ofproto_ct_del_zone_timeout_policy(const char *datapath_type,
> uint16_t zone);
> +void ofproto_ct_zone_limit_update(const char *datapath_type, int32_t zone_id,
> + int64_t *limit);
> void ofproto_get_datapath_cap(const char *datapath_type,
> struct smap *dp_cap);
>
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index f35cfaad9..d2897feb6 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -5220,6 +5220,60 @@ default limit=0
> zone=4,limit=0,count=0
> ])
>
> +dnl Test limit set via database.
> +VSCTL_ADD_DATAPATH_TABLE()
> +
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=0])
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=3])
> +
> +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=10])
> +AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=3])
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> +default limit=10
> +zone=0,limit=5,count=0
> +])
> +
> +AT_CHECK([ovs-vsctl add-zone-limit $DP_TYPE zone=0 limit=3])
> +AT_CHECK([ovs-vsctl add-zone-limit $DP_TYPE zone=3 limit=3])
> +
> +OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/ct-get-limits], [dnl
> +default limit=10
> +zone=0,limit=3,count=0
> +zone=3,limit=3,count=0])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2
> packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000200080000
> actions=resubmit(,0)"])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2
> packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000300080000
> actions=resubmit(,0)"])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2
> packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000400080000
> actions=resubmit(,0)"])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2
> packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000500080000
> actions=resubmit(,0)"])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2
> packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000600080000
> actions=resubmit(,0)"])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.3," |
> sort ], [0], [dnl
> +udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=2),reply=(src=10.1.1.4,dst=10.1.1.3,sport=2,dport=1),zone=3
> +udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=3),reply=(src=10.1.1.4,dst=10.1.1.3,sport=3,dport=1),zone=3
> +udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=4),reply=(src=10.1.1.4,dst=10.1.1.3,sport=4,dport=1),zone=3
> +])
> +
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> +default limit=10
> +zone=0,limit=3,count=0
> +zone=3,limit=3,count=3
> +])
> +
> +AT_CHECK([ovs-vsctl del-zone-limit $DP_TYPE zone=3])
> +OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/ct-get-limits], [dnl
> +default limit=10
> +zone=0,limit=3,count=0])
Are we waiting for all the ct entries to expire here? Might take
a lot of time unnecessarily.
> +
> +AT_CHECK([ovs-vsctl set-zone-default-limit $DP_TYPE limit=5])
> +OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/ct-get-limits], [dnl
> +default limit=5
> +zone=0,limit=3,count=0])
> +
> +AT_CHECK([ovs-vsctl del-zone-default-limit $DP_TYPE])
> +OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/ct-get-limits], [dnl
> +default limit=0
> +zone=0,limit=3,count=0])
> +
> OVS_TRAFFIC_VSWITCHD_STOP(["dnl
> /could not create datapath/d
> /(Cannot allocate memory) on packet/d"])
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index e9110c1d8..1e02cc8df 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -157,6 +157,7 @@ struct aa_mapping {
> /* Internal representation of conntrack zone configuration table in OVSDB. */
> struct ct_zone {
> uint16_t zone_id;
> + int64_t limit; /* Limit of allowed entries. */
> struct simap tp; /* A map from timeout policy attribute to
> * timeout value. */
> struct hmap_node node; /* Node in 'struct datapath' 'ct_zones'
> @@ -176,6 +177,7 @@ struct datapath {
> unsigned int last_used; /* The last idl_seqno that this 'datapath'
> * used in OVSDB. This number is used for
> * garbage collection. */
> + int64_t ct_default_limit; /* Default limit for CT zones. */
> };
>
> /* All bridges, indexed by name. */
> @@ -722,6 +724,11 @@ datapath_destroy(struct datapath *dp)
> ct_zone_remove_and_destroy(dp, ct_zone);
Missed updating zone limits here?
> }
>
> + if (dp->ct_default_limit > -1) {
> + ofproto_ct_zone_limit_update(dp->type,
> OFPROTO_CT_DEFAULT_ZONE_ID,
> + NULL);
> + }
> +
> hmap_remove(&all_datapaths, &dp->node);
> hmap_destroy(&dp->ct_zones);
> free(dp->type);
> @@ -743,29 +750,60 @@ ct_zones_reconfigure(struct datapath *dp, struct
> ovsrec_datapath *dp_cfg)
> struct ovsrec_ct_timeout_policy *tp_cfg = zone_cfg->timeout_policy;
>
> ct_zone = ct_zone_lookup(&dp->ct_zones, zone_id);
> - if (ct_zone) {
> - struct simap new_tp = SIMAP_INITIALIZER(&new_tp);
> - get_timeout_policy_from_ovsrec(&new_tp, tp_cfg);
> - if (update_timeout_policy(&ct_zone->tp, &new_tp)) {
> + if (!ct_zone) {
> + ct_zone = ct_zone_alloc(zone_id, tp_cfg);
> + hmap_insert(&dp->ct_zones, &ct_zone->node, hash_int(zone_id, 0));
> + }
> +
> + struct simap new_tp = SIMAP_INITIALIZER(&new_tp);
> + get_timeout_policy_from_ovsrec(&new_tp, tp_cfg);
> +
> + if (update_timeout_policy(&ct_zone->tp, &new_tp)) {
> + if (simap_count(&ct_zone->tp)) {
Use simap_is_empty() instead.
> ofproto_ct_set_zone_timeout_policy(dp->type,
> ct_zone->zone_id,
> &ct_zone->tp);
> + } else {
> + ofproto_ct_del_zone_timeout_policy(dp->type,
> ct_zone->zone_id);
> }
> - } else {
> - ct_zone = ct_zone_alloc(zone_id, tp_cfg);
> - hmap_insert(&dp->ct_zones, &ct_zone->node, hash_int(zone_id, 0));
> - ofproto_ct_set_zone_timeout_policy(dp->type, ct_zone->zone_id,
> - &ct_zone->tp);
> }
> +
> + int64_t desired_limit = zone_cfg->limit ? *zone_cfg->limit : -1;
> + if (ct_zone->limit != desired_limit) {
The 'limit' is not initialiezd to -1 by ct_zone_alloc().
> + ofproto_ct_zone_limit_update(dp->type, zone_id, zone_cfg->limit);
> + }
> +
> + ct_zone->limit = desired_limit;
This can be moved into the 'if' statement.
> ct_zone->last_used = idl_seqno;
> }
>
> /* Purge 'ct_zone's no longer found in the database. */
> HMAP_FOR_EACH_SAFE (ct_zone, node, &dp->ct_zones) {
> - if (ct_zone->last_used != idl_seqno) {
> + if (ct_zone->last_used == idl_seqno) {
> + continue;
> + }
> +
> + if (simap_count(&ct_zone->tp)) {
> ofproto_ct_del_zone_timeout_policy(dp->type, ct_zone->zone_id);
> - ct_zone_remove_and_destroy(dp, ct_zone);
> }
> +
> + if (ct_zone->limit > -1) {
> + ofproto_ct_zone_limit_update(dp->type, ct_zone->zone_id, NULL);
> + }
Can we move the checks above into ct_zone_remove_and_destroy() ?
This will also help with the missed zone limit updates on datapath
destruction.
> +
> + ct_zone_remove_and_destroy(dp, ct_zone);
> + }
> +
> + /* Reconfigure default CT zone limit if needed. */
> + int64_t default_limit = dp_cfg->ct_zone_default_limit
> + ? *dp_cfg->ct_zone_default_limit
> + : -1;
> +
> + if (dp->ct_default_limit != default_limit) {
'ct_default_limit' is used uninitialized here? It is initialized
with xzalloc, but it's not -1, i.e. we will issue an unnecessary
update command.
> + ofproto_ct_zone_limit_update(dp->type, OFPROTO_CT_DEFAULT_ZONE_ID,
> + dp_cfg->ct_zone_default_limit);
> }
> +
> + dp->ct_default_limit = default_limit;
Can be moved into the 'if'.
> }
>
> static void
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
