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 B80455A0272 for ; Wed, 17 Jan 2024 20:59:28 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1705521567; 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=PS72VY2SkFl7xT2ER+am9PxH5q7p2/gbDM2X9ces8vo=; b=Pi3FI2S2PpHfZOyjoDoRP3/wU1l08zPTjEMmp1KkKI1KBr7Cy5+7hvMTFa5GcolgqvzOsA lCOuK9x/ml/VmsKZld8HBPE3BotPjT3Ga94mFmctYrYhpFWZDvHk/mPzeV0ALMYQvXGYb0 UHZv79sL7NVcOJx5S5oXRB3arlT/O8k= 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-581-OCPv60NMNaiUKyb1mNYN7g-1; Wed, 17 Jan 2024 14:59:26 -0500 X-MC-Unique: OCPv60NMNaiUKyb1mNYN7g-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (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 ACBDE185A782; Wed, 17 Jan 2024 19:59:25 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.3]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D5023492BE6; Wed, 17 Jan 2024 19:59:24 +0000 (UTC) Date: Wed, 17 Jan 2024 20:59:22 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v3 05/15] flow, tcp, tcp_splice: Uniform debug helpers for new flows Message-ID: <20240117205922.7bba3b54@elisabeth> In-Reply-To: <20231221070237.1422557-6-david@gibson.dropbear.id.au> References: <20231221070237.1422557-1-david@gibson.dropbear.id.au> <20231221070237.1422557-6-david@gibson.dropbear.id.au> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: 3BMQG4WZ5P2ZVGYMOV7VOXS6YVJLPUOK X-Message-ID-Hash: 3BMQG4WZ5P2ZVGYMOV7VOXS6YVJLPUOK 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:27 +1100 David Gibson wrote: > When debugging passt/pasta, and the flow table in particular, one of the > most obvious things to know is when a new flow is initiated, along with the > details of its interface, addresses and ports. Once we've determined to > what interface the flow should be forwarded, it's useful to know the > details of how it will appear on that other interface. > > To help present that information uniformly, introduce FLOW_NEW_DBG() and > FLOW_FWD_DBG() helpers and use them for TCP connections, both "tap" and > spliced. > > Signed-off-by: David Gibson > --- > flow.c | 40 ++++++++++++++++++++++++++++++++++++++++ > flow.h | 16 ++++++++++++++++ > tcp.c | 11 +++++++++-- > tcp_splice.c | 3 ++- > 4 files changed, 67 insertions(+), 3 deletions(-) > > diff --git a/flow.c b/flow.c > index b9c4a18..bc8cfc6 100644 > --- a/flow.c > +++ b/flow.c > @@ -9,6 +9,7 @@ > #include > #include > #include > +#include > > #include "util.h" > #include "passt.h" > @@ -50,6 +51,45 @@ void flow_log_(const struct flow_common *f, int pri, const char *fmt, ...) > logmsg(pri, "Flow %u (%s): %s", flow_idx(f), FLOW_TYPE(f), msg); > } > > +/** > + * flow_new_dbg() - Print debug message for new flow > + * @f: Common flow structure > + * @side: Which side initiated the new flow > + */ > +void flow_new_dbg(const struct flow_common *f, unsigned side) > +{ > + char ebuf[INET6_ADDRSTRLEN], fbuf[INET6_ADDRSTRLEN]; > + const struct flowside *fside = &f->side[side]; > + > + flow_log_(f, LOG_DEBUG, "New %s from %s/%u: [%s]:%hu <-> [%s]:%hu", I think we should always print the connection index too (passed from the macro, or passing 'conn' as argument here) -- especially if we want to correlate this to what flow_fwd_dbg() will print later. It's also useful if anything goes wrong with the flow table itself. Sure, address/ports pairs should now be sufficient to uniquely identify a flow, and the flow table should be otherwise transparent, but the idea behind debugging is that there's a bug somewhere. > + flow_type_str[f->type], pif_name(fside->pif), side, > + inet_ntop(AF_INET6, &fside->faddr, fbuf, sizeof(fbuf)), > + fside->fport, > + inet_ntop(AF_INET6, &fside->eaddr, ebuf, sizeof(ebuf)), > + fside->eport); > +} > + > +/** > + * flow_fwd_dbg() - Print debug message for newly forwarded flow > + * @f: Common flow structure > + * @side: Which side was the flow forwarded to > + */ > +void flow_fwd_dbg(const struct flow_common *f, unsigned side) > +{ > + char ebuf[INET6_ADDRSTRLEN], fbuf[INET6_ADDRSTRLEN]; > + const struct flowside *fside = &f->side[side]; > + > + inet_ntop(AF_INET6, &fside->eaddr, ebuf, sizeof(ebuf)); > + inet_ntop(AF_INET6, &fside->faddr, fbuf, sizeof(fbuf)); > + > + flow_log_(f, LOG_DEBUG, "Forwarded to %s/%u: [%s]:%hu <-> [%s]:%hu", > + pif_name(fside->pif), side, > + inet_ntop(AF_INET6, &fside->faddr, fbuf, sizeof(fbuf)), > + fside->fport, > + inet_ntop(AF_INET6, &fside->eaddr, ebuf, sizeof(ebuf)), > + fside->eport); > +} > + > /** flowside_from_sock - Initialize flowside to match an existing socket > * @fside: flowside to initialize > * @pif: pif id of this flowside > diff --git a/flow.h b/flow.h > index 37885b2..e7c4484 100644 > --- a/flow.h > +++ b/flow.h > @@ -97,6 +97,22 @@ struct flow_common { > #define FLOW_TABLE_PRESSURE 30 /* % of FLOW_MAX */ > #define FLOW_FILE_PRESSURE 30 /* % of c->nofile */ > > +/** flow_complete - Check if common parts of flow are fully initialized > + * @flow: flow to check > + */ > +static inline bool flow_complete(const struct flow_common *f) > +{ > + return f->type != FLOW_TYPE_NONE && f->type < FLOW_NUM_TYPES && > + flowside_complete(&f->side[0]) && > + flowside_complete(&f->side[1]); Preferably align next lines after "return ". -- Stefano