Re: Review of patch that uses "volatile sig_atomic_t"

On Wed, Aug 2, 2023 at 6:14 PM Konstantin Belousov <kostikbel@gmail.com> wrote:
>
> On Tue, Aug 01, 2023 at 04:33:16PM -0700, Rick Macklem wrote:
> > 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.
> Really does not matter.
>
> > 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.
> In mountd, there are two signal handlers.  One for SIGHUP, and another for
> SIGTERM.  They are very different, let me explain.
>
> For SIGHUP, the only action in the signal handler is the assignment to
> the variable.  The assignment itself is atomic on FreeBSD.  But what is more
> important, there is no ordering issues between this assignment and any other
> action.  Eventually the main execution context notices that the variable is
> set and does something (re-read config).  As consequence, there is no need
> in any fencing of the SIGHUP handler.

The only concern would be that the setting of "got_sighup" would get lost due
to some compiler optimization and it sounds like _Atomic(int) is sufficient to
guarantee that will not happen.

rick

>
> > 3 - Is there any need for other atomic_XXX() calls where the variable is used
> >      outside of the signal handler?
>
> The SIGTERM is different, it does some rpc bind calls to unregister itself
> as a mount program.  If these actions can interfere with the main context
> execution (I believe they are), then userspace rpc bind client state can
> be corrupted, resulting in failing attempt to unregister.
>
> No amount of atomics or fencing would fix it, the unregister action should
> be either moved out of the signal handler context to main context.  Another
> option might be to block SIGTERM, and only unblock it in places where it
> is safe to do rpcb calls from interrupt.  This is approximately what dd(1)
> does.
Yes, I think moving the termination into the main context should work.
I'll do that as a separate commit/review.

I suspect that failing to de-register doesn't cause too much of a problem.
since the client won't be able to connect to the port# after mountd has
terminated and will fail (just maybe not a gracefully).  Same would happen
if mountd crashes for some reason (and not terminated via SIGTERM).

rick

>
> >
> > 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.
> > > >
> >
>