Re: Review of patch that uses "volatile sig_atomic_t"

On Mon, Jul 31, 2023 at 11:33 PM David Chisnall <theraven@freebsd.org> 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 <rick.macklem@gmail.com> 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.
> >