From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=YV38j08e; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 888B45A0008 for ; Wed, 26 Mar 2025 23:14:40 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1743027279; 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=IcgqpeBr+gQU514m4pG2ErTd2W5XM+sHcHuCZG6kuC4=; b=YV38j08egq+8n3Q15B8X/cbV4acz5M5wTh5+GVfptih4a/vjsjMTY0IWDsdoX2KZlSkmmy sTPtkiRC0/Y7a/PBU024rBcWcrZWeAaEKFl3bj80V5Bkufg9cK2lpxGTIMRdPuRaEQAZSt jHPsPNrNCy7h9NbR94vPtHMl0b7ynIw= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-266-sSIGH0BHOh2mCGPtQX3jLQ-1; Wed, 26 Mar 2025 18:14:37 -0400 X-MC-Unique: sSIGH0BHOh2mCGPtQX3jLQ-1 X-Mimecast-MFC-AGG-ID: sSIGH0BHOh2mCGPtQX3jLQ_1743027277 Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-43d007b2c79so2415135e9.2 for ; Wed, 26 Mar 2025 15:14:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1743027276; x=1743632076; 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=IcgqpeBr+gQU514m4pG2ErTd2W5XM+sHcHuCZG6kuC4=; b=U/3m6E4U32UYAI68WKY22EBZWCQjtdDUoX8VBjfoZPiXdkBHMSwvjNqPuKh7rOltyo 5pmkCVMrYUUGOnqmxW9WYDSD0kfRzaXhUffppcJleMe3c+w3st2SJVSD64DOFJ6TzSUM jvDSbu5RtcLKSU5MZ0yjvLl1MBS+dUQ4PlI7kO3S8Dxky5ECZRlQhPnchuGT/t7KIZb+ M92q1Wswl9clXNU1i8dJSr1JBj8GTuq7Byx2UodXPgybz5OAl+rZOdR8bxH4xTJ5k37c kGwzRZBQFTv1AFVke1m5wKSbwuv3bItyme8P8AIAuj7OJKXqSR3hkvcUlUIi+/lHHmZY RzOA== X-Gm-Message-State: AOJu0YxcpAEi5kBHL5yvx03yqwMBQx+U9b5N1qt2oLBVdJba4qM0AouN DHoB9kmcNDn4rmbCAIHjpnnKpCRqMV/6cd32mtdPFlvv3WmrCsSsNnAaO8DY2PjKg2LOhQl2rEl M830W/1ssp+QO8N/ptIJta82KMiw701T73ACJpN4lDAtuY10DL3xxF6KZjw== X-Gm-Gg: ASbGncvwxBCZ8PT7dCef4HeB504XSllfhTayVKg6px6cr8zfL9mKccDsT62PCasTH5z E+fOejnYviZX4idJPJp2K9ljp15Ew4Lf/V1DN6BCOz7egueql3YqqVhetlTKGw7wxu2SJomzPHl olQmsu6EcfdrvkkGPp8leGd6Kr38j4PSj1Uv4c19J3BQe0814ZZozADKGcuODBBp2RrsqV0C0Vj MBu0Np7cOZE0iptJuk4FSqMDeX3w5UD+2pifpBDrt0EvmiQrGQ9RXvl8KyTrp4YCRTRVnmfh1o8 PxPYCi10Rjv5gFS+Si4DE3c78FY= X-Received: by 2002:a05:6000:188e:b0:39a:ca05:55ab with SMTP id ffacd0b85a97d-39ad1773721mr1000892f8f.44.1743027276368; Wed, 26 Mar 2025 15:14:36 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGH6Ig5iHCOR8U7mLFw85BlGVQ6uVMMtSjkv9aIaL4jNdNtUAn+Ip97O6luucqRVyCrAfNyEg== X-Received: by 2002:a05:6000:188e:b0:39a:ca05:55ab with SMTP id ffacd0b85a97d-39ad1773721mr1000874f8f.44.1743027275925; Wed, 26 Mar 2025 15:14:35 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-39acf3a9105sm2734037f8f.101.2025.03.26.15.14.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 26 Mar 2025 15:14:35 -0700 (PDT) Date: Wed, 26 Mar 2025 23:14:33 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v2 4/7] udp: Share more logic between vu and non-vu reply socket paths Message-ID: <20250326231433.06d4b263@elisabeth> In-Reply-To: <20250326034407.2240846-5-david@gibson.dropbear.id.au> References: <20250326034407.2240846-1-david@gibson.dropbear.id.au> <20250326034407.2240846-5-david@gibson.dropbear.id.au> 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-MFC-PROC-ID: xEkR5nKyM0jvCbt3E4ANtj5ulUWnNAxjI2pfQByeays_1743027277 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: BNNK33YFUVB62FFXYSFXQNGDW5IJKCFA X-Message-ID-Hash: BNNK33YFUVB62FFXYSFXQNGDW5IJKCFA 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 Wed, 26 Mar 2025 14:44:04 +1100 David Gibson wrote: > Share some additional miscellaneous logic between the vhost-user and "buf" > paths for data on udp reply sockets. The biggest piece is error handling > of cases where we can't forward between the two pifs of the flow. We also > make common some more simple logic locating the correct flow and its > parameters. > > This adds some lines of code due to extra comment lines, but nonetheless > reduces logic duplication. > > Signed-off-by: David Gibson > --- > udp.c | 41 ++++++++++++++++++++++++++--------------- > udp_vu.c | 26 +++++++++++--------------- > udp_vu.h | 3 ++- > 3 files changed, 39 insertions(+), 31 deletions(-) > > diff --git a/udp.c b/udp.c > index 55021ac4..4258812e 100644 > --- a/udp.c > +++ b/udp.c > @@ -754,24 +754,25 @@ void udp_listen_sock_handler(const struct ctx *c, > /** > * udp_buf_reply_sock_data() - Handle new data from flow specific socket > * @c: Execution context > - * @ref: epoll reference > + * @s: Socket to read data from > + * @tosidx: Flow & side to forward data from @s to > * @now: Current timestamp > * > + * Return: true on success, false if can't forward from socket to flow's pif "on success"... > + * > * #syscalls recvmmsg > */ > -static void udp_buf_reply_sock_data(const struct ctx *c, union epoll_ref ref, > +static bool udp_buf_reply_sock_data(const struct ctx *c, > + int s, flow_sidx_t tosidx, > const struct timespec *now) > { > - flow_sidx_t tosidx = flow_sidx_opposite(ref.flowside); > const struct flowside *toside = flowside_at_sidx(tosidx); > - struct udp_flow *uflow = udp_at_sidx(ref.flowside); > + struct udp_flow *uflow = udp_at_sidx(tosidx); > uint8_t topif = pif_at_sidx(tosidx); > - int n, i, from_s; > - > - from_s = uflow->s[ref.flowside.sidei]; > + int n, i; > > - if ((n = udp_sock_recv(c, from_s, udp_mh_recv)) <= 0) > - return; > + if ((n = udp_sock_recv(c, s, udp_mh_recv)) <= 0) > + return true; is not exactly accurate: 0 means either no datagrams (should never happen) or error on recvmmsg(). But I think it's clear enough, and you'll have follow-ups anyway, so I went ahead and applied this. Maybe: * Return: false if we can't forward from socket to flow's pif, true otherwise ? Actually, looking at the caller: > > flow_trace(uflow, "Received %d datagrams on reply socket", n); > uflow->ts = now->tv_sec; > @@ -790,11 +791,10 @@ static void udp_buf_reply_sock_data(const struct ctx *c, union epoll_ref ref, > } else if (topif == PIF_TAP) { > tap_send_frames(c, &udp_l2_iov[0][0], UDP_NUM_IOVS, n); > } else { > - uint8_t frompif = pif_at_sidx(ref.flowside); > - > - flow_err(uflow, "No support for forwarding UDP from %s to %s", > - pif_name(frompif), pif_name(topif)); > + return false; > } > + > + return true; > } > > /** > @@ -821,10 +821,21 @@ void udp_reply_sock_handler(const struct ctx *c, union epoll_ref ref, > } > > if (events & EPOLLIN) { > + flow_sidx_t tosidx = flow_sidx_opposite(ref.flowside); > + int s = ref.fd; > + bool ret; > + > if (c->mode == MODE_VU) > - udp_vu_reply_sock_data(c, ref, now); > + ret = udp_vu_reply_sock_data(c, s, tosidx, now); > else > - udp_buf_reply_sock_data(c, ref, now); > + ret = udp_buf_reply_sock_data(c, s, tosidx, now); > + > + if (!ret) { the opposite would probably be less surprising, if (ret) flow_err(...). > + flow_err(uflow, > + "No support for forwarding UDP from %s to %s", > + pif_name(pif_at_sidx(ref.flowside)), > + pif_name(pif_at_sidx(tosidx))); > + } > } > } > > diff --git a/udp_vu.c b/udp_vu.c > index 6e1823a9..06bdeae6 100644 > --- a/udp_vu.c > +++ b/udp_vu.c > @@ -273,38 +273,32 @@ void udp_vu_listen_sock_data(const struct ctx *c, union epoll_ref ref, > /** > * udp_vu_reply_sock_data() - Handle new data from flow specific socket > * @c: Execution context > - * @ref: epoll reference > + * @s: Socket to read data from > + * @tosidx: Flow & side to forward data from @s to > * @now: Current timestamp > + * > + * Return: true on success, false if can't forward from socket to flow's pif > */ > -void udp_vu_reply_sock_data(const struct ctx *c, union epoll_ref ref, > +bool udp_vu_reply_sock_data(const struct ctx *c, int s, flow_sidx_t tosidx, > const struct timespec *now) > { > - flow_sidx_t tosidx = flow_sidx_opposite(ref.flowside); > const struct flowside *toside = flowside_at_sidx(tosidx); > bool v6 = !(inany_v4(&toside->eaddr) && inany_v4(&toside->oaddr)); > - struct udp_flow *uflow = udp_at_sidx(ref.flowside); > - int from_s = uflow->s[ref.flowside.sidei]; > + struct udp_flow *uflow = udp_at_sidx(tosidx); > struct vu_dev *vdev = c->vdev; > struct vu_virtq *vq = &vdev->vq[VHOST_USER_RX_QUEUE]; > - uint8_t topif = pif_at_sidx(tosidx); > int i; > > ASSERT(uflow); > > - if (topif != PIF_TAP) { > - uint8_t frompif = pif_at_sidx(ref.flowside); > - > - flow_err(uflow, > - "No support for forwarding UDP from %s to %s", > - pif_name(frompif), pif_name(topif)); > - return; > - } > + if (pif_at_sidx(tosidx) != PIF_TAP) > + return false; > > for (i = 0; i < UDP_MAX_FRAMES; i++) { > ssize_t dlen; > int iov_used; > > - iov_used = udp_vu_sock_recv(c, from_s, v6, &dlen); > + iov_used = udp_vu_sock_recv(c, s, v6, &dlen); > if (iov_used <= 0) > break; > flow_trace(uflow, "Received 1 datagram on reply socket"); > @@ -318,4 +312,6 @@ void udp_vu_reply_sock_data(const struct ctx *c, union epoll_ref ref, > } > vu_flush(vdev, vq, elem, iov_used); > } > + > + return true; > } > diff --git a/udp_vu.h b/udp_vu.h > index 4f2262d0..2299b51f 100644 > --- a/udp_vu.h > +++ b/udp_vu.h > @@ -8,6 +8,7 @@ > > void udp_vu_listen_sock_data(const struct ctx *c, union epoll_ref ref, > const struct timespec *now); > -void udp_vu_reply_sock_data(const struct ctx *c, union epoll_ref ref, > +bool udp_vu_reply_sock_data(const struct ctx *c, int s, flow_sidx_t tosidx, > const struct timespec *now); > + > #endif /* UDP_VU_H */ -- Stefano