Page MenuHomeFreeBSD
Differential D28434

Add FIB_ALGO to GENERIC on amd64/arm64.
ClosedPublic
Actions

Authored by melifaro on Jan 31 2021, 11:56 AM.
Tags
None
Referenced Files
F170025839: D28434.id88091.diff
Thu, Sep 3, 7:09 AM
F169926283: D28434.id83175.diff
Thu, Sep 3, 12:05 AM
Unknown Object (File)
Tue, Sep 1, 5:19 PM
Unknown Object (File)
Mon, Aug 31, 11:40 PM
Unknown Object (File)
Sat, Aug 29, 9:05 PM
Unknown Object (File)
Sat, Aug 29, 7:21 PM
Unknown Object (File)
Thu, Aug 27, 7:50 PM
Unknown Object (File)
Thu, Aug 27, 7:13 PM
View All Files
Subscribers
andrew
arichardson
emaste
imp
jrtc27
markj

Details

Reviewers
manu
olivier
ae
gnn
Group Reviewers
network
Commits
rG0ca122044369: Add FIB_ALGO to GENERIC on amd64/arm64.
rG6993187a8c30: Add FIB_ALGO to GENERIC on amd64/arm64.
Summary

Option FIB_ALGO gates new modular fib lookup functionality, enabling more performant routing table lookups.

The initial support was added in D27401, back in December.

A number of issues have been fixed since then: 1, 2, 3.

At this point, I believe it should be ready for the wider audience, e.g. all -CURRENT users.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 36633
Build 33522: arc lint + arc unit

Event Timeline

melifaro created this revision.Jan 31 2021, 11:56 AM
Herald added a reviewer: manu. · View Herald TranscriptJan 31 2021, 11:56 AM
Herald added subscribers: emaste, andrew, imp. · View Herald Transcript
melifaro requested review of this revision.Jan 31 2021, 11:56 AM
Harbormaster completed remote builds in B36633: Diff 83175.Jan 31 2021, 11:56 AM
melifaro edited the summary of this revision. (Show Details)Jan 31 2021, 1:15 PM
melifaro added reviewers: network, olivier, ae.
melifaro changed the repository for this revision from rS FreeBSD src repository - subversion to rG FreeBSD src repository.
olivier accepted this revision as: olivier.Feb 2 2021, 5:21 PM
This revision is now accepted and ready to land.Feb 2 2021, 5:21 PM
melifaro added a comment.Apr 20 2021, 10:43 PM

Going to commit this on Saturday, April 24

gnn accepted this revision.Apr 20 2021, 11:38 PM
Closed by commit rG6993187a8c30: Add FIB_ALGO to GENERIC on amd64/arm64. (authored by melifaro). · Explain WhyApr 24 2021, 11:25 PM
This revision was automatically updated to reflect the committed changes.
melifaro added a commit: rG6993187a8c30: Add FIB_ALGO to GENERIC on amd64/arm64..
arichardson added a subscriber: arichardson.Apr 26 2021, 8:49 AM

