On Wed, Aug 2, 2023 at 6:14 PM Konstantin Belousov <kostik...@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 <thera...@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.mack...@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. > > > > > > >
