Date: Tue, 1 Aug 2023 16:33:16 -0700 From: Rick Macklem <rick.macklem@gmail.com> To: David Chisnall <theraven@freebsd.org> Cc: FreeBSD CURRENT <freebsd-current@freebsd.org> Subject: Re: Review of patch that uses "volatile sig_atomic_t" Message-ID: <CAM5tNy7jNQ2NB91V96vgMQ8AeZSVp5rHMZcZ7PG0WPQyBEJ6XQ@mail.gmail.com> In-Reply-To: <A5E9074A-5D63-4351-9C0B-D7990E12AC9E@freebsd.org> References: <CAM5tNy5aAyRk_CML57Q7OEPXGVEjM=o8fqWdLJCRkHubBYzCNQ@mail.gmail.com> <A5E9074A-5D63-4351-9C0B-D7990E12AC9E@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Jul 31, 2023 at 11:33=E2=80=AFPM David Chisnall <theraven@freebsd.o= rg> 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=E2=80= =99s 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=E2=80=99s 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 reord= ered with respect to any other accesses. > > The sig_atomic_t typedef just indicates an integer type that can be loade= d and stored with a single instruction and so is immune to tearing if updat= ed from a signal handler. There is no requirement to use this from signal h= andlers in preference to int on FreeBSD (whether other types work is implem= entation defined and int works on all supported architectures for us). > > The weak ordering guarantees for volatile mean that any code using volati= le for detecting whether a signal has fired is probably wrong if if does no= t include a call to automic_signal_fence(). This guarantees that the compil= er will not reorder the load of the volatile with respect to other accesses= . In practice, compilers tend to be fairly conservative about reordering vo= latile accesses and so it probably won=E2=80=99t break until you upgrade yo= ur 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 yo= u add additional threads monitoring the same variable later. In something l= ike mountd, the extra performance overhead from the barriers is unlikely to= be measurable, if it is then you can weaken the atomicity (sequentially co= nsistent unless specified otherwise is a good default in C/C++, for once pr= ioritising 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 t= he 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 us= ed outside of the signal handler? In general, it is looking like FreeBSD needs to have a standard way of deal= ing 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: > > > > =EF=BB=BFHi, > > > > 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. > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAM5tNy7jNQ2NB91V96vgMQ8AeZSVp5rHMZcZ7PG0WPQyBEJ6XQ>