Page MenuHomeFreeBSD
Differential D52731

sys/netinet6: Use atomic(9) for dad_failures counter
ClosedPublic
Actions

Authored by madpilot on Sep 25 2025, 1:44 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Jan 14, 5:14 PM
Unknown Object (File)
Wed, Jan 14, 12:24 PM
Unknown Object (File)
Wed, Jan 14, 12:05 PM
Unknown Object (File)
Mon, Jan 12, 8:55 PM
Unknown Object (File)
Mon, Jan 12, 8:55 PM
Unknown Object (File)
Sat, Jan 10, 5:31 AM
Unknown Object (File)
Sat, Jan 10, 5:31 AM
Unknown Object (File)
Mon, Jan 5, 1:20 AM
View All 63 Files
Subscribers
ae
cperciva
imp
jhibbits
lytboris_gmail.com
melifaro
zarychtam_plan-b.pwste.edu.pl

Details

Reviewers
jtl
zlei
glebius
Commits
rG0c94dc9da7c2: sys/netinet6: Use atomic(9) for dad_failures counter
Summary

As suggested by jtl counter(9) objects are overkill for this variable and atomic makes more sense.

This patch replaces the counter_u64_t with a uint8_t and uses atomic(9) to handle the value.

I decided to use an 8 bit value since the counter is not expected to reach any high values.

Since the counter is now smaller, I modified the related stableaddr_maxretries sysctl to be a SYSCTL_U8 value, this avoids the risk of the sysctl being set to a value higher than the counter can count.

While here I also moved includes I added to respect style(9).

The atomic(9) man page has a warning about using the variants with size <= 16 bits in portable code, but I noticed there are instances of this usage in the kernel (for example in tmpfs, ena driver, and other instances), so I guess it is allowed.

Test Plan

I've tested this in virtual machines and on my laptop, also forcing dad failures, and it works as expected.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 67327
Build 64210: arc lint + arc unit

Event Timeline

madpilot created this revision.Sep 25 2025, 1:44 PM
Herald added subscribers: melifaro, ae, imp. · View Herald TranscriptSep 25 2025, 1:44 PM
madpilot requested review of this revision.Sep 25 2025, 1:44 PM
Harbormaster completed remote builds in B67327: Diff 162826.Sep 25 2025, 1:44 PM
madpilot added inline comments.Sep 25 2025, 1:53 PM
sys/netinet6/nd6_nbr.c
1527

Oops, this should obviously be uint8_t. I'm going to correct this shortly.

zlei added a subscriber: jhibbits.Sep 25 2025, 2:57 PM
zlei added inline comments.
sys/netinet6/in6_ifattach.c
382

It appears powerpc does not have atomic_load_acq_8() / atomic_store_rel_8() defined. CC @jhibbits .

jhibbits added inline comments.Sep 25 2025, 3:16 PM
sys/netinet6/in6_ifattach.c
382

I see atomic_load_acq_8() defined in sys/sys/_atomic_subword.h, but no atomic_store_rel_8(). So powerpc is "safe" for the load, but not the store.

madpilot updated this revision to Diff 162852.Sep 25 2025, 7:37 PM

THanks for the feedback!

I've updated the code, fixing the type in nd6_nbr.c.

I also tried to addressthe issue about atomic_store_rel_8() on powerpc.

Unluckily I'm unable to test this and I'm not sure I got it right.

My idea is to use atomic_store_rel_8() if available otherwise fallback to atomic_store_8() and accept the minor risk of out of order store.

As far as I understand, atomic_store_8() should be available on all supported architectures, it is being used in multiple places in the kernel sources.

Harbormaster completed remote builds in B67335: Diff 162852.Sep 25 2025, 7:37 PM
glebius added a comment.Sep 27 2025, 10:07 AM

@jtl is absolutely right with this suggestion. However, what's the point on saving bits and going with uint8_t, especially with portability considerations. We aren't saving any memory here. Why not just use u_int?

madpilot planned changes to this revision.Sep 27 2025, 12:44 PM
madpilot marked 2 inline comments as done.

@glebius

I think you're correct. Also adding the #ifdef looks a little ugly to me, and I'd be happy to avoid that.

madpilot added inline comments.Sep 27 2025, 1:40 PM
sys/netinet6/in6_ifattach.c
428

I now notice I did introduce a bug here, that does not fail by random chance.

Changed the size of dad_failures, but I'm still reading 64 bits here.

Now, if I change the size from 64 bits to 32 bit, I need to read only 4 bytes and hashes will change for people already using this.

If we want to make the counter 32 bit, better do this now. I'd be in favour of doing this with this change and fix it for the future.

madpilot edited the summary of this revision. (Show Details)Sep 27 2025, 1:43 PM
madpilot updated this revision to Diff 162925.Sep 27 2025, 2:45 PM

Updated the patch to use 32 bit sized counter and atomic(9) methods.

This should fix the portability issues and work on all architectures.

Also aligned to this type the sysctl.

