Paths

Table of Contentst

route/fib_algo: Use refcount(9) for fib_lookup_module
Needs ReviewPublic
Actions

Authored by pouria on Wed, Aug 12, 6:47 PM.

Details

Reviewers

glebius
melifaro
markj
kfv

Group Reviewers

network

Summary

Change flm_refcount from int to u_int and use refcount(9)
instead of manually setting flm_refcount.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 75577
Build 72460: arc lint + arc unit

Event Timeline

pouria created this revision.Wed, Aug 12, 6:47 PM

Herald added subscribers: ae, imp. · View Herald TranscriptWed, Aug 12, 6:47 PM

pouria requested review of this revision.Wed, Aug 12, 6:47 PM

Harbormaster completed remote builds in B75536: Diff 183926.Wed, Aug 12, 6:47 PM

That would make sense if you also got rid of FIB_MOD_LOCK() at least for some of the operations. The change as is just adds synchronization inside sections that are already synchronized by the mutex.

pouria added inline comments.Wed, Aug 12, 6:51 PM

sys/net/route/fib_algo.c
1448–1449	Not sure but I believe this lock here is not necessary anymore since we're using atomic(9) here.

In D58803#1349302, @glebius wrote:

That would make sense if you also got rid of FIB_MOD_LOCK() at least for some of the operations. The change as is just adds synchronization inside sections that are already synchronized by the mutex.

You're faster than my internet :D, I just asked.

Wanted to make sure, here is the removal of FIB_MOD_LOCK in unref_algo.

Harbormaster completed remote builds in B75539: Diff 183929.Wed, Aug 12, 7:06 PM

But why do this? What do we gain? These operations are rare. In such code, it is better to keep things simpler and "obviously correct." You're right that we can now probably drop that one use of the fib module lock, but now the code is harder to reason about and debug, and we do not gain anything in return (that I can see, at least).

In D58803#1349330, @markj wrote:

But why do this? What do we gain? These operations are rare. In such code, it is better to keep things simpler and "obviously correct." You're right that we can now probably drop that one use of the fib module lock, but now the code is harder to reason about and debug, and we do not gain anything in return (that I can see, at least).

Unfortunately, if you're using the radix[6]_lockless algorithm, which is usually true for IPv6, these operations happen frequently.
This is what I found while debugging PR297339.

You can test it yourself, set sysctl net.route.algo.debug_level=8 and try to keep a simple routing daemon for routes under 10k.
Then you see these functions are frequently run must of the time.
Since radix[6]_lockless rebuilds itself on every change.

I'm trying to cleanup fib_algo file.
It's too hard to work on it with radix_lockless.

I've some other changes, I'm fine if those are rejected. but this one actually helps reading the code.
Beside the optimization, at least, one can simply find the start of flm by looking for refcount_init.

In D58803#1349353, @pouria wrote:

In D58803#1349330, @markj wrote:

But why do this? What do we gain? These operations are rare. In such code, it is better to keep things simpler and "obviously correct." You're right that we can now probably drop that one use of the fib module lock, but now the code is harder to reason about and debug, and we do not gain anything in return (that I can see, at least).

Unfortunately, if you're using the radix[6]_lockless algorithm, which is usually true for IPv6, these operations happen frequently.
This is what I found while debugging PR297339.

You can test it yourself, set sysctl net.route.algo.debug_level=8 and try to keep a simple routing daemon for routes under 10k.
Then you see these functions are frequently run must of the time.
Since radix[6]_lockless rebuilds itself on every change.

I'm trying to cleanup fib_algo file.
It's too hard to work on it with radix_lockless.

What does "frequently" mean? Once a second? Ten thousand times a second? Does the change actually help performance in some measurable way?

I've some other changes, I'm fine if those are rejected. but this one actually helps reading the code.
Beside the optimization, at least, one can simply find the start of flm by looking for refcount_init.

Why not just assign flm->flm_refcount = 0 there explicitly? That seems fine to me.

In D58803#1349372, @markj wrote:

In D58803#1349353, @pouria wrote:

In D58803#1349330, @markj wrote:

But why do this? What do we gain? These operations are rare. In such code, it is better to keep things simpler and "obviously correct." You're right that we can now probably drop that one use of the fib module lock, but now the code is harder to reason about and debug, and we do not gain anything in return (that I can see, at least).

Unfortunately, if you're using the radix[6]_lockless algorithm, which is usually true for IPv6, these operations happen frequently.
This is what I found while debugging PR297339.

You can test it yourself, set sysctl net.route.algo.debug_level=8 and try to keep a simple routing daemon for routes under 10k.
Then you see these functions are frequently run must of the time.
Since radix[6]_lockless rebuilds itself on every change.

I'm trying to cleanup fib_algo file.
It's too hard to work on it with radix_lockless.

What does "frequently" mean? Once a second? Ten thousand times a second? Does the change actually help performance in some measurable way?

Ok, I just run the dtrace below on my IXP router (unmodified CURRENT kernel).
This might answer your question:

2026 Aug 12 23:20:26 | fib_unref_nhop calls/sec: 0
2026 Aug 12 23:20:27 | fib_unref_nhop calls/sec: 0
2026 Aug 12 23:20:28 | fib_unref_nhop calls/sec: 28457
2026 Aug 12 23:20:29 | fib_unref_nhop calls/sec: 29924
2026 Aug 12 23:20:30 | fib_unref_nhop calls/sec: 15264
2026 Aug 12 23:20:31 | fib_unref_nhop calls/sec: 0
2026 Aug 12 23:20:32 | fib_unref_nhop calls/sec: 26293
2026 Aug 12 23:20:33 | fib_unref_nhop calls/sec: 5281
2026 Aug 12 23:20:34 | fib_unref_nhop calls/sec: 11039
2026 Aug 12 23:20:35 | fib_unref_nhop calls/sec: 20683
2026 Aug 12 23:20:36 | fib_unref_nhop calls/sec: 0
2026 Aug 12 23:20:37 | fib_unref_nhop calls/sec: 40568
2026 Aug 12 23:20:38 | fib_unref_nhop calls/sec: 20572
2026 Aug 12 23:20:39 | fib_unref_nhop calls/sec: 0
2026 Aug 12 23:20:40 | fib_unref_nhop calls/sec: 62496
2026 Aug 12 23:20:41 | fib_unref_nhop calls/sec: 26772
2026 Aug 12 23:20:42 | fib_unref_nhop calls/sec: 16567
2026 Aug 12 23:20:43 | fib_unref_nhop calls/sec: 0
2026 Aug 12 23:20:44 | fib_unref_nhop calls/sec: 58711
2026 Aug 12 23:20:45 | fib_unref_nhop calls/sec: 12417
2026 Aug 12 23:20:46 | fib_unref_nhop calls/sec: 51156
2026 Aug 12 23:20:47 | fib_unref_nhop calls/sec: 8387
2026 Aug 12 23:20:48 | fib_unref_nhop calls/sec: 35018
2026 Aug 12 23:20:49 | fib_unref_nhop calls/sec: 0
2026 Aug 12 23:20:50 | fib_unref_nhop calls/sec: 25337
2026 Aug 12 23:20:51 | fib_unref_nhop calls/sec: 27799
2026 Aug 12 23:20:52 | fib_unref_nhop calls/sec: 0
2026 Aug 12 23:20:53 | fib_unref_nhop calls/sec: 30136
2026 Aug 12 23:20:54 | fib_unref_nhop calls/sec: 27932
2026 Aug 12 23:20:55 | fib_unref_nhop calls/sec: 61646
2026 Aug 12 23:20:56 | fib_unref_nhop calls/sec: 0
2026 Aug 12 23:20:57 | fib_unref_nhop calls/sec: 49824
2026 Aug 12 23:20:58 | fib_unref_nhop calls/sec: 0
2026 Aug 12 23:20:59 | fib_unref_nhop calls/sec: 13276
2026 Aug 12 23:21:00 | fib_unref_nhop calls/sec: 33503
2026 Aug 12 23:21:01 | fib_unref_nhop calls/sec: 0
2026 Aug 12 23:21:02 | fib_unref_nhop calls/sec: 34121
2026 Aug 12 23:21:03 | fib_unref_nhop calls/sec: 0
2026 Aug 12 23:21:04 | fib_unref_nhop calls/sec: 114
2026 Aug 12 23:21:05 | fib_unref_nhop calls/sec: 0
2026 Aug 12 23:21:06 | fib_unref_nhop calls/sec: 2
2026 Aug 12 23:21:07 | fib_unref_nhop calls/sec: 0
2026 Aug 12 23:21:08 | fib_unref_nhop calls/sec: 0

