From nobody Tue Aug 01 06:05:19 2023 X-Original-To: freebsd-current@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4RFPk3068Pz4prVj for ; Tue, 1 Aug 2023 06:05:47 +0000 (UTC) (envelope-from theraven@freebsd.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4RFPjT6hfNz4Nsj; Tue, 1 Aug 2023 06:05:33 +0000 (UTC) (envelope-from theraven@freebsd.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1690869934; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=CBUhqarDV5UFS2xfJTCgRo6eoEXQ720J3E7d9+e1Q/8=; b=ul0VotGWbddJzpfobzOdkouk0tvr9+m8AKPlf48z/i/fUxFGJS6xfwleJNOOcbtD+jM/U4 j0LMHbup9qAlGYtVjkMahzMg+GqBGnlb9NuVtcwX965RT+E67Nk1RvorOo2BZ+MZcgWN/r sRrAWj3kiR5Pc0d+DtIj3KsG4GbIOWPR2WZrOAwVTiVlA5TkcTajOMva26gdpkKybaIiEf kICXO4jFoMhiIW2dFHvn3vsd+Wr7T+G2BvZxSfoXz6aQnIMv7YeWRgM5XAPA/84S2CpDnM xMYKn8BA1SqZon6YakfhP/jALmJ5JRYYrT0AiNCQSiurlJB7C95Sc9ddeGuohQ== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1690869934; a=rsa-sha256; cv=none; b=KHAWrRF2b7TUzrdnNxk+QRwxMkfCePy9v4boUA0Vy24ArEJwHvomFSPefyKkfobv6MRU6r CsBMKzm5XFrQGbaaUtKY4i+mp1V8wpUPJxmjH8FCwCcm05pUke5M/R82chJfXI5Pol8Nlb CbUH6/uRfCmj8VshdQlO2adOzgPQpEZVm+f+J/9ct3CfIy3EO6v3mNBrEtvJYdKKgSSklA 0ueaP3DtzKfI0U5jogfSnL265Bfm7DpIymrqb6VRTlBeOpDfw2mDGGEl1TdRn7qlH1Y1dy ZeNki6CTC37FKvneTD7DFPl9fdjjwXD/oxCViQtrpVlB5l3vDVnWKJtL4YygxA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1690869934; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=CBUhqarDV5UFS2xfJTCgRo6eoEXQ720J3E7d9+e1Q/8=; b=ZmS6RF+ZfbfSnwo03aaMebea0nWL/mQJrmRqR0FdKwcq6Wrpks/4GO7dAGYPveLo7qzCkD 0BqZIWB4jNGrAxH64mtSrILKdwfJhvkDMQ2DdlSN6ON7u/AvmhF4Ceb8FqzhFSdkWBAWLR P3ntrWaYmeIiWWy+piZa9lNckuXKquTYWe8EI10O6ly87M8Lh1Bwm8MEfITM9v2MsJCNCU dN6gRJ5Rzj6FJGykoOyBDJ3rJvAWUNBUxd175benjCjgWsR2E2VBz52NWMzT+6RqXuy0j7 1l5LpeLztI0yXiC5D1UPaY9i7D25woEZZXBsYm5yIHQzHqvAIToe/epu9et2WA== Received: from smtp.theravensnest.org (smtp.theravensnest.org [45.77.103.195]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: theraven) by smtp.freebsd.org (Postfix) with ESMTPSA id 4RFPjT2FKwzcG9; Tue, 1 Aug 2023 06:05:33 +0000 (UTC) (envelope-from theraven@freebsd.org) Received: from smtpclient.apple (host86-141-212-56.range86-141.btcentralplus.com [86.141.212.56]) by smtp.theravensnest.org (Postfix) with ESMTPSA id 2589A8869; Tue, 1 Aug 2023 07:05:31 +0100 (BST) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable From: David Chisnall List-Id: Discussions about the use of FreeBSD-current List-Archive: https://lists.freebsd.org/archives/freebsd-current List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-current@freebsd.org Mime-Version: 1.0 (1.0) Subject: Re: Review of patch that uses "volatile sig_atomic_t" Date: Tue, 1 Aug 2023 07:05:19 +0100 Message-Id: References: Cc: FreeBSD CURRENT In-Reply-To: To: Rick Macklem X-Mailer: iPad Mail (20F75) 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 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