routing: Allow nhop reuse in nhgrp with different metric

[pouria]

Summary

nhop_get_nhop_internal() looks up nhops based on priv hash.
only return nhops with different metrics if the input nexthop's
metric is zero.

Depends on: D56323

Test Plan

Add two nexthops with different metric and delete one of them.

 # route -n6 add 3fff:a::/64 -gateway fe80::5a9c:fcff:fe10:2%vtnet1 -metric 2
add net 3fff:a::/64: gateway fe80::5a9c:fcff:fe10:2%vtnet1 fib 0
 # route -n6 add 3fff:a::/64 -gateway fe80::5a9c:fcff:fe10:2%vtnet1
add net 3fff:a::/64: gateway fe80::5a9c:fcff:fe10:2%vtnet1 fib 0
 # route -on6 get 3fff:a::/64
route to: 3fff:a::
destination: 3fff:a::
mask: ffff:ffff:ffff:ffff::
fib: 0
flags: <UP,GATEWAY,DONE,STATIC>
nhops: 2
via gw fe80::5a9c:fcff:fe10:2%vtnet1 iface vtnet1 metric 2 weight 1 mtu 1500 table inet6.0
via gw fe80::5a9c:fcff:fe10:2%vtnet1 iface vtnet1 metric 1 weight 1 mtu 1500
 # route -n6 del 3fff:a::/64 -gateway fe80::5a9c:fcff:fe10:2%vtnet1 -metric 2
del net 3fff:a::/64: gateway fe80::5a9c:fcff:fe10:2%vtnet1 fib 0
 # route -on6 get 3fff:a::/64
route to: 3fff:a::
destination: 3fff:a::
mask: ffff:ffff:ffff:ffff::
fib: 0
flags: <UP,GATEWAY,DONE,STATIC>
nhops: 0
via gw fe80::5a9c:fcff:fe10:2%vtnet1 iface vtnet1 metric 1 weight 0 mtu 1500 table inet6.0

[markj]

sys/net/route/nhop.c

402 ↗

(On Diff #177854)

So this looks up a nexthop based on a hash of the nh_priv... how do you know there aren't multiple values with the same hash but different metrics? In that case, we're only looking at the first match.

[pouria]

sys/net/route/nhop.c

171–194 ↗

(On Diff #177854)

@markj here.

402 ↗

(On Diff #177854)

So this looks up a nexthop based on a hash of the nh_priv... how do you know there aren't multiple values with the same hash but different metrics? In that case, we're only looking at the first match.

Because we hash objects based on these values:
uint16_t ifentropy;
uint8_t family;
uint8_t nh_type;
uint32_t gw_addr;

These obj are not guaranteed to be unique either, so I used the fact to add metric too.

[markj]

sys/net/route/nhop.c

402 ↗

(On Diff #177854)

Suppose there are two identical objects with different metrics. CHT_SLIST_FIND_BYOBJ() will return one of them, then here we will check nh_priv_ret->nh->nh_metric != metric. If so, then we give up. So what about the second object?

[pouria]

Document metric != 0 condition by replacing it with macro name.
No functional change.

[pouria]

Address @markj comment.
Saved me, thank you.

[markj]

sys/net/route/nhop_ctl.c

176

Why not handle it in cmp_priv(), without introducing this new macro and comparator? nh_metric is after NHOP_END_CMP, so it does not get included in the first comparison. So, you can check:

if (_one->nh->nh_metric != RT_WILDCARD_METRIC && _one->nh->nh_metric != _two->nh->nh_metric)
    return (0);

This is a bit weird because it assumes that _one is the search key and _two is an actual object, but we can rename the parameters too...

[pouria]

sys/net/route/nhop_ctl.c

176

Why not handle it in cmp_priv(), without introducing this new macro and comparator? nh_metric is after NHOP_END_CMP, so it does not get included in the first comparison. So, you can check:

I was worried the cmp_priv name might confuse people, but I also prefer that approach anyway.
If you're okay with it, I'm happy to make that change.

if (_one->nh->nh_metric != RT_WILDCARD_METRIC && _one->nh->nh_metric != _two->nh->nh_metric)
    return (0);

This is a bit weird because it assumes that _one is the search key and _two is an actual object, but we can rename the parameters too...

Make sense. Will do.

[markj]

sys/net/route/nhop_ctl.c

176

Well, the cmp_priv() name is confusing either way IMO. (What is the relationship between nhop_object and nhop_priv? How do you decide whether to add a field to one vs. the other?)

I think having fewer macros/comparators/etc. is better.

[pouria]

Address @markj comment.
Remove extra macros and move metric cmp logic inside cmp_priv.
Rename cmp args.

[pouria]

sys/net/route/nhop_ctl.c

176

nhop_priv is control plane only.
nhop_object contains data necessary for the dataplane.
We don't use metric in the dataplane, so my alternative was to add metric into nhop_priv in the lookup section, which would also avoid the extra comparison.
But our nhop_priv comparison was aligned to 128 bits.
I didn't want to break that, so I used a spare member.
I'm not sure this design decision is worth it or not. but I believe breaking 2 lines of cache would be expensive per nhop...

[pouria]

Merge into D56322.