dtrace.s:

fbt:kernel:fib_unref_nhop:entry
{
fib_calls++;
}

tick-1s
{
printf("%Y | fib_unref_nhop calls/sec: %d\n", walltimestamp, fib_calls);
fib_calls = 0;
}

I had to restart my router because of radix6_lockless memory leak.
The output below is RIGHT after boot with dpdk_lpm6 + dxr:

[pouria@nanali] [~] % uptime
11:29PM  up 45 secs, 1 user, load averages: 2.25, 0.55, 0.20
[pouria@nanali] [~] % kldload dtraceall
kldload: can't load dtraceall: Operation not permitted
[pouria@nanali] [~] % mdo kldload dtraceall
[pouria@nanali] [~] % mdo dtrace -s stats.d
2026 Aug 12 23:29:39 | fib_unref_nhop calls/sec: 6932
2026 Aug 12 23:29:40 | fib_unref_nhop calls/sec: 6580
2026 Aug 12 23:29:41 | fib_unref_nhop calls/sec: 7452
2026 Aug 12 23:29:42 | fib_unref_nhop calls/sec: 8890
2026 Aug 12 23:29:43 | fib_unref_nhop calls/sec: 6836
2026 Aug 12 23:29:44 | fib_unref_nhop calls/sec: 6776
2026 Aug 12 23:29:45 | fib_unref_nhop calls/sec: 5161
2026 Aug 12 23:29:46 | fib_unref_nhop calls/sec: 9029
2026 Aug 12 23:29:47 | fib_unref_nhop calls/sec: 8917
2026 Aug 12 23:29:48 | fib_unref_nhop calls/sec: 7426
2026 Aug 12 23:29:49 | fib_unref_nhop calls/sec: 3824

In D58803#1349394, @pouria wrote:
In D58803#1349372, @markj wrote:

In D58803#1349353, @pouria wrote:

In D58803#1349330, @markj wrote:

But why do this? What do we gain? These operations are rare. In such code, it is better to keep things simpler and "obviously correct." You're right that we can now probably drop that one use of the fib module lock, but now the code is harder to reason about and debug, and we do not gain anything in return (that I can see, at least).

Unfortunately, if you're using the radix[6]_lockless algorithm, which is usually true for IPv6, these operations happen frequently.
This is what I found while debugging PR297339.

You can test it yourself, set sysctl net.route.algo.debug_level=8 and try to keep a simple routing daemon for routes under 10k.
Then you see these functions are frequently run must of the time.
Since radix[6]_lockless rebuilds itself on every change.

I'm trying to cleanup fib_algo file.
It's too hard to work on it with radix_lockless.

What does "frequently" mean? Once a second? Ten thousand times a second? Does the change actually help performance in some measurable way?

Ok, I just run the dtrace below on my IXP router (unmodified CURRENT kernel).
This might answer your question:
[...]
dtrace.s:
fbt:kernel:fib_unref_nhop:entry
{
fib_calls++;
}

BTW, this is racy, see the note here https://docs.oracle.com/en/operating-systems/solaris/oracle-solaris/11.4/dtrace-guide/variables-dtrace.html

Better would be to use an aggregation: @calls = count();

tick-1s
{
printf("%Y | fib_unref_nhop calls/sec: %d\n", walltimestamp, fib_calls);
fib_calls = 0;
}

Ok, but fib_unref_nhop() doesn't acquire the module lock.