This may be the cause of CI failures starting in build 18011: https://ci.freebsd.org/job/FreeBSD-main-amd64-test/18011/
The only changes are the following the commits (https://ci.freebsd.org/job/FreeBSD-main-amd64-test/18011/changes):
bddae5c8a64dc6b292198945cbe676bb2158d438, 5d1403a79a3e56403fb63c062252a23fce81e5f1, and 6993187a8c30e83a408aad631a8d8629d8273c9d

melifaro added a comment.Apr 26 2021, 8:58 AM
In D28434#672782, @arichardson wrote:

This may be the cause of CI failures starting in build 18011: https://ci.freebsd.org/job/FreeBSD-main-amd64-test/18011/
The only changes are the following the commits (https://ci.freebsd.org/job/FreeBSD-main-amd64-test/18011/changes):
bddae5c8a64dc6b292198945cbe676bb2158d438, 5d1403a79a3e56403fb63c062252a23fce81e5f1, and 6993187a8c30e83a408aad631a8d8629d8273c9d

Ack. Interesting, local test run worked. Will take a look in the evening.

markj added a subscriber: markj.Apr 26 2021, 12:55 PM
In D28434#672782, @arichardson wrote:

This may be the cause of CI failures starting in build 18011: https://ci.freebsd.org/job/FreeBSD-main-amd64-test/18011/
The only changes are the following the commits (https://ci.freebsd.org/job/FreeBSD-main-amd64-test/18011/changes):
bddae5c8a64dc6b292198945cbe676bb2158d438, 5d1403a79a3e56403fb63c062252a23fce81e5f1, and 6993187a8c30e83a408aad631a8d8629d8273c9d

It's yet another problem related to type definitions and CTF, causing dtrace to bail. In this case, struct rib_head has a conditionally defined member, rnh_gen_rib. FIB_ALGO is defined only when opt_route.h is included, so unless everything that includes route.h also includes opt_route.h, we end up with different definitions for struct rib_head depending on whether opt_route.h is included. I can see two solutions: move FIB_ALGO to opt_global.h so that FIB_ALGO is consistently defined, or make the definition of rnh_gen_fib unconditional. I prefer the second solution since out-of-tree modules could be compiled without FIB_ALGO defined.

Even if not for the CTF limits causing problems here, I think it's good to address these kinds of problems as otherwise debuggers can end up using the wrong definition when it's ambiguous.

markj added a comment.Apr 26 2021, 1:23 PM
In D28434#672871, @markj wrote:
In D28434#672782, @arichardson wrote:

This may be the cause of CI failures starting in build 18011: https://ci.freebsd.org/job/FreeBSD-main-amd64-test/18011/
The only changes are the following the commits (https://ci.freebsd.org/job/FreeBSD-main-amd64-test/18011/changes):
bddae5c8a64dc6b292198945cbe676bb2158d438, 5d1403a79a3e56403fb63c062252a23fce81e5f1, and 6993187a8c30e83a408aad631a8d8629d8273c9d

It's yet another problem related to type definitions and CTF, causing dtrace to bail. In this case, struct rib_head has a conditionally defined member, rnh_gen_rib. FIB_ALGO is defined only when opt_route.h is included, so unless everything that includes route.h also includes opt_route.h, we end up with different definitions for struct rib_head depending on whether opt_route.h is included. I can see two solutions: move FIB_ALGO to opt_global.h so that FIB_ALGO is consistently defined, or make the definition of rnh_gen_fib unconditional. I prefer the second solution since out-of-tree modules could be compiled without FIB_ALGO defined.

I misread a bit, the problematic definition is in route_var.h, not route.h, and so is not so widely included in the kernel. Indeed, some files in sys/net/route fail to include opt_route.h. It may be sufficient to just fix those, as a third solution.

melifaro added a comment.Apr 27 2021, 8:14 AM
In D28434#672895, @markj wrote:
In D28434#672871, @markj wrote:
In D28434#672782, @arichardson wrote:

This may be the cause of CI failures starting in build 18011: https://ci.freebsd.org/job/FreeBSD-main-amd64-test/18011/
The only changes are the following the commits (https://ci.freebsd.org/job/FreeBSD-main-amd64-test/18011/changes):
bddae5c8a64dc6b292198945cbe676bb2158d438, 5d1403a79a3e56403fb63c062252a23fce81e5f1, and 6993187a8c30e83a408aad631a8d8629d8273c9d

It's yet another problem related to type definitions and CTF, causing dtrace to bail. In this case, struct rib_head has a conditionally defined member, rnh_gen_rib. FIB_ALGO is defined only when opt_route.h is included, so unless everything that includes route.h also includes opt_route.h, we end up with different definitions for struct rib_head depending on whether opt_route.h is included. I can see two solutions: move FIB_ALGO to opt_global.h so that FIB_ALGO is consistently defined, or make the definition of rnh_gen_fib unconditional. I prefer the second solution since out-of-tree modules could be compiled without FIB_ALGO defined.

Option 2 landed as bc5ef45aec3fa8acf2dd3408cebd207317543a8b. Thank you!

I misread a bit, the problematic definition is in route_var.h, not route.h, and so is not so widely included in the kernel. Indeed, some files in sys/net/route fail to include opt_route.h. It may be sufficient to just fix those, as a third solution.

melifaro added a comment.Apr 27 2021, 8:34 AM
In D28434#672787, @melifaro wrote:
In D28434#672782, @arichardson wrote:

This may be the cause of CI failures starting in build 18011: https://ci.freebsd.org/job/FreeBSD-main-amd64-test/18011/
The only changes are the following the commits (https://ci.freebsd.org/job/FreeBSD-main-amd64-test/18011/changes):
bddae5c8a64dc6b292198945cbe676bb2158d438, 5d1403a79a3e56403fb63c062252a23fce81e5f1, and 6993187a8c30e83a408aad631a8d8629d8273c9d

Ack. Interesting, local test run worked. Will take a look in the evening.

Should be better with 439d087d0b55.

jrtc27 added a subscriber: jrtc27.Nov 17 2021, 7:07 PM

Is there a reason why this was only enabled on amd64 and arm64? I'd expect this kind of thing to either be for all kernels or for all 64-bit kernels (maybe excluding mips), but powerpcp64(le) doesn't enable it despite having big iron servers, nor does riscv (though performance sucks on all shipping hardware currently anyway so hardly a big deal). Normally the only reason to limit something to amd64+arm64 is if it's not supported anywhere else.

gbe added a commit: rG0ca122044369: Add FIB_ALGO to GENERIC on amd64/arm64..Sep 14 2022, 9:02 PM

Revision Contents
Changeset List

PathSize
sys/
amd64/
conf/
1 line
arm64/
conf/
1 line

Diff 83175

View Options

sys/amd64/conf/GENERIC

Loading...
View Options

sys/arm64/conf/GENERIC

Loading...