Re: [PATCH 2/4] udp: Fold checking of splice flag into udp_mmh_splice_port()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4260 bytes --]

On Thu, Jun 13, 2024 at 05:06:54PM +0200, Stefano Brivio wrote:
> On Wed,  5 Jun 2024 11:39:01 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > udp_mmh_splice_port() is used to determine if a UDP datagram can be
> > "spliced" (forwarded via a socket instead of tap).  We only invoke it if
> > the origin socket has the 'splice' flag set.
> > 
> > Fold the checking of the flag into the helper itself, which makes the
> > caller simpler.  It does mean we have a loop looking for a batch of
> > spliceable or non-spliceable packets even in the case where the flag is
> > clear.  This shouldn't be that expensive though, since each call to
> > udp_mmh_splice_port() will return without accessing memory in that case.
> > In any case we're going to need a similar loop in more cases with upcoming
> > flow table work.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  udp.c | 31 ++++++++++++++++---------------
> >  1 file changed, 16 insertions(+), 15 deletions(-)
> > 
> > diff --git a/udp.c b/udp.c
> > index 3abafc99..7487d2b2 100644
> > --- a/udp.c
> > +++ b/udp.c
> > @@ -467,21 +467,25 @@ static int udp_splice_new_ns(void *arg)
> >  
> >  /**
> >   * udp_mmh_splice_port() - Is source address of message suitable for splicing?
> > - * @v6:		Is @sa a sockaddr_in6 (otherwise sockaddr_in)?
> > + * @uref:	UDP epoll reference for incoming message's origin socket
> >   * @mmh:	mmsghdr of incoming message
> >   *
> > - * Return: if @sa refers to localhost (127.0.0.1 or ::1) the port from
> > - *         @sa in host order, otherwise -1.
> > + * Return: if source address of message in @mmh refers to localhost (127.0.0.1
> 
> Pre-existing, and I guess this might change again with the complete
> flow table implementation, so it probably doesn't make sense to fix
> this now: it's 127.0.0.0/8, not necessarily 127.0.0.1.

Right.  In fact this function will go away entirely with the flow
table.

> As to whether we actually need to preserve a source address that's not
> 127.0.0.1, but in 127.0.0.0/8, as we "splice", I'm not quite sure. I
> think we could bind() the socket in the target namespace, but I haven't
> tried, and I don't know if it makes sense at all (I can't think of any
> use case).

So, how to handle 127.0.0.0/8 is something I'm actively thinking
about.  It should be much easier to tweak this with the flow table in
place.

> > + *         or ::1) its source port (host order), otherwise -1.
> >   */
> > -static int udp_mmh_splice_port(bool v6, const struct mmsghdr *mmh)
> > +static int udp_mmh_splice_port(union udp_epoll_ref uref,
> > +			       const struct mmsghdr *mmh)
> >  {
> >  	const struct sockaddr_in6 *sa6 = mmh->msg_hdr.msg_name;
> >  	const struct sockaddr_in *sa4 = mmh->msg_hdr.msg_name;
> >  
> > -	if (v6 && IN6_IS_ADDR_LOOPBACK(&sa6->sin6_addr))
> > +	if (!uref.splice)
> > +		return -1;
> > +
> > +	if (uref.v6 && IN6_IS_ADDR_LOOPBACK(&sa6->sin6_addr))
> >  		return ntohs(sa6->sin6_port);
> >  
> > -	if (!v6 && IN4_IS_ADDR_LOOPBACK(&sa4->sin_addr))
> > +	if (!uref.v6 && IN4_IS_ADDR_LOOPBACK(&sa4->sin_addr))
> >  		return ntohs(sa4->sin_port);
> >  
> >  	return -1;
> > @@ -768,18 +772,15 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref, uint32_t events,
> 
> (now renamed to udp_buf_sock_handler() if you're wondering)
> 
> >  
> >  	for (i = 0; i < n; i += m) {
> >  		int splicefrom = -1;
> > -		m = n;
> >  
> > -		if (ref.udp.splice) {
> > -			splicefrom = udp_mmh_splice_port(v6, mmh_recv + i);
> > +		splicefrom = udp_mmh_splice_port(ref.udp, mmh_recv + i);
> >  
> > -			for (m = 1; i + m < n; m++) {
> > -				int p;
> > +		for (m = 1; i + m < n; m++) {
> > +			int p;
> >  
> > -				p = udp_mmh_splice_port(v6, mmh_recv + i + m);
> > -				if (p != splicefrom)
> > -					break;
> > -			}
> > +			p = udp_mmh_splice_port(ref.udp, mmh_recv + i + m);
> > +			if (p != splicefrom)
> > +				break;
> >  		}
> >  
> >  		if (splicefrom >= 0)
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]