Felix Huettner <[email protected]> writes: > Am Wed, Jul 01, 2026 at 03:06:57PM -0400 schrieb Aaron Conole: >> Felix Huettner <[email protected]> writes: >> >> > Am Mon, Jun 29, 2026 at 02:37:54PM -0400 schrieb Aaron Conole: >> >> Felix Huettner <[email protected]> writes: >> >> >> >> > Am Fri, Jun 26, 2026 at 06:01:03PM -0400 schrieb Aaron Conole via dev: >> >> >> Felix Huettner via dev <[email protected]> writes: >> >> >> >> >> >> > Previously modifications to the list of conntrack entries required >> >> >> > taking a global lock. As zones are generally separate from each other >> >> >> > there is no need for such a global lock. This allows multiple pmds to >> >> >> > handle changes to different zones in parallel. >> >> >> > >> >> >> > During the removal of the global lock we also extracted the > zone limits >> >> >> > to be part of the individual zones. This skips an additional need for >> >> >> > global locks. >> >> >> > >> >> >> > In addition this required a restructuring of conntrack_cleanup to no >> >> >> > longer use rculists and instead iterate over each zone. This > also allows >> >> >> > to parallalize such operations in the future. >> >> >> > >> >> >> > The new "ovstest test-conntrack benchmark-tcp" shows the benefits >> >> >> > nicely. They mostly exist on connections that are setup and > teared down >> >> >> > all the time (maybe as part of some portscanning or similar). >> >> >> > >> >> >> > When using the same testcases as in the previous commits >> >> >> > Below lists the total time for the testrun with these various >> >> >> > configs. >> >> >> > | | Without this patch | With this patch | >> >> >> > | Config 1 | 70.5 s | 17.9 s | >> >> >> > | Config 2 | 80.7 s | 22.9 s | >> >> >> > | Config 3 | 65.9 s | 19.2 s | >> >> >> > | Config 4 | 71.4 s | 18.5 s | >> >> >> >> >> >> This is a really large change. And the following are some large >> >> >> regressions: >> >> >> >> >> >> > | Config 5 | 43.3 s | 47.0 s | >> >> >> > | Config 6 | 46.6 s | 51.1 s | >> >> >> >> >> >> I think they might be tied to how the new list iteration happens - IE >> >> >> we >> >> >> now walk all connections in the zone rather than having an expiration >> >> >> list. >> >> >> >> >> >> But I wonder how it will look with even more connections than config 6. >> >> >> >> >> >> Did you test with hundreds of thousands of connections? I would be >> >> >> more >> >> >> interested in a dataset with larger numbers of connections, rather than >> >> >> micro-benchmark. I suspect the architecture of this is showing that >> >> >> for small numbers of connections (maybe even a small number of >> >> >> thousand) >> >> >> things look good. >> >> >> >> >> >> Maybe try with something like 4 threads, and 46,877 connections per >> >> >> zone? Just some way to exercise the clean thread so it needs to >> >> >> continue (with the benchmarks above it barely wakes). >> >> > >> >> > Thanks thats a good point. >> >> > I tried it now with: >> >> > * 4 threads >> >> > * 50k connections per thread >> >> > * 4 zones >> >> > * 20 data packets per connection >> >> > * 100 iterations >> >> > >> >> > Before this patch: 80s >> >> > After this patch: 175s >> >> > At the end of the series: 52s >> > >> > Hi Aaron, >> > >> > thanks for the feedback. >> > >> >> >> >> Thanks for confirming. I wanted to be sure before I guessed wrong about >> >> the rest of the series. >> >> >> >> My architectural analysis is this - the performance wins here are from >> >> the per-zone lock, and the batched deletion. Probably also clipping the >> >> sleep (since without a global lock the cleanup can become continuous). >> >> The tradeoff is based on the connection pattern that we expect. For >> >> example, if all the connections are under high churn (which is most of >> >> the benchmarking here), it's easy to show improvement with the full list >> >> traversal, because the full expiration list is being evaluated anyway. >> >> With the expiration list approach we have, we have an early-exit >> >> guarantee at the cost of needing to keep the list in order. But I think >> >> some of the bits around that are simply properties of using rcu-list vs >> >> cmap. >> > >> > I would generally agree with your findings regarding the improvements. >> > >> > However i don't see the early-exit possiblility of the expiration list. >> > I can not find any code that sorts the exp_lists according to the expiry >> > timestamp of their connections and also ct_sweep just seems to >> > iterate over all elements in a list. >> > >> > It seems that before 3d9c1b855a5fbc0205aed0b38722cd98c6a074b0 there was >> > actual tracking of the expiry time. But it was removed due to the >> > performance cost of maintaining the ordered list. >> >> Ugh... yes, you're right. It got removed, but we kept the exp_list >> around - for some reason I continue to think of exp lists as being >> ordered. >> >> > Afterwards the current exp_lists now rather seem to ensure we equally >> > clean connections across all zones so no single zone can starve the >> > cleaning. But with the ct_sweep changes later in the series that is >> > solved as well in here. >> > >> > Thats why i do not get why we should keep these lists. Without ordering >> > they seem not helpful compared to just iterating over all >> > connections. >> >> Maybe they can be removed. Recently, I've been working with NVIDIA for >> ct offloading. In the code so far, we just tell the hardware about the >> ct objects we are cleaning based on the exp list details. I think it >> probably can be okay to remove - but it means that hwol logic may become >> a bit more complex. Gaetan can comment if he thinks there might be some >> problems with this approach. (See >> https://mail.openvswitch.org/pipermail/ovs-dev/2026-April/431605.html) >> >> >> I think you have the right idea - the global lock is really doing too >> >> much - guarding the insert, guarding the exp_lists, and the >> >> lookup+insert atomicity guarantee. And that means we have three >> >> critical sections that all converge to the same stall. >> >> >> >> Maybe we can solve this with a smaller scoped change - create the >> >> finer-grained lock array per-zone, and then the insert can take the >> >> ct_locks[zone] instead of the global lock. At least, that's something >> >> similar to what Paolo proposed many years back, and we didn't adopt it >> >> because the exp-list showed a real advantage, and the difficulty of >> >> doing per-bucket locks with the way we currently have implemented >> >> the conn storage (maybe I'm misremembering). The exp lists only need a >> >> lock when doing list modifications, and I think we can convert them to >> >> per-zone instead of the current 100-buckets thing since it's difficult >> >> to imagine how to make that work. Then the exp-list can share the same >> >> lock array (after all, it already basically does - just an array of >> >> one, and we'd just be expanding to per-zone). >> >> >> >> An approach like that with zone scoped locks would also be a bit smaller >> >> in the impact, I think. We're just adding an array of locks, changing >> >> the exp-list definition, and updating the ct lock call sites (like >> >> you're already doing in this series), without needing to create new cmap >> >> primitives, fundamentally change how the sweep works, etc. >> >> >> >> I'd expect it could be like 5-ish broad steps: >> >> >> >> 1. Introduce the benchmark (same as you have) >> >> >> >> 2. Introduce the per-zone "lock and exp-list": >> >> >> >> struct conntrack_zone { >> >> struct ovs_mutex lock; >> >> struct rculist exp_list; >> >> }; >> >> >> >> and initialize / destroy in the init and destroy functions. >> >> NO CHANGE >> > >> > connections are also already stored per zone, thats why i included them >> > in there as well. >> > >> >> >> >> 3. convert expiry lists to the per-zone zone: >> >> >> >> remove exp_lists[N_EXP_LISTS], and N_EXP_LISTS >> >> go through ct->zones[zone].exp_list >> >> conn_clean__() removes from the zone exp_list >> >> sweep iterates zones[current_zone].exp_list directly >> >> >> >> (still under the ct lock) >> >> NO CHANGE >> >> >> >> 4. replace ct_lock with per-zone lock >> >> update the annotations, and hit all of the ct_lock call sites that >> >> are needed (maybe everything in the hot path). >> >> PERFORMANCE BOOST >> > >> > The thing is that also the zone_limits are in the hot insert path. The >> > current code could be tuned to not require a lock in the common case >> > where there is already conntrack_zone_limit or none is needed. >> > >> > But here again i had the question of why we should have that indirection >> > of having a separate struct (which we need to search for) when we could >> > just >> > combine it in the zone that already needs it anyway. It takes an additional >> > 24 bytes for each zone (so 1,5MB in total). But from my feeling it makes >> > the resulting code significantly simpler in the common case. >> > >> > In addition we get the count of connections in all zones, even if > there is no >> > limit set. This also fixes the issue (which i just saw by chance) that >> > existing connections are ignored in the zone limit, if the limit is created >> > after the connection already exists. >> >> Yes, it's a good fix. >> >> >> >> >> 5. batch deletion and drop the mandatory sleep >> >> PERFORMANCE BOOST >> > >> > I think we should not skip the partial zone cleaning. If you have a zone >> > with a large amount of churn and a large limit then the ct_sweep would >> > otherwise clean all of them in a single run. This would mean we might >> > take a while until we hit an rcu quiesced state. >> > I always understood that this is not desirable. But if thats not too bad >> > i am fine with leaving it out. >> >> I understand this creates a large memory spike while we wait, so it's >> not desirable. The partial zone cleaning should be included. >> >> >> >> >> Those are my expectations of what would happen, and I think it's >> >> probably simpler to follow for others. >> >> >> >> WDYT about this approach? >> > >> > I have the felling i am missing something important, so maybe what i >> > wrote above does not make sense because of that. >> >> Maybe the whole series can be considered, but at the moment, it's a bit >> difficult to review because so much happens in 05/10 compared to the >> others. For example, this patch: >> >> 1. Introduces zone locking >> 2. Removes exp_lists >> 3. Rewrites sweep logic >> >> It is probably better to start by introducing the API / calls / smaller >> strutures first. For example: >> >> - benchmark patch >> - introduce conntrack zone struct with just the cmap for conns, and >> just migrate conns object calls. >> - another patch add per-zone lock object, and update the conn >> insert/remove code. >> - merge in the zone limits in another patch >> - remove exp_lists (remove the rculist node, remove the push_front >> call, introduce the sweep_zone function and have conntrack_clean use the >> zone loop) >> - implement the partial clean >> - introduce the cmap cursor-pos-cursor tooling >> - update the position based iterator >> - introduce batching >> - remove the sleep >> >> Finally, I'd suggest a tail patch (maybe it's 11 or 12 based on how you >> think about the above) and that patch should update NEWS (just like >> 3d9c1b855a5fbc0205aed0b38722cd98c6a074b0) to note the improvement. That >> also means that the benchmark improvements really should just be first >> and last patches (ie: details of the tests on introduction, and details >> after the series is completed). The intermediary improvements are less >> important. >> >> This is just a suggestion for reworking the series structure. Right >> now, there's so much mixed it is difficult to review (and I didn't >> really properly get everything, I'm sure, nor did I fully look at patch >> 06/10, 07/10 just because of the amount of change here). >> >> I think also the benchmark patch (either first or 'last') can also >> include the 50k+ connections test as config7, if possible. The one that >> showed the "regression" here. >> >> WDYT? > > Hi Aaron, > > that sounds good. I'll clean this up and then send a new version. > However as i am unavailable for a little while i might only send an > update in 2 weeks or so.
That's okay. > Thanks, > Felix > >> >> > Already thanks a lot for clarifying. >> > Felix >> > >> >> >> >> > So while it gets worse here at the end of the series we are better than >> >> > before. >> >> > >> >> > I would add such benchmark to all patches of the series, to have some >> >> > comparison. >> >> > >> >> >> >> >> >> > >> >> >> > Signed-off-by: Felix Huettner <[email protected]> >> >> >> > --- >> >> >> > lib/conntrack-private.h | 36 ++-- >> >> >> > lib/conntrack.c | 445 > ++++++++++++++-------------------------- >> >> >> > 2 files changed, 166 insertions(+), 315 deletions(-) >> >> >> > >> >> >> > diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h >> >> >> > index 0141afd4a..8282bbe5a 100644 >> >> >> > --- a/lib/conntrack-private.h >> >> >> > +++ b/lib/conntrack-private.h >> >> >> > @@ -134,9 +134,6 @@ struct conn { >> >> >> > * True as soon as a thread has started freeing >> >> >> > * its memory. */ >> >> >> > >> >> >> > - /* Inserted once by a PMD, then managed by the 'ct_clean' > thread. */ >> >> >> > - struct rculist node; >> >> >> > - >> >> >> > /* Mutable data. */ >> >> >> > struct ovs_mutex lock; /* Guards all mutable fields. */ >> >> >> > ovs_u128 label; >> >> >> > @@ -144,10 +141,6 @@ struct conn { >> >> >> > uint32_t mark; >> >> >> > int seq_skew; >> >> >> > >> >> >> > - /* Immutable data. */ >> >> >> > - int32_t admit_zone; /* The zone for managing zone limit > counts. */ >> >> >> > - uint32_t zone_limit_seq; /* Used to disambiguate zone limit >> > counts. */ >> >> >> > - >> >> >> > /* Mutable data. */ >> >> >> > bool seq_skew_dir; /* TCP sequence skew direction due to >> > NATTing of FTP >> >> >> > * control messages; true if reply direction. */ >> >> >> > @@ -198,32 +191,33 @@ enum ct_ephemeral_range { >> >> >> > #define FOR_EACH_PORT_IN_RANGE(curr, min, max) \ >> >> >> > FOR_EACH_PORT_IN_RANGE__(curr, min, max, OVS_JOIN(idx, > __COUNTER__)) >> >> >> > >> >> >> > -#define ZONE_LIMIT_CONN_DEFAULT -1 >> >> >> > +#define CONN_LIMIT_NONE -1 >> >> >> > +#define CONN_LIMIT_USE_DEFAULT -2 >> >> >> > >> >> >> > -struct conntrack_zone_limit { >> >> >> > +struct conntrack_zone { >> >> >> > int32_t zone; >> >> >> > - atomic_int64_t limit; >> >> >> > - atomic_count count; >> >> >> > - uint32_t zone_limit_seq; /* Used to disambiguate zone limit >> > counts. */ >> >> >> > + >> >> >> > + struct ovs_mutex zone_lock; /* Protects the following fields. */ >> >> >> > + struct cmap conns; >> >> >> > + >> >> >> > + /* Limits */ >> >> >> > + atomic_int64_t limit; /* Currently active limit. */ >> >> >> > + atomic_int64_t requested_limit; /* User requested limit. May be >> >> >> > + * ZONE_LIMIT_CONN_DEFAULT if it should use >> >> >> > + * the default limit. */ >> >> >> > + atomic_count count; /* Number of connections currently > tracked. */ >> >> >> > }; >> >> >> > >> >> >> > struct conntrack { >> >> >> > struct ovs_mutex ct_lock; /* Protects the following fields. */ >> >> >> > - struct cmap conns[UINT16_MAX + 1]; >> >> >> > - struct rculist exp_lists[N_EXP_LISTS]; >> >> >> > - struct cmap zone_limits; >> >> >> > + struct conntrack_zone zones[UINT16_MAX + 1]; >> >> >> > struct cmap timeout_policies; >> >> >> > - uint32_t zone_limit_seq OVS_GUARDED; /* Used to disambiguate >> > zone limit >> >> >> > - * counts. */ >> >> >> > atomic_uint32_t default_zone_limit; >> >> >> > >> >> >> > uint32_t hash_basis; /* Salt for hashing a connection key. */ >> >> >> > pthread_t clean_thread; /* Periodically cleans up connection >> >> > tracker. */ >> >> >> > struct latch clean_thread_exit; /* To destroy the > 'clean_thread'. */ >> >> >> > - unsigned int next_list; /* Next list where the newly created >> > connection >> >> >> > - * gets inserted. */ >> >> >> > - unsigned int next_sweep; /* List from which the gc thread >> > will resume >> >> >> > - * the sweeping. */ >> >> >> > + unsigned int next_clean_zone; /* Next zone where the clean >> >> > should run. */ >> >> >> > >> >> >> > /* Counting connections. */ >> >> >> > atomic_count n_conn; /* Number of connections currently > tracked. */ >> >> >> > diff --git a/lib/conntrack.c b/lib/conntrack.c >> >> >> > index ffff4111f..6d28cffe3 100644 >> >> >> > --- a/lib/conntrack.c >> >> >> > +++ b/lib/conntrack.c >> >> >> > @@ -41,7 +41,6 @@ >> >> >> > #include "ovs-thread.h" >> >> >> > #include "openvswitch/poll-loop.h" >> >> >> > #include "random.h" >> >> >> > -#include "rculist.h" >> >> >> > #include "timeval.h" >> >> >> > #include "unaligned.h" >> >> >> > >> >> >> > @@ -89,11 +88,6 @@ enum ct_alg_ctl_type { >> >> >> > CT_ALG_CTL_SIP, >> >> >> > }; >> >> >> > >> >> >> > -struct zone_limit { >> >> >> > - struct cmap_node node; >> >> >> > - struct conntrack_zone_limit czl; >> >> >> > -}; >> >> >> > - >> >> >> > static bool conn_key_extract(struct conntrack *, struct dp_packet *, >> >> >> > ovs_be16 dl_type, struct conn_lookup_ctx *, >> >> >> > uint16_t zone); >> >> >> > @@ -111,7 +105,6 @@ static enum ct_update_res conn_update(struct >> >> > conntrack *ct, struct conn *conn, >> >> >> > long long now); >> >> >> > static long long int conn_expiration(const struct conn *); >> >> >> > static bool conn_expired(const struct conn *, long long now); >> >> >> > -static void conn_expire_push_front(struct conntrack *ct, struct >> >> > conn *conn); >> >> >> > static void set_mark(struct dp_packet *, struct conn *, >> >> >> > uint32_t val, uint32_t mask); >> >> >> > static void set_label(struct dp_packet *, struct conn *, >> >> >> > @@ -267,14 +260,12 @@ conntrack_init(void) >> >> >> > >> >> >> > ovs_mutex_init_adaptive(&ct->ct_lock); >> >> >> > ovs_mutex_lock(&ct->ct_lock); >> >> >> > - for (unsigned i = 0; i < ARRAY_SIZE(ct->conns); i++) { >> >> >> > - cmap_init(&ct->conns[i]); >> >> >> > + for (unsigned i = 0; i < ARRAY_SIZE(ct->zones); i++) { >> >> >> > + ovs_mutex_init_adaptive(&ct->zones[i].zone_lock); >> >> >> > + cmap_init(&ct->zones[i].conns); >> >> >> > + ct->zones[i].limit = CONN_LIMIT_NONE; >> >> >> > + ct->zones[i].requested_limit = CONN_LIMIT_USE_DEFAULT; >> >> >> > } >> >> >> > - for (unsigned i = 0; i < ARRAY_SIZE(ct->exp_lists); i++) { >> >> >> > - rculist_init(&ct->exp_lists[i]); >> >> >> > - } >> >> >> > - cmap_init(&ct->zone_limits); >> >> >> > - ct->zone_limit_seq = 0; >> >> >> > timeout_policy_init(ct); >> >> >> > ovs_mutex_unlock(&ct->ct_lock); >> >> >> > >> >> >> > @@ -282,7 +273,7 @@ conntrack_init(void) >> >> >> > atomic_init(&ct->n_conn_limit, DEFAULT_N_CONN_LIMIT); >> >> >> > atomic_init(&ct->tcp_seq_chk, true); >> >> >> > atomic_init(&ct->sweep_ms, 20000); >> >> >> > - atomic_init(&ct->default_zone_limit, 0); >> >> >> > + atomic_init(&ct->default_zone_limit, CONN_LIMIT_NONE); >> >> >> > latch_init(&ct->clean_thread_exit); >> >> >> > ct->clean_thread = ovs_thread_create("ct_clean", >> >> > clean_thread_main, ct); >> >> >> > ct->ipf = ipf_init(); >> >> >> > @@ -302,114 +293,28 @@ conntrack_init(void) >> >> >> > return ct; >> >> >> > } >> >> >> > >> >> >> > -static uint32_t >> >> >> > -zone_key_hash(int32_t zone, uint32_t basis) >> >> >> > -{ >> >> >> > - size_t hash = hash_int((OVS_FORCE uint32_t) zone, basis); >> >> >> > - return hash; >> >> >> > -} >> >> >> > - >> >> >> > static int64_t >> >> >> > -zone_limit_get_limit__(struct conntrack_zone_limit *czl) >> >> >> > +zone_read_limit(struct conntrack_zone *cz) >> >> >> > { >> >> >> > int64_t limit; >> >> >> > - atomic_read_relaxed(&czl->limit, &limit); >> >> >> > - >> >> >> > + atomic_read_relaxed(&cz->limit, &limit); >> >> >> > return limit; >> >> >> > } >> >> >> > >> >> >> > -static int64_t >> >> >> > -zone_limit_get_limit(struct conntrack *ct, struct >> >> > conntrack_zone_limit *czl) >> >> >> > +static struct conntrack_zone * >> >> >> > +zone_lookup(struct conntrack *ct, int32_t zone) >> >> >> > { >> >> >> > - int64_t limit = zone_limit_get_limit__(czl); >> >> >> > - >> >> >> > - if (limit == ZONE_LIMIT_CONN_DEFAULT) { >> >> >> > - atomic_read_relaxed(&ct->default_zone_limit, &limit); >> >> >> > - limit = limit ? limit : -1; >> >> >> > + if (zone < MIN_ZONE || zone > MAX_ZONE) { >> >> >> > + return NULL; >> >> >> > } >> >> >> > >> >> >> > - return limit; >> >> >> > -} >> >> >> > - >> >> >> > -static struct zone_limit * >> >> >> > -zone_limit_lookup_protected(struct conntrack *ct, int32_t zone) >> >> >> > - OVS_REQUIRES(ct->ct_lock) >> >> >> > -{ >> >> >> > - uint32_t hash = zone_key_hash(zone, ct->hash_basis); >> >> >> > - struct zone_limit *zl; >> >> >> > - CMAP_FOR_EACH_WITH_HASH_PROTECTED (zl, node, hash, >> > &ct->zone_limits) { >> >> >> > - if (zl->czl.zone == zone) { >> >> >> > - return zl; >> >> >> > - } >> >> >> > - } >> >> >> > - return NULL; >> >> >> > -} >> >> >> > - >> >> >> > -static struct zone_limit * >> >> >> > -zone_limit_lookup(struct conntrack *ct, int32_t zone) >> >> >> > -{ >> >> >> > - uint32_t hash = zone_key_hash(zone, ct->hash_basis); >> >> >> > - struct zone_limit *zl; >> >> >> > - CMAP_FOR_EACH_WITH_HASH (zl, node, hash, &ct->zone_limits) { >> >> >> > - if (zl->czl.zone == zone) { >> >> >> > - return zl; >> >> >> > - } >> >> >> > - } >> >> >> > - return NULL; >> >> >> > -} >> >> >> > - >> >> >> > -static struct zone_limit * >> >> >> > -zone_limit_create__(struct conntrack *ct, int32_t zone, > int64_t limit) >> >> >> > - OVS_REQUIRES(ct->ct_lock) >> >> >> > -{ >> >> >> > - struct zone_limit *zl = NULL; >> >> >> > - >> >> >> > - if (zone > DEFAULT_ZONE && zone <= MAX_ZONE) { >> >> >> > - zl = xmalloc(sizeof *zl); >> >> >> > - atomic_init(&zl->czl.limit, limit); >> >> >> > - atomic_count_init(&zl->czl.count, 0); >> >> >> > - zl->czl.zone = zone; >> >> >> > - zl->czl.zone_limit_seq = ct->zone_limit_seq++; >> >> >> > - uint32_t hash = zone_key_hash(zone, ct->hash_basis); >> >> >> > - cmap_insert(&ct->zone_limits, &zl->node, hash); >> >> >> > - } >> >> >> > - >> >> >> > - return zl; >> >> >> > -} >> >> >> > - >> >> >> > -static struct zone_limit * >> >> >> > -zone_limit_create(struct conntrack *ct, int32_t zone, int64_t limit) >> >> >> > - OVS_REQUIRES(ct->ct_lock) >> >> >> > -{ >> >> >> > - struct zone_limit *zl = zone_limit_lookup_protected(ct, zone); >> >> >> > - >> >> >> > - if (zl) { >> >> >> > - return zl; >> >> >> > - } >> >> >> > - >> >> >> > - return zone_limit_create__(ct, zone, limit); >> >> >> > -} >> >> >> > - >> >> >> > -/* Lazily creates a new entry in the zone_limits cmap if > default limit >> >> >> > - * is set and there's no entry for the zone. */ >> >> >> > -static struct zone_limit * >> >> >> > -zone_limit_lookup_or_default(struct conntrack *ct, int32_t zone) >> >> >> > - OVS_REQUIRES(ct->ct_lock) >> >> >> > -{ >> >> >> > - struct zone_limit *zl = zone_limit_lookup_protected(ct, zone); >> >> >> > - >> >> >> > - if (!zl) { >> >> >> > - uint32_t limit; >> >> >> > - atomic_read_relaxed(&ct->default_zone_limit, &limit); >> >> >> > - >> >> >> > - if (limit) { >> >> >> > - zl = zone_limit_create__(ct, zone, > ZONE_LIMIT_CONN_DEFAULT); >> >> >> > - } >> >> >> > - } >> >> >> > - >> >> >> > - return zl; >> >> >> > + return &ct->zones[zone]; >> >> >> > } >> >> >> > >> >> >> > +/* Returns the conntrack_zone_info for the requested zone. >> >> >> > + * Note: to be compatible with the kernel implementation we > return empty >> >> >> > + * entries if the default limit is to be used. >> >> >> > + * This can be improved upon later. */ >> >> >> > struct conntrack_zone_info >> >> >> > zone_limit_get(struct conntrack *ct, int32_t zone) >> >> >> > { >> >> >> > @@ -418,82 +323,66 @@ zone_limit_get(struct conntrack *ct, > int32_t zone) >> >> >> > .limit = 0, >> >> >> > .count = 0, >> >> >> > }; >> >> >> > - struct zone_limit *zl = zone_limit_lookup(ct, zone); >> >> >> > - if (zl) { >> >> >> > - int64_t czl_limit = zone_limit_get_limit__(&zl->czl); >> >> >> > - if (czl_limit > ZONE_LIMIT_CONN_DEFAULT) { >> >> >> > - czl.zone = zl->czl.zone; >> >> >> > - czl.limit = czl_limit; >> >> >> > - } else { >> >> >> > - atomic_read_relaxed(&ct->default_zone_limit, >> >> >> > &czl.limit); >> >> >> > - } >> >> >> > - >> >> >> > - czl.count = atomic_count_get(&zl->czl.count); >> >> >> > - } else { >> >> >> > + if (zone == DEFAULT_ZONE) { >> >> >> > atomic_read_relaxed(&ct->default_zone_limit, &czl.limit); >> >> >> > + } else { >> >> >> > + struct conntrack_zone *cz = zone_lookup(ct, zone); >> >> >> > + uint64_t req_limit; >> >> >> > + atomic_read_relaxed(&cz->requested_limit, &req_limit); >> >> >> > + if (req_limit != CONN_LIMIT_USE_DEFAULT) { >> >> >> > + czl.zone = zone; >> >> >> > + czl.limit = zone_read_limit(cz); >> >> >> > + czl.count = atomic_count_get(&cz->count); >> >> >> > + } >> >> >> > + } >> >> >> > + >> >> >> > + if (czl.limit == CONN_LIMIT_NONE) { >> >> >> > + czl.limit = 0; >> >> >> > } >> >> >> > >> >> >> > return czl; >> >> >> > } >> >> >> > >> >> >> > -static void >> >> >> > -zone_limit_clean__(struct conntrack *ct, struct zone_limit *zl) >> >> >> > - OVS_REQUIRES(ct->ct_lock) >> >> >> > +static bool >> >> >> > +zone_limit_reset(struct conntrack *ct, struct conntrack_zone *cz) >> >> >> > { >> >> >> > - uint32_t hash = zone_key_hash(zl->czl.zone, ct->hash_basis); >> >> >> > - cmap_remove(&ct->zone_limits, &zl->node, hash); >> >> >> > - ovsrcu_postpone(free, zl); >> >> >> > -} >> >> >> > - >> >> >> > -static void >> >> >> > -zone_limit_clean(struct conntrack *ct, struct zone_limit *zl) >> >> >> > - OVS_REQUIRES(ct->ct_lock) >> >> >> > -{ >> >> >> > - uint32_t limit; >> >> >> > + int64_t limit; >> >> >> > + atomic_read_relaxed(&cz->requested_limit, &limit); >> >> >> > + if (limit == CONN_LIMIT_USE_DEFAULT) { >> >> >> > + return false; >> >> >> > + } >> >> >> > >> >> >> > atomic_read_relaxed(&ct->default_zone_limit, &limit); >> >> >> > - /* Do not remove the entry if the default limit is enabled, but >> >> >> > - * simply move the limit to default. */ >> >> >> > - if (limit) { >> >> >> > - atomic_store_relaxed(&zl->czl.limit, > ZONE_LIMIT_CONN_DEFAULT); >> >> >> > - } else { >> >> >> > - zone_limit_clean__(ct, zl); >> >> >> > - } >> >> >> > + atomic_store_relaxed(&cz->limit, limit); >> >> >> > + atomic_store_relaxed(&cz->requested_limit, > CONN_LIMIT_USE_DEFAULT); >> >> >> > + return true; >> >> >> > } >> >> >> > >> >> >> > + >> >> >> > static void >> >> >> > -zone_limit_clean_default(struct conntrack *ct) >> >> >> > - OVS_REQUIRES(ct->ct_lock) >> >> >> > +zone_limit_update_default(struct conntrack *ct, uint32_t limit) >> >> >> > { >> >> >> > - struct zone_limit *zl; >> >> >> > - int64_t czl_limit; >> >> >> > + int64_t cz_req_limit; >> >> >> > >> >> >> > - atomic_store_relaxed(&ct->default_zone_limit, 0); >> >> >> > + atomic_store_relaxed(&ct->default_zone_limit, limit); >> >> >> > >> >> >> > - CMAP_FOR_EACH (zl, node, &ct->zone_limits) { >> >> >> > - atomic_read_relaxed(&zl->czl.limit, &czl_limit); >> >> >> > - if (zone_limit_get_limit__(&zl->czl) == >> > ZONE_LIMIT_CONN_DEFAULT) { >> >> >> > - zone_limit_clean__(ct, zl); >> >> >> > + for (unsigned i = 0; i < ARRAY_SIZE(ct->zones); i++) { >> >> >> > + atomic_read_relaxed(&ct->zones[i].requested_limit, >> > &cz_req_limit); >> >> >> > + if (cz_req_limit == CONN_LIMIT_USE_DEFAULT) { >> >> >> > + atomic_store_relaxed(&ct->zones[i].limit, limit); >> >> >> > } >> >> >> > } >> >> >> > } >> >> >> > >> >> >> > static bool >> >> >> > zone_limit_delete__(struct conntrack *ct, int32_t zone) >> >> >> > - OVS_REQUIRES(ct->ct_lock) >> >> >> > { >> >> >> > - struct zone_limit *zl = NULL; >> >> >> > - >> >> >> > if (zone == DEFAULT_ZONE) { >> >> >> > - zone_limit_clean_default(ct); >> >> >> > + zone_limit_update_default(ct, 0); >> >> >> > + return false; >> >> >> > } else { >> >> >> > - zl = zone_limit_lookup_protected(ct, zone); >> >> >> > - if (zl) { >> >> >> > - zone_limit_clean(ct, zl); >> >> >> > - } >> >> >> > + return zone_limit_reset(ct, zone_lookup(ct, zone)); >> >> >> > } >> >> >> > - >> >> >> > - return zl != NULL; >> >> >> > } >> >> >> > >> >> >> > int >> >> >> > @@ -501,9 +390,7 @@ zone_limit_delete(struct conntrack *ct, > int32_t zone) >> >> >> > { >> >> >> > bool deleted; >> >> >> > >> >> >> > - ovs_mutex_lock(&ct->ct_lock); >> >> >> > deleted = zone_limit_delete__(ct, zone); >> >> >> > - ovs_mutex_unlock(&ct->ct_lock); >> >> >> > >> >> >> > if (zone != DEFAULT_ZONE) { >> >> >> > VLOG_INFO(deleted >> >> >> > @@ -515,45 +402,27 @@ zone_limit_delete(struct conntrack *ct, >> > int32_t zone) >> >> >> > return 0; >> >> >> > } >> >> >> > >> >> >> > -static void >> >> >> > -zone_limit_update_default(struct conntrack *ct, int32_t zone, >> >> > uint32_t limit) >> >> >> > -{ >> >> >> > - /* limit zero means delete default. */ >> >> >> > - if (limit == 0) { >> >> >> > - ovs_mutex_lock(&ct->ct_lock); >> >> >> > - zone_limit_delete__(ct, zone); >> >> >> > - ovs_mutex_unlock(&ct->ct_lock); >> >> >> > - } else { >> >> >> > - atomic_store_relaxed(&ct->default_zone_limit, limit); >> >> >> > - } >> >> >> > -} >> >> >> > - >> >> >> > int >> >> >> > zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t >> >> >> > limit) >> >> >> > { >> >> >> > - struct zone_limit *zl; >> >> >> > + struct conntrack_zone *cz; >> >> >> > int err = 0; >> >> >> > >> >> >> > if (zone == DEFAULT_ZONE) { >> >> >> > - zone_limit_update_default(ct, zone, limit); >> >> >> > + zone_limit_update_default(ct, limit); >> >> >> > VLOG_INFO("Set default zone limit to %u", limit); >> >> >> > return err; >> >> >> > } >> >> >> > >> >> >> > - zl = zone_limit_lookup(ct, zone); >> >> >> > - if (zl) { >> >> >> > - atomic_store_relaxed(&zl->czl.limit, limit); >> >> >> > - VLOG_INFO("Changed zone limit of %u for zone %d", limit, > zone); >> >> >> > + cz = zone_lookup(ct, zone); >> >> >> > + if (cz) { >> >> >> > + atomic_store_relaxed(&cz->limit, limit); >> >> >> > + atomic_store_relaxed(&cz->requested_limit, limit); >> >> >> > + VLOG_INFO("Set zone limit of %u for zone %d", limit, zone); >> >> >> > } else { >> >> >> > - ovs_mutex_lock(&ct->ct_lock); >> >> >> > - err = zone_limit_create(ct, zone, limit) == NULL; >> >> >> > - ovs_mutex_unlock(&ct->ct_lock); >> >> >> > - if (!err) { >> >> >> > - VLOG_INFO("Created zone limit of %u for zone %d", limit, >> > zone); >> >> >> > - } else { >> >> >> > - VLOG_WARN("Request to create zone limit for invalid zone %d", >> >> >> > - zone); >> >> >> > - } >> >> >> > + VLOG_WARN("Request to create zone limit for invalid zone >> >> >> > %d", >> >> >> > + zone); >> >> >> > + err = 1; >> >> >> > } >> >> >> > >> >> >> > return err; >> >> >> > @@ -561,47 +430,47 @@ zone_limit_update(struct conntrack *ct, >> >> > int32_t zone, uint32_t limit) >> >> >> > >> >> >> > static void >> >> >> > conn_clean__(struct conntrack *ct, struct conn *conn) >> >> >> > - OVS_REQUIRES(ct->ct_lock) >> >> >> > { >> >> >> > + uint16_t fwd_zone; >> >> >> > + struct conntrack_zone *cz; >> >> >> > uint32_t hash; >> >> >> > >> >> >> > if (conn->alg) { >> >> >> > expectation_clean(ct, &conn->key_node[CT_DIR_FWD].key); >> >> >> > } >> >> >> > >> >> >> > + fwd_zone = conn->key_node[CT_DIR_FWD].key.zone; >> >> >> > + cz = zone_lookup(ct, fwd_zone); >> >> >> > hash = conn_key_hash(&conn->key_node[CT_DIR_FWD].key, >> > ct->hash_basis); >> >> >> > - cmap_remove(&ct->conns[conn->key_node[CT_DIR_FWD].key.zone], >> >> >> > + ovs_mutex_lock(&cz->zone_lock); >> >> >> > + cmap_remove(&cz->conns, >> >> >> > &conn->key_node[CT_DIR_FWD].cm_node, hash); >> >> >> > + atomic_count_dec(&cz->count); >> >> >> > >> >> >> > if (conn->nat_action) { >> >> >> > + ovs_assert(fwd_zone == conn->key_node[CT_DIR_REV].key.zone); >> >> >> > hash = conn_key_hash(&conn->key_node[CT_DIR_REV].key, >> >> >> > ct->hash_basis); >> >> >> > - cmap_remove(&ct->conns[conn->key_node[CT_DIR_REV].key.zone], >> >> >> > + >> >> >> > + cmap_remove(&cz->conns, >> >> >> > &conn->key_node[CT_DIR_REV].cm_node, hash); >> >> >> > } >> >> >> > >> >> >> > - rculist_remove(&conn->node); >> >> >> > + ovs_mutex_unlock(&cz->zone_lock); >> >> >> > } >> >> >> > >> >> >> > /* Also removes the associated nat 'conn' from the lookup >> >> >> > datastructures. */ >> >> >> > static void >> >> >> > conn_clean(struct conntrack *ct, struct conn *conn) >> >> >> > - OVS_EXCLUDED(conn->lock, ct->ct_lock) >> >> >> > + OVS_EXCLUDED(conn->lock) >> >> >> > { >> >> >> > if (atomic_flag_test_and_set(&conn->reclaimed)) { >> >> >> > return; >> >> >> > } >> >> >> > >> >> >> > COVERAGE_INC(conntrack_remove); >> >> >> > - ovs_mutex_lock(&ct->ct_lock); >> >> >> > conn_clean__(ct, conn); >> >> >> > - ovs_mutex_unlock(&ct->ct_lock); >> >> >> > - >> >> >> > - struct zone_limit *zl = zone_limit_lookup(ct, conn->admit_zone); >> >> >> > - if (zl && zl->czl.zone_limit_seq == conn->zone_limit_seq) { >> >> >> > - atomic_count_dec(&zl->czl.count); >> >> >> > - } >> >> >> > >> >> >> > ovsrcu_postpone(delete_conn, conn); >> >> >> > atomic_count_dec(&ct->n_conn); >> >> >> > @@ -619,26 +488,10 @@ conn_force_expire(struct conn *conn) >> >> >> > void >> >> >> > conntrack_destroy(struct conntrack *ct) >> >> >> > { >> >> >> > - struct conn *conn; >> >> >> > - >> >> >> > latch_set(&ct->clean_thread_exit); >> >> >> > pthread_join(ct->clean_thread, NULL); >> >> >> > latch_destroy(&ct->clean_thread_exit); >> >> >> > >> >> >> > - for (unsigned i = 0; i < N_EXP_LISTS; i++) { >> >> >> > - RCULIST_FOR_EACH (conn, node, &ct->exp_lists[i]) { >> >> >> > - conn_clean(ct, conn); >> >> >> > - } >> >> >> > - } >> >> >> > - >> >> >> > - struct zone_limit *zl; >> >> >> > - CMAP_FOR_EACH (zl, node, &ct->zone_limits) { >> >> >> > - uint32_t hash = zone_key_hash(zl->czl.zone, ct->hash_basis); >> >> >> > - >> >> >> > - cmap_remove(&ct->zone_limits, &zl->node, hash); >> >> >> > - ovsrcu_postpone(free, zl); >> >> >> > - } >> >> >> > - >> >> >> > struct timeout_policy *tp; >> >> >> > CMAP_FOR_EACH (tp, node, &ct->timeout_policies) { >> >> >> > uint32_t hash = hash_int(tp->policy.id, ct->hash_basis); >> >> >> > @@ -649,14 +502,14 @@ conntrack_destroy(struct conntrack *ct) >> >> >> > >> >> >> > conntrack_flush(ct, NULL); >> >> >> > >> >> >> > - ovs_mutex_lock(&ct->ct_lock); >> >> >> > - for (unsigned i = 0; i < ARRAY_SIZE(ct->conns); i++) { >> >> >> > - cmap_destroy(&ct->conns[i]); >> >> >> > + for (unsigned i = 0; i < ARRAY_SIZE(ct->zones); i++) { >> >> >> > + ovs_mutex_lock(&ct->zones[i].zone_lock); >> >> >> > + cmap_destroy(&ct->zones[i].conns); >> >> >> > + ovs_mutex_unlock(&ct->zones[i].zone_lock); >> >> >> > + ovs_mutex_destroy(&ct->zones[i].zone_lock); >> >> >> > } >> >> >> > - cmap_destroy(&ct->zone_limits); >> >> >> > cmap_destroy(&ct->timeout_policies); >> >> >> > >> >> >> > - ovs_mutex_unlock(&ct->ct_lock); >> >> >> > ovs_mutex_destroy(&ct->ct_lock); >> >> >> > >> >> >> > ovs_mutex_lock(&ct->resources_lock); >> >> >> > @@ -672,7 +525,7 @@ conntrack_destroy(struct conntrack *ct) >> >> >> > >> >> >> > >> >> >> > static bool >> >> >> > -conn_key_lookup(struct conntrack *ct, const struct conn_key *key, >> >> >> > +conn_key_lookup(struct conntrack_zone *cz, const struct > conn_key *key, >> >> >> > uint32_t hash, long long now, struct conn >> >> >> > **conn_out, >> >> >> > bool *reply) >> >> >> > { >> >> >> > @@ -680,7 +533,8 @@ conn_key_lookup(struct conntrack *ct, const >> >> > struct conn_key *key, >> >> >> > struct conn *conn = NULL; >> >> >> > bool found = false; >> >> >> > >> >> >> > - CMAP_FOR_EACH_WITH_HASH (keyn, cm_node, hash, >> > &ct->conns[key->zone]) { >> >> >> > + CMAP_FOR_EACH_WITH_HASH (keyn, cm_node, hash, >> >> >> > + &cz->conns) { >> >> >> > if (keyn->dir == CT_DIR_FWD) { >> >> >> > conn = CONTAINER_OF(keyn, struct conn, key_node[CT_DIR_FWD]); >> >> >> > } else { >> >> >> > @@ -712,12 +566,21 @@ out_found: >> >> >> > return found; >> >> >> > } >> >> >> > >> >> >> > +static bool >> >> >> > +conn_lookup_zone(struct conntrack *ct, struct conntrack_zone *cz, >> >> >> > + const struct conn_key *key, long long now, >> >> >> > + struct conn **conn_out, bool *reply) >> >> >> > +{ >> >> >> > + uint32_t hash = conn_key_hash(key, ct->hash_basis); >> >> >> > + return conn_key_lookup(cz, key, hash, now, conn_out, reply); >> >> >> > +} >> >> >> > + >> >> >> > static bool >> >> >> > conn_lookup(struct conntrack *ct, const struct conn_key *key, >> >> >> > long long now, struct conn **conn_out, bool *reply) >> >> >> > { >> >> >> > - uint32_t hash = conn_key_hash(key, ct->hash_basis); >> >> >> > - return conn_key_lookup(ct, key, hash, now, conn_out, reply); >> >> >> > + struct conntrack_zone *cz = &ct->zones[key->zone]; >> >> >> > + return conn_lookup_zone(ct, cz, key, now, conn_out, reply); >> >> >> > } >> >> >> > >> >> >> > static void >> >> >> > @@ -1026,27 +889,25 @@ ct_verify_helper(const char *helper, enum >> >> > ct_alg_ctl_type ct_alg_ctl) >> >> >> > } >> >> >> > >> >> >> > static struct conn * >> >> >> > -conn_insert(struct conntrack *ct, struct dp_packet *pkt, >> >> >> > +conn_insert(struct conntrack *ct, struct conntrack_zone *cz, >> >> >> > + struct dp_packet *pkt, >> >> >> > struct conn_lookup_ctx *ctx, long long now, >> >> >> > const struct nat_action_info_t *nat_action_info, >> >> >> > const char *helper, const struct alg_exp_node *alg_exp, >> >> >> > enum ct_alg_ctl_type ct_alg_ctl, uint32_t tp_id) >> >> >> > - OVS_REQUIRES(ct->ct_lock) >> >> >> > + OVS_REQUIRES(cz->zone_lock) >> >> >> > { >> >> >> > COVERAGE_INC(conntrack_insert); >> >> >> > struct conn *nc = NULL; >> >> >> > >> >> >> > int64_t czl_limit; >> >> >> > struct conn_key_node *fwd_key_node, *rev_key_node; >> >> >> > - struct zone_limit *zl = zone_limit_lookup_or_default(ct, >> >> >> > - ctx->key.zone); >> >> >> > - if (zl) { >> >> >> > - czl_limit = zone_limit_get_limit(ct, &zl->czl); >> >> >> > - if (czl_limit >= 0 && >> >> >> > - atomic_count_get(&zl->czl.count) >= czl_limit) { >> >> >> > - COVERAGE_INC(conntrack_zone_full); >> >> >> > - return nc; >> >> >> > - } >> >> >> > + >> >> >> > + czl_limit = zone_read_limit(cz); >> >> >> > + if (czl_limit != CONN_LIMIT_NONE && >> >> >> > + atomic_count_get(&cz->count) >= czl_limit) { >> >> >> > + COVERAGE_INC(conntrack_zone_full); >> >> >> > + return nc; >> >> >> >> >> >> Previously, limit=0 would delete the zone limit and reset to default. >> >> >> But CONN_LIMIT_NONE is -1; and if someone changes the zone limit to '0' >> >> >> they will max out all connections on the zone. >> >> >> >> >> >> I don't see that accounted for here. >> >> > >> >> > Actually if i see it correctly setting it to 0 would not remove the >> >> > limit, but set it to unlimited. >> >> > I'll fix that, thanks. >> >> > >> >> >> >> >> >> > } >> >> >> > >> >> >> > unsigned int n_conn_limit; >> >> >> > @@ -1080,13 +941,6 @@ conn_insert(struct conntrack *ct, struct >> >> > dp_packet *pkt, >> >> >> > fwd_key_node->dir = CT_DIR_FWD; >> >> >> > rev_key_node->dir = CT_DIR_REV; >> >> >> > >> >> >> > - if (zl) { >> >> >> > - nc->admit_zone = zl->czl.zone; >> >> >> > - nc->zone_limit_seq = zl->czl.zone_limit_seq; >> >> >> > - } else { >> >> >> > - nc->admit_zone = INVALID_ZONE; >> >> >> > - } >> >> >> > - >> >> >> > if (nat_action_info) { >> >> >> > nc->nat_action = nat_action_info->nat_action; >> >> >> > >> >> >> > @@ -1108,18 +962,14 @@ conn_insert(struct conntrack *ct, struct >> >> > dp_packet *pkt, >> >> >> > nat_packet(pkt, nc, false, ctx->icmp_related); >> >> >> > uint32_t rev_hash = conn_key_hash(&rev_key_node->key, >> >> >> > ct->hash_basis); >> >> >> > - cmap_insert(&ct->conns[ctx->key.zone], >> >> >> > + cmap_insert(&ct->zones[ctx->key.zone].conns, >> >> >> > &rev_key_node->cm_node, rev_hash); >> >> >> > } >> >> >> > >> >> >> > - cmap_insert(&ct->conns[ctx->key.zone], >> >> >> > + cmap_insert(&ct->zones[ctx->key.zone].conns, >> >> >> > &fwd_key_node->cm_node, ctx->hash); >> >> >> > - conn_expire_push_front(ct, nc); >> >> >> > atomic_count_inc(&ct->n_conn); >> >> >> > - >> >> >> > - if (zl) { >> >> >> > - atomic_count_inc(&zl->czl.count); >> >> >> > - } >> >> >> > + atomic_count_inc(&cz->count); >> >> >> > >> >> >> > ctx->conn = nc; /* For completeness. */ >> >> >> > >> >> >> > @@ -1165,6 +1015,7 @@ conn_maybe_not_found(struct conntrack *ct, >> >> > struct dp_packet *pkt, >> >> >> > { >> >> >> > COVERAGE_INC(conntrack_maybe_not_found); >> >> >> > struct conn *nc = NULL; >> >> >> > + struct conntrack_zone *cz = zone_lookup(ct, ctx->key.zone); >> >> >> > >> >> >> > /* Note that we only insert a connection if commit=true. In this >> >> >> > * case we must ensure that the connection is not already part >> >> >> > of >> >> >> > @@ -1175,19 +1026,19 @@ conn_maybe_not_found(struct conntrack *ct, >> >> > struct dp_packet *pkt, >> >> >> > * We do not use multiple small if's as this confused clangs > locking >> >> >> > * analysis. */ >> >> >> > if (commit) { >> >> >> > - ovs_mutex_lock(&ct->ct_lock); >> >> >> > - bool found = conn_lookup(ct, &ctx->key, now, NULL, NULL); >> >> >> > + ovs_mutex_lock(&cz->zone_lock); >> >> >> > + bool found = conn_lookup_zone(ct, cz, &ctx->key, now, NULL, >> > NULL); >> >> >> > if (!found) { >> >> >> > if (!pkt_set_new_ct_state(pkt, ctx, alg_exp)) { >> >> >> > - ovs_mutex_unlock(&ct->ct_lock); >> >> >> > + ovs_mutex_unlock(&cz->zone_lock); >> >> >> > return nc; >> >> >> > } >> >> >> > - nc = conn_insert(ct, pkt, ctx, now, nat_action_info, >> >> >> > + nc = conn_insert(ct, cz, pkt, ctx, now, nat_action_info, >> >> >> > helper, alg_exp, ct_alg_ctl, tp_id); >> >> >> > } >> >> >> > - ovs_mutex_unlock(&ct->ct_lock); >> >> >> > + ovs_mutex_unlock(&cz->zone_lock); >> >> >> > } else { >> >> >> > - bool found = conn_lookup(ct, &ctx->key, now, NULL, NULL); >> >> >> > + bool found = conn_lookup_zone(ct, cz, &ctx->key, now, NULL, >> > NULL); >> >> >> > if (!found) { >> >> >> > pkt_set_new_ct_state(pkt, ctx, alg_exp); >> >> >> > } >> >> >> > @@ -1399,7 +1250,8 @@ initial_conn_lookup(struct conntrack *ct, >> >> > struct conn_lookup_ctx *ctx, >> >> >> > conn_key_reverse(&ctx->key); >> >> >> > } >> >> >> > >> >> >> > - conn_key_lookup(ct, &ctx->key, ctx->hash, now, &ctx->conn, >> >> > &ctx->reply); >> >> >> > + struct conntrack_zone *cz = &ct->zones[ctx->key.zone]; >> >> >> > + conn_key_lookup(cz, &ctx->key, ctx->hash, now, &ctx->conn, >> >> > &ctx->reply); >> >> >> > >> >> >> > if (natted) { >> >> >> > if (OVS_LIKELY(ctx->conn)) { >> >> >> > @@ -1426,6 +1278,8 @@ process_one(struct conntrack *ct, struct >> >> > dp_packet *pkt, >> >> >> > const struct nat_action_info_t *nat_action_info, >> >> >> > const char *helper, uint32_t tp_id) >> >> >> > { >> >> >> > + ovs_assert(ctx->key.zone == zone); >> >> >> > + >> >> >> > /* Reset ct_state whenever entering a new zone. */ >> >> >> > if (pkt->md.ct_state && pkt->md.ct_zone != zone) { >> >> >> > pkt->md.ct_state = 0; >> >> >> > @@ -1630,28 +1484,34 @@ conntrack_get_sweep_interval(struct >> > conntrack *ct) >> >> >> > } >> >> >> > >> >> >> > static size_t >> >> >> > -ct_sweep(struct conntrack *ct, struct rculist *list, long long now, >> >> >> > - size_t *cleaned_count) >> >> >> > +ct_sweep_zone(struct conntrack *ct, uint16_t zone, long long now, >> >> >> > + size_t *cleaned_count) >> >> >> >> >> >> This used to be O(exp_list), but is now O(zone_conn_list). See above. >> >> > >> >> > If i got exp_lists correctly they contained each connection once. So for >> >> > a conn that includes nat we previously only visited it once, but now we >> >> > visit it 2x (because of the reverse direciton). Otherwise the only >> >> > difference is where the list we iterate over is stored. >> >> > >> >> > More ideal would be to store the connections in their expiry order, but >> >> > i think maintaing that order is more costly than scanning all of them. >> >> > >> >> > The idea to do this per zone is to keep the locks we need to take as >> >> > localized as possible. Also it could later allow the ct_clean thread to >> >> > be adapted to handle separate zones. E.g. you could limit the amount of >> >> > connections handled for each zone, to ensure a zone with large churn >> >> > does not take up large amount of cleaning resources. But thats just an >> >> > idea. >> >> > >> >> > (Also see above) >> >> > >> >> >> >> >> >> > OVS_NO_THREAD_SAFETY_ANALYSIS >> >> >> > { >> >> >> > + struct conn_key_node *keyn; >> >> >> > + struct conntrack_zone *cz; >> >> >> > + unsigned int conn_count = 0; >> >> >> > + unsigned int cleaned = 0; >> >> >> > struct conn *conn; >> >> >> > - size_t cleaned = 0; >> >> >> > - size_t count = 0; >> >> >> > + long long expiration; >> >> >> > >> >> >> > - RCULIST_FOR_EACH (conn, node, list) { >> >> >> > - if (conn_expired(conn, now)) { >> >> >> > - conn_clean(ct, conn); >> >> >> > - cleaned++; >> >> >> > + cz = zone_lookup(ct, zone); >> >> >> > + CMAP_FOR_EACH (keyn, cm_node, &cz->conns) { >> >> >> > + if (keyn->dir != CT_DIR_FWD) { >> >> >> > + continue; >> >> >> > } >> >> >> > >> >> >> > - count++; >> >> >> > - } >> >> >> > + conn = CONTAINER_OF(keyn, struct conn, key_node[keyn->dir]); >> >> >> > + expiration = conn_expiration(conn); >> >> >> > + if (now >= expiration) { >> >> >> > + conn_clean(ct, conn); >> >> >> > + cleaned++; >> >> >> > + } >> >> >> >> >> >> ^ Spacing >> >> >> >> >> >> > >> >> >> > - if (cleaned_count) { >> >> >> > - *cleaned_count = cleaned; >> >> >> > + conn_count++; >> >> >> > } >> >> >> > - >> >> >> > - return count; >> >> >> > + *cleaned_count = cleaned; >> >> >> > + return conn_count; >> >> >> > } >> >> >> > >> >> >> > /* Cleans up old connection entries from 'ct'. Returns the time >> >> >> > @@ -1664,23 +1524,28 @@ conntrack_clean(struct conntrack *ct, >> > long long now) >> >> >> > unsigned int n_conn_limit, i; >> >> >> > size_t clean_end, count = 0; >> >> >> > size_t total_cleaned = 0; >> >> >> > + uint16_t current_zone = ct->next_clean_zone; >> >> >> > >> >> >> > atomic_read_relaxed(&ct->n_conn_limit, &n_conn_limit); >> >> >> > clean_end = n_conn_limit / 64; >> >> >> > >> >> >> > - for (i = ct->next_sweep; i < N_EXP_LISTS; i++) { >> >> >> > - size_t cleaned; >> >> >> > + for (i = 0; i < ARRAY_SIZE(ct->zones); i++) { >> >> >> > + size_t cleaned = 0; >> >> >> > >> >> >> > if (count > clean_end) { >> >> >> > next_wakeup = 0; >> >> >> > break; >> >> >> > } >> >> >> > >> >> >> > - count += ct_sweep(ct, &ct->exp_lists[i], now, &cleaned); >> >> >> > + count += ct_sweep_zone(ct, current_zone, now, &cleaned); >> >> >> > total_cleaned += cleaned; >> >> >> > + >> >> >> > + /* This will overflow and thereby allow us to iterate through >> > all >> >> >> > + * zones. */ >> >> >> > + current_zone++; >> >> >> > } >> >> >> > >> >> >> > - ct->next_sweep = (i < N_EXP_LISTS) ? i : 0; >> >> >> > + ct->next_clean_zone = current_zone + 1; >> >> >> > >> >> >> > VLOG_DBG("conntrack cleaned %"PRIuSIZE" entries out of >> >> >> > %"PRIuSIZE >> >> >> > " entries in %lld msec", total_cleaned, count, >> >> >> > @@ -2717,16 +2582,6 @@ conn_update(struct conntrack *ct, struct >> >> > conn *conn, struct dp_packet *pkt, >> >> >> > return update_res; >> >> >> > } >> >> >> > >> >> >> > -static void >> >> >> > -conn_expire_push_front(struct conntrack *ct, struct conn *conn) >> >> >> > - OVS_REQUIRES(ct->ct_lock) >> >> >> > -{ >> >> >> > - unsigned int curr = ct->next_list; >> >> >> > - >> >> >> > - ct->next_list = (ct->next_list + 1) % N_EXP_LISTS; >> >> >> > - rculist_push_front(&ct->exp_lists[curr], &conn->node); >> >> >> > -} >> >> >> > - >> >> >> > static long long int >> >> >> > conn_expiration(const struct conn *conn) >> >> >> > { >> >> >> > @@ -2914,7 +2769,8 @@ conntrack_dump_start(struct conntrack *ct, >> >> > struct conntrack_dump *dump, >> >> >> > >> >> >> > dump->ct = ct; >> >> >> > *ptot_bkts = 1; /* Need to clean up the callers. */ >> >> >> > - dump->cursor = >> > cmap_cursor_start(&dump->ct->conns[dump->current_zone]); >> >> >> > + dump->cursor = cmap_cursor_start( >> >> >> > + &dump->ct->zones[dump->current_zone].conns); >> >> >> > return 0; >> >> >> > } >> >> >> > >> >> >> > @@ -2945,7 +2801,8 @@ conntrack_dump_next(struct conntrack_dump >> >> > *dump, struct ct_dpif_entry *entry) >> >> >> > break; >> >> >> > } >> >> >> > dump->current_zone++; >> >> >> > - dump->cursor = >> >> > cmap_cursor_start(&dump->ct->conns[dump->current_zone]); >> >> >> > + dump->cursor = cmap_cursor_start( >> >> >> > + &dump->ct->zones[dump->current_zone].conns); >> >> >> > } >> >> >> > >> >> >> > return EOF; >> >> >> > @@ -3015,7 +2872,7 @@ conntrack_flush_zone(struct conntrack *ct, >> >> > const uint16_t zone) >> >> >> > struct conn_key_node *keyn; >> >> >> > struct conn *conn; >> >> >> > >> >> >> > - CMAP_FOR_EACH (keyn, cm_node, &ct->conns[zone]) { >> >> >> > + CMAP_FOR_EACH (keyn, cm_node, &ct->zones[zone].conns) { >> >> >> > if (keyn->dir != CT_DIR_FWD) { >> >> >> > continue; >> >> >> > } >> >> >> > @@ -3033,7 +2890,7 @@ conntrack_flush(struct conntrack *ct, const >> >> > uint16_t *zone) >> >> >> > return conntrack_flush_zone(ct, *zone); >> >> >> > } >> >> >> > >> >> >> > - for (unsigned i = 0; i < ARRAY_SIZE(ct->conns); i++) { >> >> >> > + for (unsigned i = 0; i < ARRAY_SIZE(ct->zones); i++) { >> >> >> > conntrack_flush_zone(ct, i); >> >> >> > } >> >> >> >> >> >> _______________________________________________ >> >> >> dev mailing list >> >> >> [email protected] >> >> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