I think I also found a good middle ground in relation the the issue in sys/netinet6/in6_ifattach.c line 428.

The variable dad_counter in in6_get_stableifid() can stay as an uint64_t, so we can still hash 8 bytes and hashes will not change with this patch.

The extra 4 bytes will be unused at present, but they can be useful for "future expansion" anyway.

Maybe I should add a comment there to explain this, if this logic is acceptable.

Harbormaster completed remote builds in B67366: Diff 162925.Sep 27 2025, 2:46 PM
glebius accepted this revision.Sep 29 2025, 7:35 AM
This revision is now accepted and ready to land.Sep 29 2025, 7:35 AM
zlei accepted this revision.Sep 29 2025, 8:04 AM

This generally looks good to me.

sys/netinet6/in6_var.h
109

I'd personally prefer u_int directly and just forget it is a 32 bits member. The accessors can be atomic_store_int and atomic_load_int which is straight forward.

@glebius I do not think a u_int will produce much headache when struct in6_ifextra is part of KPI. Any comments ?

sys/netinet6/nd6_nbr.c
1478

It appears that the rel / acq variants of atomic_store and atomic_add atomic_store are a little strong for the purpose ( atomic access the DAD failures ). It appears it is sufficient to use the latter ones. Any reason to choose the rel / acq ?

madpilot added inline comments.Sep 29 2025, 9:50 AM
sys/netinet6/in6_var.h
109

I confess I was under the impression that exact sized types are to be preferred.

OTOH I don't see the size of u_int changing any time soon for any architecture so it really makes little difference.

So I will follow what people more versed than me in kernel land will suggest me.

sys/netinet6/nd6_nbr.c
1478

I did use the rel/acq variants so I can be sure reads in in6_get_stableifid() are ordered, at least if I understand correctly the semantics.

But I can agree for this use this can be overkill, if this is the opinion I can change it to use the "bare" atomic methods.

glebius added inline comments.Sep 30 2025, 4:49 PM
sys/netinet6/in6_var.h
109

@zlei I suggested u_int instead of uint8_t above. I'm not an expert here. With my limited knowledge, for atomics, that are machine dependent, we probably want the machine word type, as that supposedly is the most optimized atomic operation. Thus I suggested u_int.
@madpilot The exact sized types are preferred sometimes, e.g. when we care about memory layout optimization or we need to meet certain maximum value demands.

madpilot updated this revision to Diff 163355.Oct 2 2025, 4:02 PM

Simplified patch, using u_int as suggested.

Also droping the req/acq semantics in atomic accessors.

Keeping the local dad_failures variable in in6_get_stableifid() as an uint64_t, though.

Keeping it at 64 bit value avoids changing the digest output after the patch, for the same input, and leaves some headroom in case of future changes.

This revision now requires review to proceed.Oct 2 2025, 4:02 PM
Harbormaster completed remote builds in B67494: Diff 163355.Oct 2 2025, 4:02 PM
madpilot marked 2 inline comments as done.Oct 2 2025, 4:04 PM

Thanks for the suggestions. I hope this patch update addresses all the issues.

glebius accepted this revision.Oct 2 2025, 4:56 PM
This revision is now accepted and ready to land.Oct 2 2025, 4:56 PM
jtl accepted this revision.Oct 2 2025, 5:07 PM

Thanks for doing this! LGTM other than one nit.

sys/netinet6/nd6_rtr.c
1762

atomic_load_int()?

madpilot marked 2 inline comments as done.Oct 2 2025, 7:38 PM
madpilot added inline comments.
sys/netinet6/nd6_rtr.c
1762

Thanks for spotting this. It clearly slipped by.

I'm correcting it before pushing.

zlei accepted this revision.Oct 3 2025, 12:54 AM

Looks good to me.

madpilot retitled this revision from sys/netinet6: Replace use atomic(9) for dad_faulires counter to sys/netinet6: Use atomic(9) for dad_failures counter.Oct 3 2025, 10:02 AM
Closed by commit rG0c94dc9da7c2: sys/netinet6: Use atomic(9) for dad_failures counter (authored by madpilot). · Explain WhyOct 3 2025, 10:11 AM
This revision was automatically updated to reflect the committed changes.
madpilot marked an inline comment as done.
madpilot added a commit: rG0c94dc9da7c2: sys/netinet6: Use atomic(9) for dad_failures counter.

Revision Contents
Changeset List

PathSize
sys/
netinet6/
3 lines
12 lines
4 lines
2 lines
2 lines
8 lines
4 lines

Diff 162826

View Options

sys/netinet6/in6.c

Loading...
View Options

sys/netinet6/in6_ifattach.c

Loading...
View Options

sys/netinet6/in6_proto.c

Loading...
View Options

sys/netinet6/in6_var.h

Loading...
View Options

sys/netinet6/ip6_var.h

Loading...
View Options

sys/netinet6/nd6_nbr.c

Loading...
View Options

sys/netinet6/nd6_rtr.c

Loading...