Re: Kill rtable_mpath_match

[Martin Pieuchot]

On 25/10/15(Sun) 16:21, Sebastian Benoit wrote:
> Martin Pieuchot(m...@openbsd.org) on 2015.10.25 16:14:27 +0100:
> > Diff below merges the guts of rtable_mpath_match() into rtable_lookup().
> > As for the previous rtable_mpath_* diff this is a step towards MPATH by
> > default.
> > 
> > This diff introduces a behavior change for RTM_GET.  If multiple route
> > exists a gateway MUST be specified and the first one in the tree won't
> > be automagically selected by the kernel.

After discussion here's a diff that do almost the same refactoring but
without introducing a behavior change.  ok?

Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.398
diff -u -p -r1.398 if.c
--- net/if.c25 Oct 2015 21:58:04 -  1.398
+++ net/if.c26 Oct 2015 19:13:28 -
@@ -2360,7 +2360,7 @@ if_group_egress_build(void)
bzero(_in, sizeof(sa_in));
sa_in.sin_len = sizeof(sa_in);
sa_in.sin_family = AF_INET;
-   rt0 = rtable_lookup(0, sintosa(_in), sintosa(_in));
+   rt0 = rtable_lookup(0, sintosa(_in), sintosa(_in), NULL, RTP_ANY);
if (rt0 != NULL) {
rt = rt0;
do {
@@ -2377,7 +2377,8 @@ if_group_egress_build(void)
 
 #ifdef INET6
bcopy(_any, _in6, sizeof(sa_in6));
-   rt0 = rtable_lookup(0, sin6tosa(_in6), sin6tosa(_in6));
+   rt0 = rtable_lookup(0, sin6tosa(_in6), sin6tosa(_in6), NULL,
+   RTP_ANY);
if (rt0 != NULL) {
rt = rt0;
do {
Index: net/route.c
===
RCS file: /cvs/src/sys/net/route.c,v
retrieving revision 1.265
diff -u -p -r1.265 route.c
--- net/route.c 25 Oct 2015 16:25:23 -  1.265
+++ net/route.c 26 Oct 2015 19:14:45 -
@@ -733,15 +733,11 @@ rtrequest1(int req, struct rt_addrinfo *
switch (req) {
case RTM_DELETE:
rt = rtable_lookup(tableid, info->rti_info[RTAX_DST],
-   info->rti_info[RTAX_NETMASK]);
+   info->rti_info[RTAX_NETMASK], info->rti_info[RTAX_GATEWAY],
+   prio);
if (rt == NULL)
return (ESRCH);
 #ifndef SMALL_KERNEL
-   rt = rtable_mpath_match(tableid, rt,
-   info->rti_info[RTAX_GATEWAY], prio);
-   if (rt == NULL)
-   return (ESRCH);
-
/*
 * If we got multipath routes, we require users to specify
 * a matching gateway.
Index: net/rtable.c
===
RCS file: /cvs/src/sys/net/rtable.c,v
retrieving revision 1.15
diff -u -p -r1.15 rtable.c
--- net/rtable.c25 Oct 2015 14:48:51 -  1.15
+++ net/rtable.c26 Oct 2015 19:11:50 -
@@ -249,7 +249,7 @@ rtable_free(unsigned int rtableid)
 
 struct rtentry *
 rtable_lookup(unsigned int rtableid, struct sockaddr *dst,
-struct sockaddr *mask)
+struct sockaddr *mask, struct sockaddr *gateway, uint8_t prio)
 {
struct radix_node_head  *rnh;
struct radix_node   *rn;
@@ -264,6 +264,16 @@ rtable_lookup(unsigned int rtableid, str
return (NULL);
 
rt = ((struct rtentry *)rn);
+
+#ifndef SMALL_KERNEL
+   if (rnh->rnh_multipath) {
+   rt = rt_mpath_matchgate(rt, gateway, prio);
+   if (rt == NULL)
+   return (NULL);
+   }
+
+#endif /* !SMALL_KERNEL */
+
rtref(rt);
 
return (rt);
@@ -372,26 +382,6 @@ rtable_mpath_capable(unsigned int rtable
return (rnh->rnh_multipath);
 }
 
-struct rtentry *
-rtable_mpath_match(unsigned int rtableid, struct rtentry *rt0,
-struct sockaddr *gateway, uint8_t prio)
-{
-   struct radix_node_head  *rnh;
-   struct rtentry  *rt;
-
-   rnh = rtable_get(rtableid, rt_key(rt0)->sa_family);
-   if (rnh == NULL || rnh->rnh_multipath == 0)
-   return (rt0);
-
-   rt = rt_mpath_matchgate(rt0, gateway, prio);
-
-   if (rt != NULL)
-   rtref(rt);
-   rtfree(rt0);
-
-   return (rt);
-}
-
 /* Gateway selection by Hash-Threshold (RFC 2992) */
 struct rtentry *
 rtable_mpath_select(struct rtentry *rt, uint32_t hash)
@@ -457,7 +447,7 @@ rtable_free(unsigned int rtableid)
 
 struct rtentry *
 rtable_lookup(unsigned int rtableid, struct sockaddr *dst,
-struct sockaddr *mask)
+struct sockaddr *mask, struct sockaddr *gateway, uint8_t prio)
 {
struct art_root *ar;
struct art_node *an;
@@ -489,7 +479,25 @@ rtable_lookup(unsigned int rtableid, str
if (an == NULL)
return (NULL);
 
+#ifdef SMALL_KERNEL
rt = LIST_FIRST(>an_rtlist);
+#else
+   LIST_FOREACH(rt, >an_rtlist, rt_next) {
+   if (prio != RTP_ANY &&
+   (rt->rt_priority & RTP_MASK) != (prio & 

Re: Kill rtable_mpath_match

[Alexander Bluhm]

On Tue, Oct 27, 2015 at 01:40:04PM +0100, Martin Pieuchot wrote:
> On 25/10/15(Sun) 16:21, Sebastian Benoit wrote:
> > Martin Pieuchot(m...@openbsd.org) on 2015.10.25 16:14:27 +0100:
> > > Diff below merges the guts of rtable_mpath_match() into rtable_lookup().
> > > As for the previous rtable_mpath_* diff this is a step towards MPATH by
> > > default.
> > > 
> > > This diff introduces a behavior change for RTM_GET.  If multiple route
> > > exists a gateway MUST be specified and the first one in the tree won't
> > > be automagically selected by the kernel.
> 
> After discussion here's a diff that do almost the same refactoring but
> without introducing a behavior change.  ok?

Two nits.

> @@ -264,6 +264,16 @@ rtable_lookup(unsigned int rtableid, str
>   return (NULL);
>  
>   rt = ((struct rtentry *)rn);
> +
> +#ifndef SMALL_KERNEL
> + if (rnh->rnh_multipath) {
> + rt = rt_mpath_matchgate(rt, gateway, prio);
> + if (rt == NULL)
> + return (NULL);
> + }
> +

Please do not add too many new lines.

> +#endif /* !SMALL_KERNEL */
> +
>   rtref(rt);
>  
>   return (rt);


> @@ -620,51 +620,36 @@ route_output(struct mbuf *m, ...)
>   error = EAFNOSUPPORT;
>   goto flush;
>   }
> +
>   rt = rtable_lookup(tableid, info.rti_info[RTAX_DST],
> - info.rti_info[RTAX_NETMASK]);
> - if (rt == NULL) {
> - error = ESRCH;
> - goto flush;
> + info.rti_info[RTAX_NETMASK], info.rti_info[RTAX_GATEWAY],
> + prio);
> +#ifdef SMALL_KERNEL

I think this should be #ifndef

> + /*
> +  * If we got multipath routes, we require users to specify
> +  * a matching gateway, except for RTM_GET.
> +  */
> + if ((rt != NULL) && ISSET(rt->rt_flags, RTF_MPATH) &&
> + (info.rti_info[RTAX_GATEWAY] == NULL) &&
> + (rtm->rtm_type != RTM_GET)) {
> + rtfree(rt);
> + rt = NULL;
>   }

Otherwise OK bluhm@

Re: Kill rtable_mpath_match

[Sebastian Benoit]

Martin Pieuchot(m...@openbsd.org) on 2015.10.25 16:14:27 +0100:
> Diff below merges the guts of rtable_mpath_match() into rtable_lookup().
> As for the previous rtable_mpath_* diff this is a step towards MPATH by
> default.
> 
> This diff introduces a behavior change for RTM_GET.  If multiple route
> exists a gateway MUST be specified and the first one in the tree won't
> be automagically selected by the kernel.
> 
> Example:
> 
>   # route -n show -inet |grep 192.168.178/24
>   192.168.178/24 192.168.178.1  UGSP   1  230 - 8 
> vio0 
>   192.168.178/24 192.168.178.2  UGSP   00 - 8 
> vio0 

greping through 500k routes isnt really nice.

>   # route -n get 192.168.178/24
>   route: writing to routing socket: No such process

the error msg is misleading, something like "multiple routes available" or
so would be nice. Otherwise i am lead to think there is no route.

>   # route -n get 192.168.178/24 192.168.178.1
>  route to: 192.168.178.0
>   destination: 192.168.178.0
>  mask: 255.255.255.0
>   gateway: 192.168.178.1
> interface: vio0
>if address: 192.168.178.5
>  priority: 8 (static)
> flags: 
>use   mtuexpire
>326 0 0 
> 
> 
> Is it acceptable?
> 
> Index: net/if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.397
> diff -u -p -r1.397 if.c
> --- net/if.c  25 Oct 2015 13:52:45 -  1.397
> +++ net/if.c  25 Oct 2015 15:06:44 -
> @@ -2360,7 +2360,7 @@ if_group_egress_build(void)
>   bzero(_in, sizeof(sa_in));
>   sa_in.sin_len = sizeof(sa_in);
>   sa_in.sin_family = AF_INET;
> - rt0 = rtable_lookup(0, sintosa(_in), sintosa(_in));
> + rt0 = rtable_lookup(0, sintosa(_in), sintosa(_in), NULL, RTP_ANY);
>   if (rt0 != NULL) {
>   rt = rt0;
>   do {
> @@ -2377,7 +2377,8 @@ if_group_egress_build(void)
>  
>  #ifdef INET6
>   bcopy(_any, _in6, sizeof(sa_in6));
> - rt0 = rtable_lookup(0, sin6tosa(_in6), sin6tosa(_in6));
> + rt0 = rtable_lookup(0, sin6tosa(_in6), sin6tosa(_in6), NULL,
> + RTP_ANY);
>   if (rt0 != NULL) {
>   rt = rt0;
>   do {
> Index: net/route.c
> ===
> RCS file: /cvs/src/sys/net/route.c,v
> retrieving revision 1.263
> diff -u -p -r1.263 route.c
> --- net/route.c   25 Oct 2015 14:48:51 -  1.263
> +++ net/route.c   25 Oct 2015 15:06:45 -
> @@ -733,25 +733,10 @@ rtrequest1(int req, struct rt_addrinfo *
>   switch (req) {
>   case RTM_DELETE:
>   rt = rtable_lookup(tableid, info->rti_info[RTAX_DST],
> - info->rti_info[RTAX_NETMASK]);
> + info->rti_info[RTAX_NETMASK], info->rti_info[RTAX_GATEWAY],
> + prio );
>   if (rt == NULL)
>   return (ESRCH);
> -#ifndef SMALL_KERNEL
> - rt = rtable_mpath_match(tableid, rt,
> - info->rti_info[RTAX_GATEWAY], prio);
> - if (rt == NULL)
> - return (ESRCH);
> -
> - /*
> -  * If we got multipath routes, we require users to specify
> -  * a matching gateway.
> -  */
> - if ((rt->rt_flags & RTF_MPATH) &&
> - info->rti_info[RTAX_GATEWAY] == NULL) {
> - rtfree(rt);
> - return (ESRCH);
> - }
> -#endif
>  
>   /*
>* Since RTP_LOCAL cannot be set by userland, make
> Index: net/rtable.c
> ===
> RCS file: /cvs/src/sys/net/rtable.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 rtable.c
> --- net/rtable.c  25 Oct 2015 14:48:51 -  1.15
> +++ net/rtable.c  25 Oct 2015 15:06:45 -
> @@ -249,7 +249,7 @@ rtable_free(unsigned int rtableid)
>  
>  struct rtentry *
>  rtable_lookup(unsigned int rtableid, struct sockaddr *dst,
> -struct sockaddr *mask)
> +struct sockaddr *mask, struct sockaddr *gateway, uint8_t prio)
>  {
>   struct radix_node_head  *rnh;
>   struct radix_node   *rn;
> @@ -264,6 +264,22 @@ rtable_lookup(unsigned int rtableid, str
>   return (NULL);
>  
>   rt = ((struct rtentry *)rn);
> +
> +#ifndef SMALL_KERNEL
> + if (rnh->rnh_multipath) {
> + rt = rt_mpath_matchgate(rt, gateway, prio);
> + if (rt == NULL)
> + return (NULL);
> + }
> +
> + /*
> +  * If we got multipath routes, we require users to specify
> +  * a matching gateway.
> +  */
> + if (ISSET(rt->rt_flags, RTF_MPATH) && gateway == NULL)
> + return (NULL);
> +#endif /* !SMALL_KERNEL */
> +
>   rtref(rt);
>  
>   return (rt);
> @@ -372,26 +388,6 @@