From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 77AEE5A027F for ; Thu, 30 Nov 2023 13:45:37 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1701348336; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Ft3JLrkGnJEmR006DRp+BFAnhxFnqTNiedutEChZ4JY=; b=S37gVSBYSvR45jSkQF8Gxf7h8LRKej7SdbRH8DDMYES1L/HgxKO5zRW354VJ8zlU1l3zxY uaWNsn12bfZEEHaGa+VT88ieS8S0KL0HmJtcjjWR1bVPyRcXpInUjzyvQXpT2d+V6iKN4x ELz7TRBS12YeOz/cwjPNbxfNnExozX4= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-690-t0Nt12T-OTSxnkcDbXXQGg-1; Thu, 30 Nov 2023 07:45:35 -0500 X-MC-Unique: t0Nt12T-OTSxnkcDbXXQGg-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 1422085A58A; Thu, 30 Nov 2023 12:45:35 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.34]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 431712026D4C; Thu, 30 Nov 2023 12:45:34 +0000 (UTC) Date: Thu, 30 Nov 2023 13:45:32 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v3 16/16] tcp: Don't defer hash table removal Message-ID: <20231130134532.7c5b959e@elisabeth> In-Reply-To: <20231130020222.4056647-17-david@gibson.dropbear.id.au> References: <20231130020222.4056647-1-david@gibson.dropbear.id.au> <20231130020222.4056647-17-david@gibson.dropbear.id.au> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: GCBOG2BRSEW4OIS5HLSMLN46EODBKSS3 X-Message-ID-Hash: GCBOG2BRSEW4OIS5HLSMLN46EODBKSS3 X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu, 30 Nov 2023 13:02:22 +1100 David Gibson wrote: > When a TCP connection is closed, we mark it by setting events to CLOSED, > then some time later we do final cleanups: closing sockets, removing from > the hash table and so forth. > > This does mean that when making a hash lookup we need to exclude any > apparent matches that are CLOSED, since they represent a stale connection. > This can happen in practice if one connection closes and a new one with the > same endpoints is started shortly afterward. > > Checking for CLOSED is quite specific to TCP however, and won't work when > we extend the hash table to more general flows. So, alter the code to > immediately remove the connection from the hash table when CLOSED, although > we still defer closing sockets and other cleanup. > > Signed-off-by: David Gibson > --- > tcp.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/tcp.c b/tcp.c > index 74d06bf..17c7cba 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -781,6 +781,9 @@ static void conn_flag_do(const struct ctx *c, struct tcp_tap_conn *conn, > tcp_timer_ctl(c, conn); > } > > +static void tcp_hash_remove(const struct ctx *c, > + const struct tcp_tap_conn *conn); > + > /** > * conn_event_do() - Set and log connection events, update epoll state > * @c: Execution context > @@ -825,7 +828,9 @@ static void conn_event_do(const struct ctx *c, struct tcp_tap_conn *conn, > flow_dbg(conn, "%s", > num == -1 ? "CLOSED" : tcp_event_str[num]); > > - if ((event == TAP_FIN_RCVD) && !(conn->events & SOCK_FIN_RCVD)) > + if (event == CLOSED) > + tcp_hash_remove(c, conn); > + else if ((event == TAP_FIN_RCVD) && !(conn->events & SOCK_FIN_RCVD)) > conn_flag(c, conn, ACTIVE_CLOSE); > else > tcp_epoll_ctl(c, conn); > @@ -1150,7 +1155,7 @@ static int tcp_hash_match(const struct tcp_tap_conn *conn, > const union inany_addr *faddr, > in_port_t eport, in_port_t fport) > { > - if (conn->events != CLOSED && inany_equals(&conn->faddr, faddr) && > + if (inany_equals(&conn->faddr, faddr) && > conn->eport == eport && conn->fport == fport) > return 1; > > @@ -1308,7 +1313,6 @@ static void tcp_conn_destroy(struct ctx *c, union flow *flow) > if (conn->timer != -1) > close(conn->timer); > > - tcp_hash_remove(c, conn); > flow_table_compact(c, flow); I was pretty sure, due to the way I originally implemented this, that removing an entry from the hash table without compacting the table afterwards, with an event possibly coming between the two, would present some inconsistency while we're handling that event. But looking at it now, I don't see any issue with this. I just wanted to raise it in case you're aware of (but didn't think about) some concern in this sense. By the way, the reason why I deferred tcp_hash_remove() back then was to save cycles between epoll events and get higher CRR rates, but I think the effect is negligible anyway. -- Stefano