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.129.124]) by passt.top (Postfix) with ESMTP id CB3B15A0271 for ; Wed, 17 Jan 2024 20:59:41 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1705521580; 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=biL8gdnKsiCiNpEgpBffp+EQtaRN42xA5zV3l81XCM4=; b=GIpCbB4weo1I9dMWp4yOZaIvyNm/XXI32Dxe71QnfF8oYooDj22MykIBKHhiDIItJ+6Hrr ImiUu1bi53Dl8HB/JOknnv6UAUp69c2jbG0hkU4CgSuawa++IqxPRysS7exWv9jt8bIPzd iaoi/htnJ7J/ZwLYEj3TDpMq6ZrgwJI= 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-685-ahxVDtIUPv-hi9esbCdOQQ-1; Wed, 17 Jan 2024 14:59:37 -0500 X-MC-Unique: ahxVDtIUPv-hi9esbCdOQQ-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (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 535A285A588; Wed, 17 Jan 2024 19:59:37 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.3]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7F00D492BC6; Wed, 17 Jan 2024 19:59:36 +0000 (UTC) Date: Wed, 17 Jan 2024 20:59:34 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v3 06/15] tcp, flow: Replace TCP specific hash function with general flow hash Message-ID: <20240117205934.4b7d7043@elisabeth> In-Reply-To: <20231221070237.1422557-7-david@gibson.dropbear.id.au> References: <20231221070237.1422557-1-david@gibson.dropbear.id.au> <20231221070237.1422557-7-david@gibson.dropbear.id.au> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: PVX36HOBXLS5SEB5IR4ZL7WCXCHYCHUZ X-Message-ID-Hash: PVX36HOBXLS5SEB5IR4ZL7WCXCHYCHUZ 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, 21 Dec 2023 18:02:28 +1100 David Gibson wrote: > Currently we match TCP packets received on the tap connection to a TCP > connection via a hash table based on the forwarding address and both > ports. We hope in future to allow for multiple guest side addresses, or > for multiple interfaces which means we may need to distinguish based on > the endpoint address and pif as well. We also want a unified hash table > to cover multiple protocols, not just TCP. > > Replace the TCP specific hash function with one suitable for general flows, > or rather for one side of a general flow. This includes all the > information from struct flowside, plus the L4 protocol number. > > Signed-off-by: David Gibson > --- > flow.c | 21 +++++++++++++++++++++ > flow.h | 19 +++++++++++++++++++ > tcp.c | 59 +++++++++++----------------------------------------------- > 3 files changed, 51 insertions(+), 48 deletions(-) > > diff --git a/flow.c b/flow.c > index bc8cfc6..263633e 100644 > --- a/flow.c > +++ b/flow.c > @@ -229,6 +229,27 @@ void flow_alloc_cancel(union flow *flow) > flow_first_free = FLOW_IDX(flow); > } > > +/** > + * flow_hash() - Calculate hash value for one side of a flow > + * @c: Execution context > + * @proto: Protocol of this flow (IP L4 protocol number) > + * @fside: Flowside > + * > + * Return: hash value > + */ > +uint64_t flow_hash(const struct ctx *c, uint8_t proto, > + const struct flowside *fside) > +{ > + struct siphash_state state = SIPHASH_INIT(c->hash_secret); > + > + ASSERT(flowside_complete(fside)); > + inany_siphash_feed(&state, &fside->faddr); > + inany_siphash_feed(&state, &fside->eaddr); Customary newline here. > + return siphash_final(&state, 38, (uint64_t)proto << 40 | > + (uint64_t)fside->pif << 32 | > + fside->fport << 16 | fside->eport); If we add the fields from the 'tail' part (not the whole fside) to an anonymous struct in a similar way to what we had before fc8f0f8c48ef ("siphash: Use incremental rather than all-at-once siphash functions"), then we could drop those shift and masks, and use sizeof(that) + sizeof(fside->faddr) + sizeof(fside->eaddr) instead of '38'. > +} > + > /** > * flow_defer_handler() - Handler for per-flow deferred and timed tasks > * @c: Execution context > diff --git a/flow.h b/flow.h > index e7c4484..72ded54 100644 > --- a/flow.h > +++ b/flow.h > @@ -81,6 +81,22 @@ static inline bool flowside_complete(const struct flowside *fside) > > #define SIDES 2 > > +/** > + * flowside_eq() - Check if two flowsides are equal ...this raises the question: if the protocol number is now used in the hash, shouldn't it eventually become part of struct flowside -- and compared here, too? I guess it's useful iff we allow flowsides for the same flow to have different protocol numbers. Now, forwarding between TCP and UDP endpoints might not make a lot of sense, because we would have to make so many arbitrary assumptions as to make it probably not generic enough to be useful. But TCP to stream-oriented UNIX domain socket makes sense, and we also had user requests in that sense. Oh and... what would be the second protocol number in that case? Same here, I'm not proposing a concrete change here, I'm rather raising this if you didn't think about it (assuming it makes any sense). > + * @left, @right: Flowsides to compare > + * > + * Return: true if equal, false otherwise > + */ > +static inline bool flowside_eq(const struct flowside *left, > + const struct flowside *right) > +{ > + return left->pif == right->pif && > + inany_equals(&left->eaddr, &right->eaddr) && > + left->eport == right->eport && > + inany_equals(&left->faddr, &right->faddr) && > + left->fport == right->fport; Preferably align under "return ". -- Stefano