In D58803#1349400, @markj wrote:
In D58803#1349394, @pouria wrote:
In D58803#1349372, @markj wrote:

In D58803#1349353, @pouria wrote:

In D58803#1349330, @markj wrote:

But why do this? What do we gain? These operations are rare. In such code, it is better to keep things simpler and "obviously correct." You're right that we can now probably drop that one use of the fib module lock, but now the code is harder to reason about and debug, and we do not gain anything in return (that I can see, at least).

Unfortunately, if you're using the radix[6]_lockless algorithm, which is usually true for IPv6, these operations happen frequently.
This is what I found while debugging PR297339.

You can test it yourself, set sysctl net.route.algo.debug_level=8 and try to keep a simple routing daemon for routes under 10k.
Then you see these functions are frequently run must of the time.
Since radix[6]_lockless rebuilds itself on every change.

I'm trying to cleanup fib_algo file.
It's too hard to work on it with radix_lockless.

What does "frequently" mean? Once a second? Ten thousand times a second? Does the change actually help performance in some measurable way?

Ok, I just run the dtrace below on my IXP router (unmodified CURRENT kernel).
This might answer your question:
[...]
dtrace.s:
fbt:kernel:fib_unref_nhop:entry
{
fib_calls++;
}
BTW, this is racy, see the note here https://docs.oracle.com/en/operating-systems/solaris/oracle-solaris/11.4/dtrace-guide/variables-dtrace.html

Better would be to use an aggregation: @calls = count();
tick-1s
{
printf("%Y | fib_unref_nhop calls/sec: %d\n", walltimestamp, fib_calls);
fib_calls = 0;
}
Ok, but fib_unref_nhop() doesn't acquire the module lock.

Oops, For once I have asked the stochastic parrot to write me a test in dtrace, and here we go.
should be fib_unref_algo instead and looks like we don't have unref_algo in dtrace (probably become inline).

Thank you for the hint.
At this point, not sure how to test it.

In D58803#1349423, @pouria wrote:

Thank you for the hint.
At this point, not sure how to test it.

Something like dtrace -n 'lockstat:::adaptive-acquire /args[0]->lock_object.lo_name == "algo list mutex"/{printf("hi");}' can be used to get a sense for how often this lock is acquired.

In D58803#1349425, @markj wrote:

Something like dtrace -n 'lockstat:::adaptive-acquire /args[0]->lock_object.lo_name == "algo list mutex"/{printf("hi");}' can be used to get a sense for how often this lock is acquired.

THANK YOU!
Before patch, after boot with bird starting its neighborship:

[root@ftsr1] [~] # dtrace -s stats.d
2026 Aug 13 10:23:55 calls/sec:
317
2026 Aug 13 10:23:56 calls/sec:
299
2026 Aug 13 10:23:57 calls/sec:
312
2026 Aug 13 10:23:58 calls/sec:
327
2026 Aug 13 10:23:59 calls/sec:
396
2026 Aug 13 10:24:00 calls/sec:
328
2026 Aug 13 10:24:01 calls/sec:
321
2026 Aug 13 10:24:02 calls/sec:
380
2026 Aug 13 10:24:03 calls/sec:
356
2026 Aug 13 10:24:04 calls/sec:
278
2026 Aug 13 10:24:05 calls/sec:
302
2026 Aug 13 10:24:06 calls/sec:
321
2026 Aug 13 10:24:07 calls/sec:
397
2026 Aug 13 10:24:08 calls/sec:
399

It looks like a bottleneck, Increasing cpu won't change the result.

dtrace:

lockstat:::adaptive-acquire
/args[0]->lock_object.lo_name == "algo list mutex"/
{
  @calls = count();
}

tick-1sec
{
  printf("%Y calls/sec:", walltimestamp);
  printa(@calls);
  clear(@calls);
}

In D58803#1349622, @pouria wrote:
In D58803#1349425, @markj wrote:

Something like dtrace -n 'lockstat:::adaptive-acquire /args[0]->lock_object.lo_name == "algo list mutex"/{printf("hi");}' can be used to get a sense for how often this lock is acquired.

