On Sun, Dec 29, 2024 at 09:50:52PM -0700, Theo de Raadt wrote:
> > sys/atomic.h has #ifndef SYS_ATOMIC_H guard against it.
> 
> But that's not the problem being described.
> 
> The problem is that everyone has to include the file. It becomes
> a required file.  So it will be over-included.  You can talk about
> the guard all you want, but it is still pulled in.
> 
> And when it isn't, it will get added in unreasonable places.
> It is still multiple inclusion.
> 
> Adding it to the intr.h style files from the get-go is a clear
> indication that there is no clear understanding that this is being
> added as a new "intrinsic type".  Those are added in sys/_types.h.
> But obviously it can't be there, right?  There's a reason for
> pulling it in the wrong place. Eventually it will be pulled in from
> many more wrong places.  Guard or not, is NOT FREE.
> 
> The second question is why does this variable need to suddenly be
> protected as atomic.  Does it change halfway through a panic?  Or is
> this dogma.  Is it dogma driven by some tools?

I agree that this diff is just bad. I did not understand why all of a
sudden every variable used by sysctl needs to use atomic_load_int().
Especially in this case this is most probably incorrect.
The variable is evalauted in the if block of the macro but also in the
splassert_fail() now splassert_fail() will panic if the value is changed
back to 0 in between those two calls..  At least splassert_fail() needs to
handle 0 as well. Once that is done then the question is why require an
atomic load when it is not used in an atomic fashion anyway?

Also what is the price of forcing loads all the time?
-- 
:wq Claudio