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 8339C5A004F for ; Wed, 12 Jun 2024 08:35:00 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1718174099; 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=LBM1BtDEzBBoJB1+dmpKZIbeCilVqEUUDRE/bXSM+9c=; b=QzTQLrcAUIb2JAmRmBa7ECfwuPYLgt4WgOt36Cqt/z/8mhxX8P12gInZ1e0s6GQ6r2C1Na j5rHu2Us9Ap1ZG4PPkZEebLKXDryBH7Hy4F38xAeurkx/f/lgdUnOmN/Pw5UamFbU/NqAI bFTfCuMctM/pqffh5H3ElM1HjNLjob4= Received: from mail-vk1-f200.google.com (mail-vk1-f200.google.com [209.85.221.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-502-NNHG0KhROIu99jz0fqv1aQ-1; Wed, 12 Jun 2024 02:34:57 -0400 X-MC-Unique: NNHG0KhROIu99jz0fqv1aQ-1 Received: by mail-vk1-f200.google.com with SMTP id 71dfb90a1353d-4e4f01fcfd1so577469e0c.3 for ; Tue, 11 Jun 2024 23:34:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718174097; x=1718778897; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=LBM1BtDEzBBoJB1+dmpKZIbeCilVqEUUDRE/bXSM+9c=; b=h+GBZZK6Ggj9znmPpyh+UXlH78or9LpxBzvIxgXyOnNz8OYImiQc7j+Ai7M95+PMvH b2GfHAQf43KLeCWOwAl9gBwX3R5WgkjNICpl+p2Ks61KfZUApmGeYBC6luziRAWOYrtR sx2QjRNFor5ILKCpizmYTgEAPyuqkgCF/tGQz7jKzjMpUmWWc0SbcoXVOD/2Vpg63H8C 44UVmfgP+eEj3IlXoIYwSrfOTvl37jNHAdDrrCz5646Yya+4aT5gBY7FAYgJrckgnGyR AwjRL4dmiEyxlChlaqUFdIUeZy9sgPGNeMdmDcX6JhTxMuX8N70EiNd4A+2akMui//S/ b5yA== X-Forwarded-Encrypted: i=1; AJvYcCV4CXXYKrrfm3K6FFcROJjixFuHQcr6WQwkYPB87bRtbD2ZzL3gwit68mXJnc2p1/rFdlg+YJqmV8oucogN2T0uPstg X-Gm-Message-State: AOJu0Yx439IuVd1uaLh9a35W5ClM1pRqrPC8SDs2QSVWxeNoZ/ghZasZ cKsof+DKAezwCCmUJnIxOX8E6ggcUkAqS7fmgw5av7HILT95jyLBB8tL33pyNTVWFVG34QkGfZn 1OmLWoMH28Vzu2R9xMCjXgxSj2CARz1wKuk0WFOk6qALQye4PMQ== X-Received: by 2002:a05:6122:2015:b0:4eb:1a82:2037 with SMTP id 71dfb90a1353d-4ed07c11c38mr1078933e0c.14.1718174096945; Tue, 11 Jun 2024 23:34:56 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGygqhqjB07DlEMx3nlvLJtAazWxWNDVNDiy69AWcQeobJ+hbzhUnaMlpBzNtUsvEzNgbmRmA== X-Received: by 2002:a05:6122:2015:b0:4eb:1a82:2037 with SMTP id 71dfb90a1353d-4ed07c11c38mr1078920e0c.14.1718174096469; Tue, 11 Jun 2024 23:34:56 -0700 (PDT) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id af79cd13be357-795be0b425fsm262378585a.53.2024.06.11.23.34.55 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 11 Jun 2024 23:34:55 -0700 (PDT) Date: Wed, 12 Jun 2024 08:34:21 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v5 3/8] tap: refactor packets handling functions Message-ID: <20240612083408.43fffc4f@elisabeth> In-Reply-To: References: <20240605152129.1641658-1-lvivier@redhat.com> <20240605152129.1641658-4-lvivier@redhat.com> <20240612000950.6e8ad4b5@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: KTSJUBD6MM5QXFLQGS6FXGUJE5OQ5MXA X-Message-ID-Hash: KTSJUBD6MM5QXFLQGS6FXGUJE5OQ5MXA 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: Laurent Vivier , 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 Wed, 12 Jun 2024 16:18:59 +1000 David Gibson wrote: > On Wed, Jun 12, 2024 at 12:09:50AM +0200, Stefano Brivio wrote: > > On Wed, 5 Jun 2024 17:21:24 +0200 > > Laurent Vivier wrote: > > > > > Consolidate pool_tap4() and pool_tap6() into pool_flush_all(), > > > and tap4_handler() and tap6_handler() into tap_handler_all(). > > > Create a generic packet_add_all() to consolidate packet > > > addition logic and reduce code duplication. > > > > > > The purpose is to ease the export of these functions to use > > > them with the vhost-user backend. > > > > > > Signed-off-by: Laurent Vivier > > > --- > > > tap.c | 113 +++++++++++++++++++++++++++++++++------------------------- > > > tap.h | 7 ++++ > > > 2 files changed, 71 insertions(+), 49 deletions(-) > > > > > > diff --git a/tap.c b/tap.c > > > index 2ea08491a51f..5fb3cb83f3d2 100644 > > > --- a/tap.c > > > +++ b/tap.c > > > @@ -920,6 +920,61 @@ append: > > > return in->count; > > > } > > > > > > +/** > > > + * pool_flush() - Flush both IPv4 and IPv6 packet pools > > > + */ > > > +void pool_flush_all(void) > > > +{ > > > + pool_flush(pool_tap4); > > > + pool_flush(pool_tap6); > > > +} > > > + > > > +/** > > > + * tap_handler_all() - IPv4/IPv4 and ARP packet handler for tap file descriptor > > > > IPv4/IPv6 > > > > > + * @c: Execution context > > > + * @now: Current timestamp > > > + */ > > > +void tap_handler_all(struct ctx *c, const struct timespec *now) > > > > I wonder if this shouldn't be named tap_handler() instead. As we > > already have tap_handler_passt() and tap_handler_pasta(), it's not > > immediately clear what "all" refers to. > > I concur, I think tap_handler() is a better name. > > > > +{ > > > + tap4_handler(c, pool_tap4, now); > > > + tap6_handler(c, pool_tap6, now); > > > +} > > > + > > > +/** > > > + * packet_add_all_do() - Add a packet to the appropriate TAP pool > > > > A couple of remarks here: > > > > - it's a bit unexpected that this is still in tap.c (it adds packets to > > a pool, it should be in packet.c judging by this name/description). > > If we call it tap_queue_packet(), then it probably makes more sense? > > > > - this does more than adding a packet to a pool. It's probably useless > > to describe in detail what this does, as the function body is anyway > > rather short and clear, but the current description could be a bit > > misleading. What about "Queue/capture packet, update notion of > > guest MAC address"? > > So, given this, I think it does make more sense for this to be in > tap.c than packet.c. How about calling it tap_add_packets(). It's a single packet it adds, so perhaps tap_add_packet()? > > - what happens if you just call packet_add() from here, without dealing > > with 'func' and 'line'? I think it's fine to print in tracing output > > name and lines from this function, instead of the ones from the > > caller. It's obvious who the caller is > > It is as of this patch, but I believe the idea is this will also be > called from VU code down the line. Okay, but even then, it would be obvious from previous tracing output who the caller is. What's relevant here is to log that something went wrong while adding a packet to an IPv4 or IPv6 pool. I don't think we should bother passing around function name and line to log anything else. > > > + * @c: Execution context > > > + * @l2len: Total L2 packet length > > > + * @p: Packet buffer > > > + * @func: For tracing: name of calling function, NULL means no trace() > > > + * @line: For tracing: caller line of function call > > > + */ > > > +void packet_add_all_do(struct ctx *c, ssize_t l2len, char *p, > > > + const char *func, int line) > > > +{ > > > + const struct ethhdr *eh; > > > + > > > + pcap(p, l2len); > > > + > > > + eh = (struct ethhdr *)p; > > > + > > > + if (memcmp(c->mac_guest, eh->h_source, ETH_ALEN)) { > > > + memcpy(c->mac_guest, eh->h_source, ETH_ALEN); > > > + proto_update_l2_buf(c->mac_guest, NULL); > > > + } > > > + > > > + switch (ntohs(eh->h_proto)) { > > > + case ETH_P_ARP: > > > + case ETH_P_IP: > > > + packet_add_do(pool_tap4, l2len, p, func, line); > > > + break; > > > + case ETH_P_IPV6: > > > + packet_add_do(pool_tap6, l2len, p, func, line); > > > + break; > > > + default: > > > + break; > > > + } > > > +} > > > + > > > /** > > > * tap_sock_reset() - Handle closing or failure of connect AF_UNIX socket > > > * @c: Execution context > > > @@ -946,7 +1001,6 @@ static void tap_sock_reset(struct ctx *c) > > > void tap_handler_passt(struct ctx *c, uint32_t events, > > > const struct timespec *now) > > > { > > > - const struct ethhdr *eh; > > > ssize_t n, rem; > > > char *p; > > > > > > @@ -959,8 +1013,7 @@ redo: > > > p = pkt_buf; > > > rem = 0; > > > > > > - pool_flush(pool_tap4); > > > - pool_flush(pool_tap6); > > > + pool_flush_all(); > > > > > > n = recv(c->fd_tap, p, TAP_BUF_FILL, MSG_DONTWAIT); > > > if (n < 0) { > > > @@ -987,38 +1040,18 @@ redo: > > > /* Complete the partial read above before discarding a malformed > > > * frame, otherwise the stream will be inconsistent. > > > */ > > > - if (l2len < (ssize_t)sizeof(*eh) || > > > + if (l2len < (ssize_t)sizeof(struct ethhdr) || > > > l2len > (ssize_t)ETH_MAX_MTU) > > > goto next; > > > > > > - pcap(p, l2len); > > > - > > > - eh = (struct ethhdr *)p; > > > - > > > - if (memcmp(c->mac_guest, eh->h_source, ETH_ALEN)) { > > > - memcpy(c->mac_guest, eh->h_source, ETH_ALEN); > > > - proto_update_l2_buf(c->mac_guest, NULL); > > > - } > > > - > > > - switch (ntohs(eh->h_proto)) { > > > - case ETH_P_ARP: > > > - case ETH_P_IP: > > > - packet_add(pool_tap4, l2len, p); > > > - break; > > > - case ETH_P_IPV6: > > > - packet_add(pool_tap6, l2len, p); > > > - break; > > > - default: > > > - break; > > > - } > > > + packet_add_all(c, l2len, p); > > > > > > next: > > > p += l2len; > > > n -= l2len; > > > } > > > > > > - tap4_handler(c, pool_tap4, now); > > > - tap6_handler(c, pool_tap6, now); > > > + tap_handler_all(c, now); > > > > > > /* We can't use EPOLLET otherwise. */ > > > if (rem) > > > @@ -1043,35 +1076,18 @@ void tap_handler_pasta(struct ctx *c, uint32_t events, > > > redo: > > > n = 0; > > > > > > - pool_flush(pool_tap4); > > > - pool_flush(pool_tap6); > > > + pool_flush_all(); > > > restart: > > > while ((len = read(c->fd_tap, pkt_buf + n, TAP_BUF_BYTES - n)) > 0) { > > > - const struct ethhdr *eh = (struct ethhdr *)(pkt_buf + n); > > > > > > - if (len < (ssize_t)sizeof(*eh) || len > (ssize_t)ETH_MAX_MTU) { > > > + if (len < (ssize_t)sizeof(struct ethhdr) || > > > + len > (ssize_t)ETH_MAX_MTU) { > > > n += len; > > > continue; > > > } > > > > > > - pcap(pkt_buf + n, len); > > > > > > - if (memcmp(c->mac_guest, eh->h_source, ETH_ALEN)) { > > > - memcpy(c->mac_guest, eh->h_source, ETH_ALEN); > > > - proto_update_l2_buf(c->mac_guest, NULL); > > > - } > > > - > > > - switch (ntohs(eh->h_proto)) { > > > - case ETH_P_ARP: > > > - case ETH_P_IP: > > > - packet_add(pool_tap4, len, pkt_buf + n); > > > - break; > > > - case ETH_P_IPV6: > > > - packet_add(pool_tap6, len, pkt_buf + n); > > > - break; > > > - default: > > > - break; > > > - } > > > + packet_add_all(c, len, pkt_buf + n); > > > > > > if ((n += len) == TAP_BUF_BYTES) > > > break; > > > @@ -1082,8 +1098,7 @@ restart: > > > > > > ret = errno; > > > > > > - tap4_handler(c, pool_tap4, now); > > > - tap6_handler(c, pool_tap6, now); > > > + tap_handler_all(c, now); > > > > > > if (len > 0 || ret == EAGAIN) > > > return; > > > diff --git a/tap.h b/tap.h > > > index 2285a87093f9..3ffb7d6c3a91 100644 > > > --- a/tap.h > > > +++ b/tap.h > > > @@ -70,5 +70,12 @@ void tap_handler_passt(struct ctx *c, uint32_t events, > > > const struct timespec *now); > > > int tap_sock_unix_open(char *sock_path); > > > void tap_sock_init(struct ctx *c); > > > +void pool_flush_all(void); > > > +void tap_handler_all(struct ctx *c, const struct timespec *now); > > > + > > > +void packet_add_all_do(struct ctx *c, ssize_t l2len, char *p, > > > + const char *func, int line); > > > +#define packet_add_all(p, l2len, start) \ > > > + packet_add_all_do(p, l2len, start, __func__, __LINE__) > > > > > > #endif /* TAP_H */ > > > -- Stefano