> On 30 Dec 2024, at 13:04, Claudio Jeker <cjeker@diehard.n-r-g.com> wrote:
> 
> 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
> 

splassert_fail() loads the splassert_ctl value each time. So, it
will panic even now if value changed back to 0. Regardless on this
diff.