Date: Tue, 1 Aug 2023 07:05:19 +0100 From: David Chisnall <theraven@freebsd.org> To: Rick Macklem <rick.macklem@gmail.com> Cc: FreeBSD CURRENT <freebsd-current@freebsd.org> Subject: Re: Review of patch that uses "volatile sig_atomic_t" Message-ID: <A5E9074A-5D63-4351-9C0B-D7990E12AC9E@freebsd.org> In-Reply-To: <CAM5tNy5aAyRk_CML57Q7OEPXGVEjM=o8fqWdLJCRkHubBYzCNQ@mail.gmail.com> References: <CAM5tNy5aAyRk_CML57Q7OEPXGVEjM=o8fqWdLJCRkHubBYzCNQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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 s= tores and may not narrow them (I am not sure if it=E2=80=99s allowed to wide= n them). Loads and stores to a volatile variable may not be reordered with r= espect to other loads or stores to the same object but *may* be reordered wi= th respect to any other accesses. The sig_atomic_t typedef just indicates an integer type that can be loaded a= nd stored with a single instruction and so is immune to tearing if updated f= rom a signal handler. There is no requirement to use this from signal handle= rs in preference to int on FreeBSD (whether other types work is implementati= on defined and int works on all supported architectures for us). The weak ordering guarantees for volatile mean that any code using volatile f= or detecting whether a signal has fired is probably wrong if if does not inc= lude a call to automic_signal_fence(). This guarantees that the compiler wil= l not reorder the load of the volatile with respect to other accesses. In pr= actice, compilers tend to be fairly conservative about reordering volatile a= ccesses and so it probably won=E2=80=99t break until you upgrade your compil= er in a few years time. My general recommendation is to use _Atomic(int) (or ideally a enum type) fo= r this. If you just use it like a normal int, you will get sequentially cons= istent 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 a= dditional threads monitoring the same variable later. In something like moun= td, the extra performance overhead from the barriers is unlikely to be measu= rable, if it is then you can weaken the atomicity (sequentially consistent u= nless specified otherwise is a good default in C/C++, for once prioritising c= orrectness over performance). David > On 1 Aug 2023, at 06:14, Rick Macklem <rick.macklem@gmail.com> wrote: >=20 > =EF=BB=BFHi, >=20 > 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 >=20 > 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. >=20 > 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. >=20
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?A5E9074A-5D63-4351-9C0B-D7990E12AC9E>