THANK YOU!
Before patch, after boot with bird starting its neighborship:
[root@ftsr1] [~] # dtrace -s stats.d
2026 Aug 13 10:23:55 calls/sec:
317
2026 Aug 13 10:23:56 calls/sec:
299
2026 Aug 13 10:23:57 calls/sec:
312
2026 Aug 13 10:23:58 calls/sec:
327
2026 Aug 13 10:23:59 calls/sec:
396
2026 Aug 13 10:24:00 calls/sec:
328
2026 Aug 13 10:24:01 calls/sec:
321
2026 Aug 13 10:24:02 calls/sec:
380
2026 Aug 13 10:24:03 calls/sec:
356
2026 Aug 13 10:24:04 calls/sec:
278
2026 Aug 13 10:24:05 calls/sec:
302
2026 Aug 13 10:24:06 calls/sec:
321
2026 Aug 13 10:24:07 calls/sec:
397
2026 Aug 13 10:24:08 calls/sec:
399
It looks like a bottleneck, Increasing cpu won't change the result.

dtrace:
lockstat:::adaptive-acquire
/args[0]->lock_object.lo_name == "algo list mutex"/
{
  @calls = count();
}

tick-1sec
{
  printf("%Y calls/sec:", walltimestamp);
  printa(@calls);
  clear(@calls);
}

You can dig deeper: where are the lock acquisitions coming from? Use stack(). Is there actual lock contention? Use lockstat:::adaptive-spin/adaptive-block to look for this. See dtrace_lockstat(4).

In D58803#1349648, @markj wrote:

You can dig deeper: where are the lock acquisitions coming from? Use stack(). Is there actual lock contention? Use lockstat:::adaptive-spin/adaptive-block to look for this. See dtrace_lockstat(4).

before patch:

2026 Aug 13 19:37:55 block:
kernel`__mtx_lock_flags+0xdd
kernel`destroy_fd_instance_epoch+0x165
kernel`epoch_call_task+0x14a
kernel`gtaskqueue_run_locked+0x1a8
kernel`gtaskqueue_thread_loop+0xd3
kernel`fork_exit+0x82
kernel`0xffffffff8119aece
2026 Aug 13 19:37:55 block:
kernel`__mtx_lock_flags+0xdd
kernel`destroy_fd_instance_epoch+0x165
kernel`epoch_call_task+0x14a
kernel`gtaskqueue_run_locked+0x1a8
kernel`gtaskqueue_thread_loop+0xd3
kernel`fork_exit+0x82
kernel`0xffffffff8119aece

dtrace:

...
lockstat:::adaptive-block
/args[0]->lock_object.lo_name == "algo list mutex"/
{
  @calls["block"] = count();
  printf("%Y block:", walltimestamp);
  stack();
}
...

It won't happen with my patch.

pouria added inline comments.Thu, Aug 13, 7:48 PM

sys/net/route/fib_algo.c
1448–1449	Not sure but I believe this lock here is not necessary anymore since we're using atomic(9) here. @markj it seems like the removal of the lock here was important after all (on radix_lockless, which is the default after 10 routes). No contention occurs with this patch.

Unlock earlier in fib_check_best_algo.

Harbormaster completed remote builds in B75577: Diff 184018.Thu, Aug 13, 7:59 PM

pouria added inline comments.Thu, Aug 13, 8:01 PM

sys/net/route/fib_algo.c

1923–1930

Also, some of the lock pressure comes from here.
Unlock earlier.

2026 Aug 13 19:57:00 block:
kernel`__mtx_lock_flags+0xdd
kernel`fib_check_best_algo+0x5a
kernel`handle_fd_callout+0x14c
kernel`softclock_call_cc+0x1b5
kernel`softclock_thread+0xc6
kernel`fork_exit+0x82
kernel`0xffffffff8119aece

Delay lock in fib_module_unregister.

Harbormaster completed remote builds in B75591: Diff 184056.Fri, Aug 14, 3:57 PM

pouria added a reviewer: kfv.Fri, Aug 14, 3:58 PM

pouria added inline comments.