Page MenuHomeFreeBSD

atomic: Make atomic_san.h self-contained
ClosedPublic

Authored by markj on Jan 4 2023, 1:01 AM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 20 2023, 8:14 AM
Unknown Object (File)
Oct 29 2023, 4:18 AM
Unknown Object (File)
Apr 8 2023, 8:45 PM
Unknown Object (File)
Mar 23 2023, 6:08 AM
Unknown Object (File)
Feb 9 2023, 3:06 AM
Unknown Object (File)
Jan 10 2023, 7:21 PM
Unknown Object (File)
Jan 4 2023, 3:46 PM
Subscribers

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj requested review of this revision.Jan 4 2023, 1:01 AM

\\\\\\\\\\\\\\\\\\

sys/sys/atomic_san.h
43

We always required sys/types.h before machine/atomic.h, and this header claims that machine/atomic.h is the pre-requisite. This is the reason why I did not proposed to include sys/types.h into sys/atomic_san.h

IMO it is better to fix sys/systm.h to follow machine/atomic.h requirements (my #3 in the mail)

46

sys/cdefs.h inclusion is definitely redundand

sys/sys/atomic_san.h
43

In general we seem to be preferring self-contained headers. I cannot see why it is preferable to force consumers to include sys/types.h when the dependency is unconditional.

For your #3, shall we simply move the existing sys/param.h include in sys/systm.h?

46

If we can assume that sys/types.h includes sys/cdefs.h, then the cdefs include should be removed from atomic_common.h as well.

sys/sys/atomic_san.h
43

We prefer self-contained headers, but this header is not supposed to be used. It is internal helper for machine/atomic.h. This is why I do not see much sense in making it more namespace-polluting than the header that utilizes it.

I still prefer my #3 as an addition of sys/types.h to sys/systm.h instead of sys/param.h. There are user-mode bits in sys/systm.h.

46

sys/cdefs.h is mostly magic, it should not be included in random headers (IMO).

sys/sys/atomic_san.h
43

Then machine/atomic.h should include sys/types.h. But this still makes less sense to me, since it's atomic_common.h/atomic_san.h that actually need it.

There are user-mode bits in sys/systm.h.

Not really? There is a forward declaration of struct ucred for some reason, and some macros (__read_mostly etc.) which do not accomplish much in usermode without extra linker directives. In particular, I can't see why systm.h should include machine/atomic.h for userland.

Nonetheless, here is a systm.h patch: https://reviews.freebsd.org/D37943

kib added inline comments.
sys/sys/atomic_san.h
43

As I said, I prefer D37943, but would not scream.

This revision is now accepted and ready to land.Jan 4 2023, 3:21 PM
This revision was automatically updated to reflect the committed changes.

so, I like this w/o the sys/cdefs.h.
We're including the pre-reqs at the level they are used, so either here or possibly machine/atomic.h (the next layer up) is the right place.
systm.h is fixing it at the wrong level.

We're moving towards self contained files, where possible. Where that's not possible or desirable, we should included at the lowest level that makes sense.

systm.h is absolutely the wrong place, (sorry kib) imho. It move knowledge of how atomic.h is implemented there, and should we ever have to change it, we'll need to change systm.h. I'm having a lot of trouble understanding why that's the right fix.

(ah, between the time I saw this on my phone and when I got a chance to comment, it was committed)