On Thu, Oct 5, 2023 at 2:49 PM Aaron Conole <acon...@redhat.com> wrote:
> Ales Musil <amu...@redhat.com> writes: > > > Progpagate 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> > > Acked-by: Simon Horman <ho...@ovn.org> > > --- > > v3: Rebase on top of current master. > > Add ack from Simon and fix the missing '.'. > > Revert the unrelated change. > > --- > > ofproto/ofproto-dpif.c | 39 ++++++++++++++++++++++++++++++++ > > ofproto/ofproto-dpif.h | 5 +++++ > > ofproto/ofproto-provider.h | 5 +++++ > > ofproto/ofproto.c | 12 ++++++++++ > > ofproto/ofproto.h | 2 ++ > > tests/system-traffic.at | 46 +++++++++++++++++++++++++++++++++++++- > > vswitchd/bridge.c | 9 ++++++++ > > 7 files changed, 117 insertions(+), 1 deletion(-) > > > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index ba5706f6a..55eaeefa3 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, uint16_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, NULL, > &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..5604aa65b 100644 > > --- a/ofproto/ofproto-provider.h > > +++ b/ofproto/ofproto-provider.h > > @@ -1921,6 +1921,11 @@ 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); > > + > > + /* Queues the CT zone limit update. In order for this change to take > > + * effect it needs to be commited. */ > > + void (*ct_zone_limit_update)(const char *dp_type, uint16_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..e054753b8 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, uint16_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..700a6f087 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, uint16_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 418cd32fe..537db66e0 100644 > > --- a/tests/system-traffic.at > > +++ b/tests/system-traffic.at > > @@ -5195,9 +5195,53 @@ > udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=3),reply=(src=10.1.1.4,dst=10. > > > 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 > > ]) > > > > +dnl Test limit set via database. > > +VSCTL_ADD_DATAPATH_TABLE() > > + > > +AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=0]) > > I wouldn't block the series over this since the rest of the test assumes > that zone=0 is okay, but I think a good follow up would be to rewrite to > not use zone=0 in the system-traffic tests. Zone 0 is the default zone, > and can possibly get polluted on some systems by other entries, so it > probably is best to avoid it in the future. > That's fair, once this is merged I can do a follow up patch that will change zone 0 to something else. > > > +AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=3]) > > + > > +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]) > > + > > OVS_TRAFFIC_VSWITCHD_STOP(["dnl > > /could not create datapath/d > > -/(Cannot allocate memory) on packet/d"]) > > +/(Cannot allocate memory) on packet/d > > +/failed to .* timeout policy/d"]) > > AT_CLEANUP > > > > AT_SETUP([FTP - no conntrack]) > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > > index e9110c1d8..6cd09e145 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' > > @@ -756,6 +757,13 @@ ct_zones_reconfigure(struct datapath *dp, struct > ovsrec_datapath *dp_cfg) > > 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; > > } > > > > @@ -763,6 +771,7 @@ ct_zones_reconfigure(struct datapath *dp, struct > ovsrec_datapath *dp_cfg) > > 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); > > + ofproto_ct_zone_limit_update(dp->type, ct_zone->zone_id, > NULL); > > ct_zone_remove_and_destroy(dp, ct_zone); > > } > > } > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> amu...@redhat.com <https://red.ht/sig> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev