Am Thu, Aug 06, 2026 at 10:58:50AM -0400 schrieb Aaron Conole: > Felix Huettner <[email protected]> writes: > > > This will remove the inefficiencies we would have when just iterating > > using a position. > > > > Signed-off-by: Felix Huettner <[email protected]> > > --- > > lib/conntrack.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > [....] > > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > index eb8248ed9..12109f03b 100644 > > --- a/lib/conntrack.c > > +++ b/lib/conntrack.c > > @@ -23,6 +23,7 @@ > > #include <string.h> > > > > #include "conntrack.h" > > +#include "cmap.h" > > #include "conntrack-private.h" > > #include "conntrack-tp.h" > > #include "coverage.h" > > @@ -1505,7 +1506,6 @@ ct_sweep_zone(struct conntrack *ct, uint16_t zone, > > long long now, > > struct conntrack_zone *cz; > > unsigned int conn_count = 0; > > struct conn *conn; > > - struct cmap_node *node; > > long long expiration; > > > > cz = zone_lookup(ct, zone); > > @@ -1517,9 +1517,12 @@ ct_sweep_zone(struct conntrack *ct, uint16_t zone, > > long long now, > > *current_position = xzalloc(sizeof(**current_position)); > > } > > > > - while ((node = cmap_next_position(&cz->conns, *current_position))) { > > - keyn = OBJECT_CONTAINING(node, keyn, cm_node); > > + struct cmap_cursor cursor = cmap_position_to_cursor(&cz->conns, > > + *current_position); > > + > > + CMAP_CURSOR_FOR_EACH_CONTINUE (keyn, cm_node, &cursor) { > > if (conn_count > limit) { > > + cmap_cursor_to_position(&cursor, *current_position); > > CMAP_CURSOR_FOR_EACH_CONTINUE will advance as the first thing it does, > and cmap_cursor_to_position will also advance the element per the > documentation. So will we accidentally skip two nodes here? I'm > worried about a weird case where there are two nodes and we just keep > advancing past them. It's a weird case, agreed. > > But I think it's less theoretical given the conn_count > limit has a > case to be hit since (clean_end - count) is the argument that is passed > for the limit (so earlier zones exhaust the budet, and we just cycle > past the nodes).
ah you are right, thanks for finding that. i guess that would happen if you have `(n_max_conns/64)+1` connections, then the last one would be skipped continuously. However that is only a problem if you would always stay with exactly these connections. And in this case the clean thread would only delete that single connection anyway so i guess it does not hurt too much. However in combination with creating the current_position in the lines above that also means we will always skip the first connection in each zone from being cleaned. If you have a low global connection limit and distribute connections over nearly all zones that could mean you will never clean 64k connections (one per zone). I'll fix at least this case. > > Apart from that one thing though, I guess it is more a weirdness that we > may want to document in the code with a comment because the > double-advance isn't obvious on first glance. The practical side effect > is that we're already over the limit anyway and we will just do a double > advance here but (hopefully) the next time through we will start from > the beginning and clean the nodes anyway. Yep, i'll add the comment. Thanks a lot, Felix > > > return conn_count; > > } > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
