routing: Implement merge of nhgrp in new multipath route
AcceptedPublic
Actions

Authored by pouria on Tue, Mar 31, 4:08 PM.

Details

Reviewers

kp
markj
glebius
zlei
melifaro
ae

Group Reviewers

network

Summary

Routing subsystem allows creating new multipath routes by
nexthop groups (e.g RTA_MULTIPATH in netlink), in case of
a second nexthop group on the same route, don't panic and
merge the existing nhgrp with new one.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 71876
Build 68759: arc lint + arc unit

Event Timeline

pouria created this revision.Tue, Mar 31, 4:08 PM

Herald added a reviewer: melifaro. · View Herald TranscriptTue, Mar 31, 4:08 PM

Herald added subscribers: melifaro, ae, imp. · View Herald Transcript

pouria requested review of this revision.Tue, Mar 31, 4:08 PM

pouria added a child revision: D56188: netlink: Fix RTA_MULTIPATH on RTM_GETROUTE.

Harbormaster completed remote builds in B71865: Diff 174671.Tue, Mar 31, 4:09 PM

For tests, see D56190.

sys/net/route/nhgrp_ctl.c
836–838	For reviewers, it's meaningless to reference nhops of an nhgrp here like above. Therefore, I didn't.
843–845	For reviewers, don't worry about the reverse order here, it will eventually sorted by qsort.

For reviewers: this patch also fixes a panic caused by an incorrect assumption in nhgrp_get_addition_group().
That function assumes rnd->rnd_nhop is not an nhgrp_object, therefore sending two or more RTA_MULTIPATH attributes can trigger a panic.

pouria added a comment.Tue, Mar 31, 5:02 PM

This comment was removed by pouria.

glebius added inline comments.Tue, Mar 31, 6:30 PM

sys/net/route/nhgrp_ctl.c
662	The parenthesis around sizeof() are superfluous.

Address @glebius comment.

Harbormaster completed remote builds in B71876: Diff 174691.Tue, Mar 31, 7:18 PM

Generally LGTM with the only comment above.

sys/net/route/nhgrp_ctl.c
642	The nexthop group contains next's and their relative weights. I'd rather try to have a single representation of all variants, where next hops are not duplicated,. E.g. if the merged list contains nehhops [(A, W1), (B, W2), (A, W3)] then it should be represented by the nhg (A, W1+W3), (B, W2))]

This revision is now accepted and ready to land.Sat, Apr 11, 9:31 PM

pouria added inline comments.Sun, Apr 12, 4:18 PM

sys/net/route/nhgrp_ctl.c
642	Since part of the weight value is used for the metric (it was previously reserved for that), this would result in an incorrect metric because the metric value would be summed twice. Also, we don't do this in `append_nhops`, so our compiled nhops become inconsistent. We can fix `append_nhops`, but please also see my other revision, especially D56322. After D56322, I can overwrite existing duplicate nhops with same metric but different weights.