On Thu, Feb 08, 2024 at 03:35:14PM +0100, Alexander Bluhm wrote:
> On Wed, Feb 07, 2024 at 11:24:22AM +0100, Claudio Jeker wrote:
> > On Fri, Feb 02, 2024 at 10:11:00PM +0100, Alexander Bluhm wrote:
> > > Hi,
> > > 
> > > Claudio complained about generic struct route usage without enough
> > > storage to hold IPv6 address.  This diff splits definitions in
> > > struct route_in and route_in6 with correct types.
> > > 
> > > Is that what you want?
> > 
> > In an ideal world I would like to have one struct route object that can
> > handle both IPv4 and IPv6. This would remove the union in inpcb and makes
> > the struct route object more general.
> 
> Diff below.  Only compile tested.  But now we have to include the
> netinet and netinet6 stuff into route header.  At least I moved
> struct route into #ifdef _KERNEL.  Does any user land use struct
> route?

Nothing should use that outside of _KERNEL. Not super happy about the
reach around but no better idea.
 
> In an ideal world struct sockaddr would be large enough for IPv6.
> Is there any chance that we can ever change that?

I agree. What could go wrong, it is such a trivial change :)
 
> If you like the direction, I will run regress and build release to
> test it.
> 
> > If we have one struct route object it is possible to extend it and include
> > the source address into the cache so that rtalloc_mpath() can use that
> > instead of the uint32_t * used for the source right now.
> 
> I also came to the inclusion that src should be included in route
> cache to make mpath work properly with caching.

Exactly. Then we can pass struct route around instead of many arguments to
those rtalloc functions.
 
> > I think this would help clean up the route lookups a fair bit more.
> 
> My goal is to make the cache invalidation correct, clean the code,
> and use more caching.  I have a diff where I put a global route
> cache in percpu memory.  Also it is possible to cache the route at
> pf state.  Both increase performance as routing lookup matters.

Hmmm. A route cache in the forwarding path only makes sense if we make the
input queues sorted by flow so that same source packets are processed back
to back. Be careful with percpu caches, I think you want to cache per
network task.

Few comments below
 
> Index: net/if_veb.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_veb.c,v
> diff -u -p -r1.34 if_veb.c
> --- net/if_veb.c	23 Dec 2023 10:52:54 -0000	1.34
> +++ net/if_veb.c	8 Feb 2024 13:54:21 -0000
> @@ -38,6 +38,7 @@
>  #include <net/if.h>
>  #include <net/if_dl.h>
>  #include <net/if_types.h>
> +#include <net/if_types.h>

Double include.
  
>  #include <netinet/in.h>
>  #include <netinet/ip.h>
> Index: net/route.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.h,v
> diff -u -p -r1.205 route.h
> --- net/route.h	5 Feb 2024 12:52:11 -0000	1.205
> +++ net/route.h	8 Feb 2024 13:11:18 -0000

> @@ -393,6 +381,27 @@ struct rt_addrinfo {
>  
>  #ifdef _KERNEL
>  
> +#include <netinet/in.h>
> +
> +/*
> + * A route consists of a destination address and a reference
> + * to a routing entry.  These are often held by protocols
> + * in their control blocks, e.g. inpcb.
> + */
> +struct route {
> +	struct	rtentry *ro_rt;
> +	u_long		 ro_generation;
> +	u_long		 ro_tableid;	/* u_long because of alignment */
> +	union {
> +		struct	sockaddr	rod_sa;
> +		struct	sockaddr_in	rod_sin;
> +		struct	sockaddr_in6	rod_sin6;
> +	} ro_dst;
> +#define ro_dstsa	ro_dst.rod_sa
> +#define ro_dstsin	ro_dst.rod_sin
> +#define ro_dstsin6	ro_dst.rod_sin6
> +};
> +

You could use an anonimous union here and remove those defines.

struct route {
	union {
		struct  sockaddr	ro_dstsa;
		struct  sockaddr_in	ro_dstsin;
		struct  sockaddr_in6	ro_dstsin6;
	};
};

IMO this is nicer and results in less macro madness.

-- 
:wq Claudio