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

[madpilot]

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.

[madpilot]

sys/netinet6/nd6_nbr.c

1514

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

[zlei]

sys/netinet6/in6_ifattach.c

382

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

[jhibbits]

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]

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.

[glebius]

@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]

@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]

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]

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.

[zlei]

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

1465

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]

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

1465

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]

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]

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.

[madpilot]

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

[jtl]

Thanks for doing this! LGTM other than one nit.

sys/netinet6/nd6_rtr.c

1761

atomic_load_int()?

[madpilot]

sys/netinet6/nd6_rtr.c

1761

Thanks for spotting this. It clearly slipped by.

I'm correcting it before pushing.

[zlei]

Looks good to me.