From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none 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=aUr1HD/2; 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 ESMTP id 018035A004C for ; Mon, 26 Aug 2024 21:33:39 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1724700819; 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=1sJwE+1pf5crbEyY18mFAVIcrTRhVHbnt1apTAPvHgw=; b=aUr1HD/25e6n9v6/JV6+1BUhKB7HzGAesLejeReeBjr15rSOBHgNiK/Gh6GzEhUjPih79n hSW8vLKZdwSQhpR2PMnLHvJpJes3CWGIV2AimJAVz0S44UcemnJJPm/cw1vqJ+WJsIdsxi wAl85N3wVKqbES9VEmEE8RQSvg4ynI8= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-186-bXyT1DoOPTKcHQ3k7r51kQ-1; Mon, 26 Aug 2024 15:33:37 -0400 X-MC-Unique: bXyT1DoOPTKcHQ3k7r51kQ-1 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-4281310bf7aso39416565e9.1 for ; Mon, 26 Aug 2024 12:33:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724700814; x=1725305614; 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=1sJwE+1pf5crbEyY18mFAVIcrTRhVHbnt1apTAPvHgw=; b=qNgRvCX22iRtjo9Z5K0SKz+vMNPlIhP40whql9eyKVPXjWqjgvvxwDNdDCLXPG7s5o wer8rtw1X6jBHCs+q3MSEEP+adcYFopYWhmwF+TeAb4tfdu1MRTP+klZtXbWvR2zFGEY kHwuAtU5Hgy4qve1/pOr5jXaFjEyFIpxO5JOx4kEh+CV7gYKxfl2LDZaqT8XivIAZntx AnmCbOt+PQ/P8Qh0A0nxS4IfREBmJOrFVo9V5RZQbJI8EfxfjzLWFaS1tAWKkJIk7E3c hECJIdG2O29lJaliDSWSd7sHG72ZwYAteXSdNI8/7nhrij5nL/PMPK6ZZnEMdnpYwbFV LgIA== X-Gm-Message-State: AOJu0YxeXTvNfErIjWOBGGfi2Wt7xM58uSRDwhvDfbqAlonJ6ybhF3WW gEGLn+Ux9/TXSDzdJtWu027cUorfEHMgC75nkDvgGhpVThIsL7/rEBj0MMr+QqIfOvJ4bRNjvwJ vyZFRvWdIM7tuTmXUitPZOnZUy51MRQ/BfWf9BTPGSAowNIY5aMdGUNPM4w== X-Received: by 2002:a05:600c:a4c:b0:429:a05:32fb with SMTP id 5b1f17b1804b1-42acd5573e1mr83331555e9.10.1724700813773; Mon, 26 Aug 2024 12:33:33 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEFZohnCJm7Y8v/Ps2ckHIDZ9fNx3jV3QoqrFp8r4F3ShDl0LXtwnODjbkjZmRS4mQuqBYZKw== X-Received: by 2002:a05:600c:a4c:b0:429:a05:32fb with SMTP id 5b1f17b1804b1-42acd5573e1mr83331435e9.10.1724700813237; Mon, 26 Aug 2024 12:33:33 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-42abed8b91dsm201410935e9.10.2024.08.26.12.33.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 26 Aug 2024 12:33:17 -0700 (PDT) Date: Mon, 26 Aug 2024 21:32:55 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 1/3] udp: Merge udp[46]_mh_recv arrays Message-ID: <20240826213255.769242da@elisabeth> In-Reply-To: <20240826093716.1925064-2-david@gibson.dropbear.id.au> References: <20240826093716.1925064-1-david@gibson.dropbear.id.au> <20240826093716.1925064-2-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-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: HH6VN45HIZAMUZ2DB753GV6ANSIKCTO7 X-Message-ID-Hash: HH6VN45HIZAMUZ2DB753GV6ANSIKCTO7 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 Mon, 26 Aug 2024 19:37:14 +1000 David Gibson wrote: > We've already gotten rid of most of the IPv4/IPv6 specific data structures > in udp.c by merging them with each other. One significant one remains: > udp[46]_mh_recv. This was a bit awkward to remove because of a subtle > interaction. We initialise the msg_namelen fields to represent the total > size we have for a socket address, but when we receive into the arrays > those are modified to the actual length of the sockaddr we received. > > That meant that naively merging the arrays meant that if we received IPv4 > datagrams, then IPv6 datagrams, the addresses for the latter would be > truncated. In this patch address that by resetting the received > msg_namelen as soon as we've found a flow for the datagram. Finding the > flow is the only thing that might use the actual sockaddr length, although > we in fact don't need it for the time being. > > This also removes the last use of the 'v6' field from udp_listen_epoll_ref, > so remove that as well. > > Signed-off-by: David Gibson > --- > udp.c | 57 ++++++++++++++++++++------------------------------------- > udp.h | 2 -- > 2 files changed, 20 insertions(+), 39 deletions(-) > > diff --git a/udp.c b/udp.c > index 8a93aad6..6638c22b 100644 > --- a/udp.c > +++ b/udp.c > @@ -178,8 +178,7 @@ enum udp_iov_idx { > > /* IOVs and msghdr arrays for receiving datagrams from sockets */ > static struct iovec udp_iov_recv [UDP_MAX_FRAMES]; > -static struct mmsghdr udp4_mh_recv [UDP_MAX_FRAMES]; > -static struct mmsghdr udp6_mh_recv [UDP_MAX_FRAMES]; > +static struct mmsghdr udp_mh_recv [UDP_MAX_FRAMES]; > > /* IOVs and msghdr arrays for sending "spliced" datagrams to sockets */ > static union sockaddr_inany udp_splice_to; > @@ -222,6 +221,7 @@ void udp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s) > static void udp_iov_init_one(const struct ctx *c, size_t i) > { > struct udp_payload_t *payload = &udp_payload[i]; > + struct msghdr *mh = &udp_mh_recv[i].msg_hdr; > struct udp_meta_t *meta = &udp_meta[i]; > struct iovec *siov = &udp_iov_recv[i]; > struct iovec *tiov = udp_l2_iov[i]; > @@ -236,27 +236,10 @@ static void udp_iov_init_one(const struct ctx *c, size_t i) > tiov[UDP_IOV_TAP] = tap_hdr_iov(c, &meta->taph); > tiov[UDP_IOV_PAYLOAD].iov_base = payload; > > - /* It's useful to have separate msghdr arrays for receiving. Otherwise, > - * an IPv4 recv() will alter msg_namelen, so we'd have to reset it every > - * time or risk truncating the address on future IPv6 recv()s. > - */ > - if (c->ifi4) { > - struct msghdr *mh = &udp4_mh_recv[i].msg_hdr; > - > - mh->msg_name = &meta->s_in; > - mh->msg_namelen = sizeof(struct sockaddr_in); > - mh->msg_iov = siov; > - mh->msg_iovlen = 1; > - } > - > - if (c->ifi6) { > - struct msghdr *mh = &udp6_mh_recv[i].msg_hdr; > - > - mh->msg_name = &meta->s_in; > - mh->msg_namelen = sizeof(struct sockaddr_in6); > - mh->msg_iov = siov; > - mh->msg_iovlen = 1; > - } > + mh->msg_name = &meta->s_in; > + mh->msg_namelen = sizeof(meta->s_in); > + mh->msg_iov = siov; > + mh->msg_iovlen = 1; > } > > /** > @@ -506,10 +489,10 @@ static int udp_sock_recv(const struct ctx *c, int s, uint32_t events, > void udp_listen_sock_handler(const struct ctx *c, union epoll_ref ref, > uint32_t events, const struct timespec *now) > { > - struct mmsghdr *mmh_recv = ref.udp.v6 ? udp6_mh_recv : udp4_mh_recv; > + const socklen_t sasize = sizeof(udp_meta[0].s_in); > int n, i; > > - if ((n = udp_sock_recv(c, ref.fd, events, mmh_recv)) <= 0) > + if ((n = udp_sock_recv(c, ref.fd, events, udp_mh_recv)) <= 0) > return; > > /* We divide datagrams into batches based on how we need to send them, > @@ -518,6 +501,7 @@ void udp_listen_sock_handler(const struct ctx *c, union epoll_ref ref, > * populate it one entry *ahead* of the loop counter. > */ > udp_meta[0].tosidx = udp_flow_from_sock(c, ref, &udp_meta[0].s_in, now); > + udp_mh_recv[0].msg_hdr.msg_namelen = sasize; I don't understand why you need this assignment. To me it looks redundant with: udp_mh_recv[i].msg_hdr.msg_namelen = sizeof(udp_meta[i].s_in); later (because n > 0), and: > for (i = 0; i < n; ) { > flow_sidx_t batchsidx = udp_meta[i].tosidx; > uint8_t batchpif = pif_at_sidx(batchsidx); > @@ -525,18 +509,22 @@ void udp_listen_sock_handler(const struct ctx *c, union epoll_ref ref, > > do { > if (pif_is_socket(batchpif)) { > - udp_splice_prepare(mmh_recv, i); > + udp_splice_prepare(udp_mh_recv, i); > } else if (batchpif == PIF_TAP) { > - udp_tap_prepare(mmh_recv, i, > + udp_tap_prepare(udp_mh_recv, i, > flowside_at_sidx(batchsidx)); > } > > + /* Restore sockaddr length clobbered by recvmsg() */ > + udp_mh_recv[i].msg_hdr.msg_namelen = sizeof(udp_meta[i].s_in); what is the difference between assigning sizeof(udp_meta[i].s_in); and sasize? I thought it would be the same quantity. > + > if (++i >= n) > break; > > udp_meta[i].tosidx = udp_flow_from_sock(c, ref, > &udp_meta[i].s_in, > now); > + udp_mh_recv[i].msg_hdr.msg_namelen = sasize; > } while (flow_sidx_eq(udp_meta[i].tosidx, batchsidx)); > > if (pif_is_socket(batchpif)) { -- Stefano