routing: remove ROUTE_MPATH compile option

[pouria]

Summary

The ROUTE_MPATH compile option was introduced to
test the new multipath implementation.
Since compiling it has no overhead, and it's enabled
by default.
Remove it to reduce routing complexity.

Test Plan

Run all network related tests:

kyua test -k /usr/tests/Kyuafile sys/net
kyua test -k /usr/tests/Kyuafile sys/netlink
kyua test -k /usr/tests/Kyuafile sys/netinet
kyua test -k /usr/tests/Kyuafile sys/netinet6
kyua test -k /usr/tests/Kyuafile sys/netgraph

[pouria]

I strongly believe this option is useless now and it just increase complexity of the code.
multipath is part of routing, making it optional is just like making arp/ndp optional in ipv4/ipv6.
the original reason to have this option is no longer valid.
Also, as noted in the d5fe384b4d41a5c5adeaa6039d33d59d7bc33e76, ROUTE_MPATH does not consume memory unless actually used.

[bms]

+1 vote for just doing this, all the RIBs built on top of modern FreeBSD FIB need to change with it anyway.

[glebius]

sys/net/route.h

127–152 ↗

(On Diff #173766)

Glad this is cleanup up (modulo RSS). In general putting config(8) derived options into headers installed into /usr/include is a land mine.

[pouria]

sys/net/route.h

127–152 ↗

(On Diff #173766)

This part was my original motiviation :)
See PR293136

[zlei]

The ROUTE_MPATH still has subtle performance impact and the kernel built with this option has larger text size. I'd expect some people want to keep this option so they can disable it.

Another option is VIMAGE which has little performance impact. The feature is stable enough, but still there're cases people want to disable it entirely.

[zlei]

I'm for reduce routing complexity, if it is possible to keep that option.

[glebius]

In D55884#1278650, @zlei wrote:

The ROUTE_MPATH still has subtle performance impact and the kernel built with this option has larger text size. I'd expect some people want to keep this option so they can disable it.

What is the performance impact? AFAIU, it is just array of fibs that by default has only one element.

[zlei]

In D55884#1278698, @glebius wrote:

In D55884#1278650, @zlei wrote:

The ROUTE_MPATH still has subtle performance impact and the kernel built with this option has larger text size. I'd expect some people want to keep this option so they can disable it.

What is the performance impact? AFAIU, it is just array of fibs that by default has only one element.

I re-checked the fast path, fib4_lookup() and fib6_lookup(), I think you're right. The last time I do benchmark on the route lookup is years ago, I might misremember and messed up ROUTE_MPATH with other options.

[pouria]

remove opt_route.h from files below:
sys/net/route/nhgrp.c
sys/net/route/nhgrp_ctl.c
sys/net/route/nhop.c
sys/net/route/nhop_ctl.c
sys/net/route/nhop_utils.c
sys/net/route/route_rtentry.c
sys/net/route/route_subscription.c
sys/netgraph/netflow/netflow.c
sys/netgraph/netflow/netflow_v9.c
sys/netinet/in_pcb.c
sys/netinet/ip_input.c
sys/netinet/ip_output.c
sys/netinet/raw_ip.c
sys/netinet/udp_usrreq.c
sys/netinet6/in6_proto.c
sys/netinet6/ip6_input.c
sys/netinet6/ip6_output.c
sys/netinet6/raw_ip6.c
sys/netinet6/udp6_usrreq.c
sys/netlink/route/nexthop.c

Still some files exists with opt_route.h included:
sys/modules/netlink/Makefile:SRCS+= opt_inet.h opt_inet6.h opt_route.h
sys/modules/netgraph/netflow/Makefile:SRCS= opt_route.h
sys/netlink/route/rt.c:#include "opt_route.h"
sys/net/rtsock.c:#include "opt_route.h"
sys/net/route.c:#include "opt_route.h"
sys/net/route/route_tables.c:#include "opt_route.h"
sys/net/route/route_helpers.c:#include "opt_route.h"
sys/net/route/fib_algo.c:#include "opt_route.h"
sys/net/route/route_ctl.c:#include "opt_route.h"
sys/net/route/route_ifaddrs.c:#include "opt_route.h"
sys/netgraph/netflow/ng_netflow.c:#include "opt_route.h"
sys/netinet/in_fib.c:#include "opt_route.h"
sys/netinet6/in6_fib.c:#include "opt_route.h"
sys/netinet6/in6_pcb.c:#include "opt_route.h"

Those are probably required by two options below:
sys/conf/options:ROUTETABLES opt_route.h
sys/conf/options:FIB_ALGO opt_route.h

But I'm not sure about ROUTETABLES, so I didn't remove from them.
Why?
because compiling works, even by removing opt_route.h from all other files except
for the files below:
sys/net/route/fib_algo.c:#include "opt_route.h"
sys/netinet/in_fib.c:#include "opt_route.h"
sys/netinet6/in6_fib.c:#include "opt_route.h"

Not sure why!

[glebius]

net/route/route_tables.c is the only user of ROUTETABLES

[pouria]

Remove "opt_route.h" header from all files except for:
sys/net/route/route_tables.c:#include "opt_route.h"
sys/net/route/fib_algo.c:#include "opt_route.h"
sys/netinet/in_fib.c:#include "opt_route.h"
sys/netinet6/in6_fib.c:#include "opt_route.h"

[melifaro]

sys/net/route/route_ctl.c

1450

why do we want to inflate the scope of those variables?

sys/netlink/route/rt.c

854 ↗

(On Diff #173933)

Same scope question here

[pouria]

Address @melifaro comments.
You're right, I was mistaken.
I was thinking of style(9) and assumed we still declare variables at the top of functions, but that's no longer true.
Thank you for your time and review.

[pouria]

I'd prefer to have at least one more review.
I'd appreciate it if anyone could look at this when they have time.

[markj]

In D55884#1281583, @pouria wrote:

Address @melifaro comments.
You're right, I was mistaken.
I was thinking of style(9) and assumed we still declare variables at the top of functions, but that's no longer true.
Thank you for your time and review.

We should declare variables at the beginning of a new scope, just FYI. I think the patch is ok in that respect.

Looks ok to me. I tried to check for cases where we incorrectly remove includes of opt_route.h. Maybe it would be better to move the remaining opt_route.h options to opt_global.h so we don't have to worry about these kinds of bugs. I think the whole opt_*.h mechanism is too fragile and not very useful.

[pouria]

In D55884#1282511, @markj wrote:

I tried to check for cases where we incorrectly remove includes of opt_route.h. Maybe it would be better to move the remaining opt_route.h options to opt_global.h so we don't have to worry about these kinds of bugs. I think the whole opt_*.h mechanism is too fragile and not very useful.

I really don't like the whole idea of making parts of the network optional.
IMHO, either you need INET/INET6 or not. I really don't understand the need for ROUTETABLES, SCTP_SUPPORT, and many more network compile-time options.
However, unfortunately, that's another topic.

@glebius could you please check the removal of "opt_route.h" include headers as well?
I'm really afraid of accidental removal of the "opt_route.h" header where it's required, despite the fact that the whole kernel compiles.

Also, does this change require Relnotes?

[pouria]

I'm planning to commit this tonight with a Relnotes tag around 9:30 PM +3:30 (of course, if I don't get disconnected).
I would appreciate any additional input on this review.

[pouria]

In D55884#1283308, @pouria wrote:

I'm planning to commit this tonight with a Relnotes tag around 9:30 PM +3:30 (of course, if I don't get disconnected).
I would appreciate any additional input on this review.

FYI, The reason for this timing is the end of stabilization week. (Fri 18:00 UTC)