On 10/18/23 09:56, 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> > --- > v5: Rebase on top of current master. > Address comments from Ilya: > - Make sure the zones are always removed. > - Fix style related problems. > - Make sure the limit is initialized to -1. > 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 | 39 +++++++++++++++++++++++++ > ofproto/ofproto-dpif.h | 5 ++++ > ofproto/ofproto-provider.h | 4 +++ > ofproto/ofproto.c | 12 ++++++++ > ofproto/ofproto.h | 2 ++ > tests/system-traffic.at | 54 +++++++++++++++++++++++++++++++++++ > vswitchd/bridge.c | 58 +++++++++++++++++++++++++++++++------- > 7 files changed, 164 insertions(+), 10 deletions(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index ba5706f6a..6a931a806 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,38 @@ ct_del_zone_timeout_policy(const char *datapath_type, > uint16_t zone_id) > } > } > > +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 +6952,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. */
This function can use a behavior explanation, i.e. what happens if the limit is NULL, or what value 'zone' zone should take for a default 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..649add089 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. */ > > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h > index 8efdb20a0..7ce6a65e1 100644 > --- a/ofproto/ofproto.h > +++ b/ofproto/ofproto.h > @@ -384,6 +384,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 25e8c3f75..445a9ffbd 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -5221,6 +5221,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 set-zone-limit $DP_TYPE zone=0 limit=3]) > +AT_CHECK([ovs-vsctl set-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)"]) Maybe a loop instead of copying almost the same thing? It can also probably be E.g. for i in 2 3 4 5 6; do packet="50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000${i}00080000" AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 \ "in_port=2 packet=${packet} actions=resubmit(,0)"]) done > + > +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]) > + > +AT_CHECK([ovs-vsctl set-zone-limit $DP_TYPE default 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-limit $DP_TYPE default]) > +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..4545556b5 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. */ May add '-1 if not specified.' to the comment. > 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. */ Should probably be ct_zone_default_limit. Might have the same comment update. > }; > > /* All bridges, indexed by name. */ > @@ -662,6 +664,7 @@ ct_zone_alloc(uint16_t zone_id, struct > ovsrec_ct_timeout_policy *tp_cfg) > struct ct_zone *ct_zone = xzalloc(sizeof *ct_zone); > > ct_zone->zone_id = zone_id; > + ct_zone->limit = -1; > simap_init(&ct_zone->tp); > get_timeout_policy_from_ovsrec(&ct_zone->tp, tp_cfg); > return ct_zone; > @@ -670,6 +673,14 @@ ct_zone_alloc(uint16_t zone_id, struct > ovsrec_ct_timeout_policy *tp_cfg) > static void > ct_zone_remove_and_destroy(struct datapath *dp, struct ct_zone *ct_zone) > { > + if (!simap_is_empty(&ct_zone->tp)) { > + ofproto_ct_del_zone_timeout_policy(dp->type, ct_zone->zone_id); > + } > + > + if (ct_zone->limit > -1) { > + ofproto_ct_zone_limit_update(dp->type, ct_zone->zone_id, NULL); > + } > + > hmap_remove(&dp->ct_zones, &ct_zone->node); > simap_destroy(&ct_zone->tp); > free(ct_zone); > @@ -706,6 +717,7 @@ datapath_create(const char *type) > { > struct datapath *dp = xzalloc(sizeof *dp); > dp->type = xstrdup(type); > + dp->ct_default_limit = -1; > hmap_init(&dp->ct_zones); > hmap_insert(&all_datapaths, &dp->node, hash_string(type, 0)); > smap_init(&dp->caps); > @@ -722,6 +734,11 @@ datapath_destroy(struct datapath *dp) > ct_zone_remove_and_destroy(dp, ct_zone); > } > > + if (dp->ct_default_limit > -1) { > + ofproto_ct_zone_limit_update(dp->type, > OVS_ZONE_LIMIT_DEFAULT_ZONE, > + NULL); > + } > + > hmap_remove(&all_datapaths, &dp->node); > hmap_destroy(&dp->ct_zones); > free(dp->type); > @@ -743,29 +760,50 @@ 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)) { > 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) { > + ofproto_ct_zone_limit_update(dp->type, zone_id, zone_cfg->limit); > + ct_zone->limit = desired_limit; > + } > + > 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) { > - ofproto_ct_del_zone_timeout_policy(dp->type, ct_zone->zone_id); > 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) { > + ofproto_ct_zone_limit_update(dp->type, OVS_ZONE_LIMIT_DEFAULT_ZONE, > + dp_cfg->ct_zone_default_limit); > + dp->ct_default_limit = default_limit; > + } > + > } > > static void _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev