From nobody Wed Aug 02 22:51:08 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 4RGRzz1VGQz4d7kv for ; Wed, 2 Aug 2023 22:51:30 +0000 (UTC) (envelope-from pen@lysator.liu.se) Received: from mail.lysator.liu.se (mail.lysator.liu.se [IPv6:2001:6b0:17:f0a0::3]) (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) by mx1.freebsd.org (Postfix) with ESMTPS id 4RGRzj3f0fz3m15; Wed, 2 Aug 2023 22:51:29 +0000 (UTC) (envelope-from pen@lysator.liu.se) Authentication-Results: mx1.freebsd.org; none Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 44D45141D3; Thu, 3 Aug 2023 00:51:20 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 4332514367; Thu, 3 Aug 2023 00:51:20 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,AWL,HTML_MESSAGE, T_SCC_BODY_TEXT_LINE autolearn=disabled version=3.4.6 X-Spam-Score: -1.0 Received: from smtpclient.apple (unknown [IPv6:2001:9b1:28fa:7900:50ae:3785:3e33:f653]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id F052D14824; Thu, 3 Aug 2023 00:51:18 +0200 (CEST) From: Peter Eriksson Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_58B5A35C-7465-46C1-B6E9-025C34A8A2FE" 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 (Mac OS X Mail 16.0 \(3731.600.7\)) Subject: Re: Review of patch that uses "volatile sig_atomic_t" Date: Thu, 3 Aug 2023 00:51:08 +0200 In-Reply-To: <9A5C0082-2FFD-4B2B-9381-7ACD9EB7C4DF@FreeBSD.org> Cc: Rick Macklem , FreeBSD CURRENT To: David Chisnall References: <9A5C0082-2FFD-4B2B-9381-7ACD9EB7C4DF@FreeBSD.org> X-Mailer: Apple Mail (2.3731.600.7) X-Virus-Scanned: ClamAV using ClamSMTP X-Rspamd-Queue-Id: 4RGRzj3f0fz3m15 X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:1653, ipnet:2001:6b0::/32, country:EU]; TAGGED_RCPT(0.00)[] --Apple-Mail=_58B5A35C-7465-46C1-B6E9-025C34A8A2FE Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Interesting discussion regarding sig_atomic_t, volatile & stuff. It = seems I opened a can of worms. Sorry :-) Anyway here are the references I had read when suggesting this change: C89: - http://port70.net/~nsz/c/c99/n1256.html#7.14p2 CERT C Coding Standard: - = https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+share= d+objects+in+signal+handlers I=E2=80=99ve also been using "static int foo;=E2=80=9D in my own code = forever before and like you say it works but then I got curious and = started searching to see if there was a more =E2=80=9Ccorrect=E2=80=9D = (and portable) way of doing this) If I read you right then just dropping =E2=80=9Cvolatile=E2=80=9D and = use =E2=80=9Csig_atomic_t=E2=80=9D should be better (and =E2=80=9Cportable= =E2=80=9D for some value of portable, not a problem in this case with = mountd) but using atomic_int from stdatomic.h from C11 is even better? = (volatile sig_atomic_t is from C89 I think). Something like this: > #if __STDC_NO_ATOMICS__ !=3D 1 > /* C11 */ > #include > static atomic_int got_sighup =3D ATOMIC_VAR_INIT(0); >=20 > #else > /* C89 */ > static volatile sig_atomic_t got_sighup =3D 0; > #endif (for portability, not needed for mountd though).=20 https://www.informit.com/articles/article.aspx?p=3D2204014=EF=BF=BC Ah well, time to relearn C again :-) (K&R C was simpler :-) - Peter > On 2 Aug 2023, at 10:12, David Chisnall wrote: >=20 > On 2 Aug 2023, at 00:33, Rick Macklem wrote: >>=20 >> Just trying to understand what you are suggesting... >> 1 - Declare the variable _Atomic(int) OR atomic_int (is there a = preference) and >> not volatile. >=20 > Either is fine (the latter is a typedef for the former). I am not a = huge fan of the typedefs, some people like them, as far as I can tell = it=E2=80=99s purely personal preference. >=20 >> 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. >=20 > You don=E2=80=99t need a fence if you use an atomic variable. The = fence prevents the compiler reordering things across it, using atomic = operations also prevents this. You might be able to use a fence and not = use an atomic but I=E2=80=99d have to reread the spec very carefully to = convince myself that this didn=E2=80=99t trigger undefined behaviour. >=20 >> 3 - Is there any need for other atomic_XXX() calls where the variable = is used >> outside of the signal handler? >=20 > No. By default, _Atomic variables use sequentially consistent = semantics. You need to use the `atomic_` functions only for explicit = memory orderings, which you might want to do for optimisation (very = unlikely in this case). Reading it outside the signal handler is the = equivalent of doing `atomic_load` with a sequentially consistent memory = order. This is a stronger guarantee than you need, but it=E2=80=99s = unlikely to cause performance problems if you=E2=80=99re doing more than = a few dozen instructions worth of work between checks. >=20 >> 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? >=20 > If we used a language that let you build abstractions, that would be = easy (I have a C++ class that provides a static epoch counter that=E2=80=99= s incremented in a signal handler and a local copy for each instance, so = you can check if the signal handler has fired since it was last used. = It=E2=80=99s trivial to reuse in C++ projects but C doesn=E2=80=99t give = you tools for doing this. >=20 > David >=20 >=20 --Apple-Mail=_58B5A35C-7465-46C1-B6E9-025C34A8A2FE Content-Type: multipart/related; type="text/html"; boundary="Apple-Mail=_CE0F325C-78E2-41AE-B8CF-3F116913547F" --Apple-Mail=_CE0F325C-78E2-41AE-B8CF-3F116913547F Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Interesting = discussion regarding sig_atomic_t, volatile & stuff. It seems I = opened a can of worms. Sorry :-)


Anyway here are the references I had read = when suggesting this change:

C89:
- = http://port70.net/~nsz/c/c99/n1256.html#7.14p2

CERT C Coding = Standard:
- = https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+share= d+objects+in+signal+handlers


I=E2=80=99ve also been using "static int = foo;=E2=80=9D in my own code forever before and like you say it works = but then I got curious and started searching to see if there was a more = =E2=80=9Ccorrect=E2=80=9D (and portable) way of doing this)


If I read you = right then just dropping =E2=80=9Cvolatile=E2=80=9D and use = =E2=80=9Csig_atomic_t=E2=80=9D should be better (and =E2=80=9Cportable=E2=80= =9D for some value of portable, not a problem in this case with mountd) = but using atomic_int from stdatomic.h from C11 is even better? (volatile = sig_atomic_t is from C89 I think). Something like this:

#if __STDC_NO_ATOMICS__ = !=3D 1
/* C11 */
#include = <stdatomic.h>
static atomic_int got_sighup =3D = ATOMIC_VAR_INIT(0);

#else
/* C89 = */
static volatile sig_atomic_t got_sighup =3D = 0;
#endif

(for = portability, not needed for mountd = though). 

3D"apple-touch-icon-144x144-precomposed.png"


Ah well, time to relearn C again :-)

(K&R C was = simpler :-)

- Peter

On 2 = Aug 2023, at 10:12, David Chisnall <theraven@FreeBSD.org> = wrote:

On 2 Aug = 2023, at 00:33, Rick Macklem <rick.macklem@gmail.com> = wrote:

Just trying to understand what = you are suggesting...
1 - Declare the variable _Atomic(int) OR = atomic_int (is there a preference) and
   not = volatile.

Either is fine (the latter is a typedef = for the former).  I am not a huge fan of the typedefs, some people = like them, as far as I can tell it=E2=80=99s purely personal = preference.

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.

You don=E2=80=99t need a fence if you = use an atomic variable.  The fence prevents the compiler reordering = things across it, using atomic operations also prevents this.  You = might be able to use a fence and not use an atomic but I=E2=80=99d have = to reread the spec very carefully to convince myself that this didn=E2=80=99= t trigger undefined behaviour.

3 - Is = there any need for other atomic_XXX() calls where the variable is = used
   outside of the signal = handler?

No.  By default, _Atomic variables use = sequentially consistent semantics.  You need to use the `atomic_` = functions only for explicit memory orderings, which you might want to do = for optimisation (very unlikely in this case).  Reading it outside = the signal handler is the equivalent of doing `atomic_load` with a = sequentially consistent memory order.  This is a stronger guarantee = than you need, but it=E2=80=99s unlikely to cause performance problems = if you=E2=80=99re doing more than a few dozen instructions worth of work = between checks.

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?

If we used a language that let you build = abstractions, that would be easy (I have a C++ class that provides a = static epoch counter that=E2=80=99s incremented in a signal handler and = a local copy for each instance, so you can check if the signal handler = has fired since it was last used.  It=E2=80=99s trivial to reuse in = C++ projects but C doesn=E2=80=99t give you tools for doing = this.

David



= --Apple-Mail=_CE0F325C-78E2-41AE-B8CF-3F116913547F Content-Transfer-Encoding: base64 Content-Disposition: inline; filename=apple-touch-icon-144x144-precomposed.png Content-Type: image/png; x-unix-mode=0666; name="apple-touch-icon-144x144-precomposed.png" Content-Id: <770B6173-1659-4CD9-814E-194843E87775> iVBORw0KGgoAAAANSUhEUgAAAJAAAACQCAYAAADnRuK4AAANpUlEQVR4Xu2cDUzU9xnHnzvugDtB 3pUiCFIVLFiYqDPQuio6ieq0aW3W2mRxc6VJ2iXd2m623Zotje3SvWapk6QzWdpsS7vOGq3vSn0l KiCqBVVUAUUU8DjejvMA9v0lf/Hy965y8ibH95P88rtySvhxX7/P83ue519DT0+PDAf5+fkh2FKw krAisEKxjNJXSAOWDasS61JBQUGHDAMmGTjMWBYsa+9rNw4cOBBXUVGRZTAYvg/RTk5LS4sAPVih VqvVHBgYKH2HtLe3S0dHh6u+vr791q1bAevXr78VFBR0Gl87mpWVVThz5sx6k8nk7g4OLBdWu9vr ftMvB+ru7jZiC8MaixUkOvbv3z/h4sWLuU6nczkOF5uUlBQN5JFHHpHIyEgxm80yMJCGhgaprq6W 2tpa+eabbzosFks11hdPPvnkF1OnTm2Re3FiNWPZjUZj95AKqLOz04wtHCtMH3ba2toCtm/f/nhz c/Mr+MFSH3300ejJkycr0UhAQIAMDUSJ6ejRo3Lp0qWOMWPGVOD3/96KFSvOePIBLDtWExzLNegC gptEYIvylK989tlns1tbW1+HUFKzs7ODUlJSBDYqwwfBP3Y5d+6cFBcXdyASlIeHh78PIZ3yIqRG RArboAiovd1hxhbnKVSdOFEaXVZWts7lcuUsWLDAhPxGHi4IciM5e/aslJeXOxAlCpOTkz/Mzs6p 9RLaaq1Wi2vABARXCcEWq3cdm81m3rFjx0oI52cZGRkRSNwe8ryGOBwOKS0tlZKSkhbkSH9dtWrV J17cqC4kJKS13wKy2+1jlXhEBxwnGsnaH8LCwmbl5uaaxo0bJyMHgs9VNm3a1IWocWj69Om/RNTw FLrq8Pk2P7CA4DAexYMkOR3J2ScLFy60KtcZmRDkRHLkyBHBTbkKpZQ3ly5dWuZJRCi1NPssoMbG xhBscaJjy5Yty6Dad+bPnx+Oa7mMfEhlZaXs3LmzFSJ6/+mnn/6v3EttVFRUa58FdPPmTTO2RCyj 7pa1BtsryOIt6lruP5Dr16/Ltm3b2pDDvrtkyZItHnKiKqQprj4JqK6uToknSOc8zyAB+/WaNWss /lk1JvjcZe/evc0IbR8sXrz4f7pKtjM2NrbqvgJCJTMCW4xodHV1GbZu3fpDJFOvIWyFqUqyf8Pk esOGDZ2IMG/m5eV9pXu7Pi4uzuZVQDU1V/WhC7FxR9aNGzf+CecxIw6K/0NgIrJv375GFCFfXbQo r0QfyhIS4l0eBXTlSpVyngjRKCo6kowey8fIeSbAvmR0wVYIcl7788+/MFt/OU9KSqzXC0j1TIzY kt3dZ/Pmzf+eM2fOjNF5VSeFhYVy+vTpsieeeOJHMTExHW4udAmVbLWLya2z7t4YVRa2Bq4zHRVm GZ2QefPmydWrV9NPnTr1Al5vFADuTGDYBJjckuWxooHmWzQc6Y21a9eqZqgolxqdENVl2LVr1+pj x47txZxRlQCgtGLrDWEY9DKLyCTRQFHp77NmzZqPJYSoajVcaBc6D6/KXS5PmzbNZdLcxyIaBw8e zITr5CDvGVDnKXt8aENhCH7+yRv/IZ6o/PFPpLW4WEYjmadOiq+oNObChQtzMVWanpOTc0YAsGC5 TFr+Y3Vr+7+Nek+QaBCCFoeg2RpcVFT0OxQZn0W1ulsbXW6+IyCz5j5ZqPVMUROE7hCSmZkphw4d SsEYyJTZs2ef0+bexT2ECdT1OuKaBROFfpA493g/A94bpTzw54rxZEH4MkFAr0MvP9VCmBixIKBu TBWemIQeV5IaQ/UEIenp6SrF+c6lS5cjlGYURi2EYeqwbfGUKVMivTVKCcG8tOAhiVC0tp5RmlFo OVAXlOX4waRJk/ym5oNjeD0L3mMIe0AQoVTXfgE087EAo1Y4jEWMi4yPj5dvg5CEhASVKydhijFa gFFrnOWitzH2fs9tEaJSnMTExAi0ODIFmLQvzse0mbI32jVD2H1BYxWjPzV5IrLeqD1nndHX2g8h ymwMBkOqAGN+fn4AhuSD8cSiENJXB8ItLBTaCVYh7HFUn52wNjPtui8whAUHB6u0J0hExqsQlggB BYsPEIIZ+RBsCcqBJiJ8mfwx3xy8M/F3YrFYzNiilYBCUWH0w/6QOg97YYN1drM5UECoCmGdBgM2 nyCsB5kFhCoHCu7sdPmUXLGVwYuF6s6DwH5ZDyHG/qiSENODWBvrQAxh0MsAOhChA42mOtDEDRuk v1S//LK0l5bKUJJ67Niw14H0ejG5fYk1Dx/geXwIYYQwhPE8g5KMm/zb7nv8LJz0sA5EWAcaSa0M PwonA36e4a8DEWIUv4MwhLGVwRBGGMIIYQgbDhjCCKED9eBMg1+JpgMRwlYGYQhjK4NJNGEdiBCG sIf9TAxhhHCklefhUxl8KoNPZRAm0WxlMImWB4IQo//lCoQhjK0MhjDCEEYIWxkPPwxhhA7Uw1aG r7CVwVYGz8NWBmESzVYGn8ogdCCDwSD+hWGQz2SQoeJh+oz0ejHqxEMIHWgIz0QH6rcdEzqQweCv IjLwPIOGQR/CxA9DGM8zuD8Lk2jCJNq7++BMfuAGvMYPHoQOxEIiHYjQgXiN7xe8xht4jfcdXuNZ SPQdnoeFRJ6HhUTCazwLibzGE2IUQhjCWIlmCCN0II6A0oEIoQPRgThUTzhUz94Ru/EjvhI9lH0p duP90H0GuQ5ER2U3nufpfxJt8vV/G0JId3e3gNvKgTpMJhOv8T7B89y+fVtAi3Igu9N5W3yBEJfL JaBFOVBVU5OtE/o28RrfV3geh8OhFNSgRHPl1q1bHXDHECbRfYXnsdvtrdhqjAUFBacbGxuDmEgT H9znjgPdMAkIDg5ugYgiY2JiONJ6XzjSCq0ozTTCfDqMAsaMGXOiqqpK+gIhdXV10tnZWdlbSGxv b9+NPCi3LwrnRCInEq9fv640s7m3Gz9hwoTDFy5csENVQsj96j/Xrl2zxcfHn+p1oKSkpJtNTU03 b9y4EYY3WHjzCs9TU1Oj8p8rcXFx9l4HCggIELPZ/FV5ebl8G4ScO3dOtTH2KM30CshoDBCrdcyW mpqrDeqK5g3C8HXx4sWWqKioL5RmFKY7DjRt2rSrx48fu3D+/PnozMxMWv498DxnzpxR4evExImJ NgHuIcyBhd30F/whh+ggpKurSwoLC7vQeP+T0gqWw92BXNgsc+bMKTty5PB5kJGSksJCYi8sJB49 elTCwsJLZs6cVSEAuNwEZGzHNlYAkul1+/bt+xQCMgsgpKWlRSXPTqvV8htoRTTa3R3IIRpZWVmn i4qKjhw4cOB7gIVEFhLl+PHjKoTtmTFjxmW5i6M3B8IbLojIiaXaGl2JiYlvFRcXt6M2JKMbUltb q1ZdZGTkH5U+tOWEZu6GMM2FmrHFCEhNTW1Aw+xve/fu/fmKFSvMSlijE7Jjx46uwMDAgoyMjGty l2YBegHZsUXdcaW5c+duxF+eV1paOhuMsmsvz6Nmnvfs2SMdHR3FSGs+dzORbiy7aBjc54CQKCkH ihCNkpKSRDjRxkWLFsWj3SGjC1act2/f3rB8+fIc3Vs2XLDqdQ7U60JN2MLuuBCcpwoq/NWmTZs+ XrVqVfD48ePF/yFqtAc5cGN0dPRLuvSlG6tJgEcHUly+fDlCy4V6QS60BOL6bV5eXii+qfgvxGaz yUcffdQ9derUl7Kzsw/q3q6fNGmSzbuANKqrqxOxBYkbO3fuXI5v/t7q1asDIyIixP8gqtP+9ddf tyD/eXfhwoVf6d52Tpw4scrrk6m6UFaLTYnIKBoLFizYsnv3btPWrVvXPvXUU6EJCQniX/C6js/W ERoa+jYizU4ButBVKzr0DqQfWwzBFidA50SLGhoafr9s2TJLcnKyjHzI2bNnZdeuXe3IcX+Rm5u7 z5O+YmNjW30SkKK+vn4stljRsX///uQrV658imJSVE5OjqDBJiMPgrFUOXz4sApdl1FAfg1hq8KT l+Bhi2YBPgtIgVlpjyI6efJkNJqu60JCQnIQ0kxQqIwsGLJwOerCGHNhWlraW7ia2+VeVAW6WYCv AtI/RBaiicgoOr788stlaLa9g3AWjjxpBLgRXefQoUOCGfgmzPb8+bnnnvuPAA85T11YWFirgH4L SNHa2mrWcqIgD7c2K5qvH2CaMVe5Ecre8vDBhwHVyHJFRYUDjdFtGCD8MD093Sb34sSqRWRxCRgA AekV7IjobXm44XQ6jWh9pNvtTWuNRmMaRBT02GOPCUKcDCccQ3VJWdkJlSh3YCT1VHh4xLolS5ZU eHGdRoxs6EQ1wALSxGLGFu5WtdY34CYjAX8DTbhUxNBYFKUEHX7BfwsZuvFT1Y6orKzsQCgqGzdu 3AdLly71Jhw7VlNQUJBLwKAJSA8SMKMmorGeQht6adGwzKfQkHsWg2oJOES0SrZxXVRrAAXFYXc8 kqVSCfXMlhKPQDDlqOn868UXX/zcmw9oXXU78tZu0RgyAemBEM3YLFhWrDuv3W9tERhM+i46yt+H aFOQzEWjYKlub0FWq9UM9StBCYQmiNHiHeYyqivf1tamEuJOOL0TOY0Lvzc70oViiOngypUrd8N1 OvV/VRtDbVev8T10btMPAQ0X+fn5cdjSsKI0JwvVDmkWb5BOzT3qsGxYZ7HqCwoKumQY+D+tDWKW GnBGhgAAAABJRU5ErkJggg== --Apple-Mail=_CE0F325C-78E2-41AE-B8CF-3F116913547F-- --Apple-Mail=_58B5A35C-7465-46C1-B6E9-025C34A8A2FE--