On Mon, Jul 31, 2023 at 11:33 PM David Chisnall <[email protected]> wrote:
>
> Hi,
>
> This bit of the C spec is a bit of a mess. There was, I believe, a desire to
> return volatile to its original use and make any use of volatile other than
> MMIO discouraged. This broke too much legacy code and so now it’s a confusing
> state.
>
> The requirements for volatile are that the compiler must not elide loads or
> stores and may not narrow them (I am not sure if it’s allowed to widen them).
> Loads and stores to a volatile variable may not be reordered with respect to
> other loads or stores to the same object but *may* be reordered with respect
> to any other accesses.
>
> The sig_atomic_t typedef just indicates an integer type that can be loaded
> and stored with a single instruction and so is immune to tearing if updated
> from a signal handler. There is no requirement to use this from signal
> handlers in preference to int on FreeBSD (whether other types work is
> implementation defined and int works on all supported architectures for us).
>
> The weak ordering guarantees for volatile mean that any code using volatile
> for detecting whether a signal has fired is probably wrong if if does not
> include a call to automic_signal_fence(). This guarantees that the compiler
> will not reorder the load of the volatile with respect to other accesses. In
> practice, compilers tend to be fairly conservative about reordering volatile
> accesses and so it probably won’t break until you upgrade your compiler in a
> few years time.
>
> My general recommendation is to use _Atomic(int) (or ideally a enum type) for
> this. If you just use it like a normal int, you will get sequentially
> consistent atomics. On a weakly ordered platform like Arm this will include
> some more atomic barrier instructions but it will do the right thing if you
> add additional threads monitoring the same variable later. In something like
> mountd, the extra performance overhead from the barriers is unlikely to be
> measurable, if it is then you can weaken the atomicity (sequentially
> consistent unless specified otherwise is a good default in C/C++, for once
> prioritising correctness over performance).
Just trying to understand what you are suggesting...
1 - Declare the variable _Atomic(int) OR atomic_int (is there a preference) and
not volatile.
2 - Is there a need for signal_atomic_fence(memory_order_acquire); before the
assignment of the variable in the signal handler. (This exists in
one place in
the source tree (bin/dd/misc,c), although for this example,
neither volatile nor
_Atomic() are used for the variable's declaration.
3 - Is there any need for other atomic_XXX() calls where the variable is used
outside of the signal handler?
In general, it is looking like FreeBSD needs to have a standard way of dealing
with this and there will be assorted places that need to be fixed?
Thanks, rick
>
> David
>
> > On 1 Aug 2023, at 06:14, Rick Macklem <[email protected]> wrote:
> >
> > Hi,
> >
> > I just put D41265 up on phabricator. It is a trivial
> > change to mountd.c that defines the variable set
> > by got_sighup() (the SIGHUP handler) as
> > static volatile sig_atomic_t
> > instead of
> > static int
> >
> > I did list a couple of reviewers, but if you are familiar
> > with this C requirement, please take a look at it and
> > review it.
> >
> > Thanks, rick
> > ps: I was unaware of this C requirement until Peter Eriksson
> > reported it to me yesterday. Several of the other NFS
> > related daemons probably need to same fix, which I will
> > do after this is reviewed.
> >
