From nobody Tue Aug 1 23:33:16 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 4RFryt6QRLz4q11W for ; Tue, 1 Aug 2023 23:33:30 +0000 (UTC) (envelope-from rick.macklem@gmail.com) Received: from mail-oo1-xc2a.google.com (mail-oo1-xc2a.google.com [IPv6:2607:f8b0:4864:20::c2a]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4RFryd6zJzz3mJ6; Tue, 1 Aug 2023 23:33:29 +0000 (UTC) (envelope-from rick.macklem@gmail.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-oo1-xc2a.google.com with SMTP id 006d021491bc7-56ca4d7079aso2312666eaf.0; Tue, 01 Aug 2023 16:33:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1690932808; x=1691537608; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=+Gs09JfHRKH+QEve/edjtuZNHhDcggtZKq4pvyRrx4g=; b=bYvaCsuS9dRUpSHSY6HFwY/nikgmWHTr9JuIMMxUFpn0N2KuRgdT2d2y5lUdMdyfk1 xNPdvwOY4RkVPs1XIyJjTETvXbffpSDMZdio0KQFMvzsX3/3fxSAaAAHAnN1KRy9bNwL NeSQWlVQ2QcjG2zPFd9k8E0tXZ+dmDeUkofdzD61Ooc5o92KNg5Q5Q04X3H9ZRSyN6Ug qQxuRjw1TyuYdfWb1TnIV27r11edA4IQkN1Uxgxbj1JTKBDW5vF2BvZ1vCri0KxvhB56 zeB53rE281CMoJ1MPLGj/y11hTXJRB2xIpsphV2bHpJnokvBz4+1Wka8zBJ07JUgSfb6 cn4w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690932808; x=1691537608; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=+Gs09JfHRKH+QEve/edjtuZNHhDcggtZKq4pvyRrx4g=; b=PHzH0x6UJysHyMRdePxDNxeaq/hReYkw1pl14xsR3NO/RpyCpWirC5OKVBMcKo8mfD 3JDKBGzp2Uh3dq8cxjc8v8AcI/5qi9AEBlJjO2peqmPlOTbZ0ihtKXHp3fDWvMehkwsp /Mkbl6b6PJ9ieCmtcw9k2DLTpY3vc/Sh+V2v0LkJP0YtzWQcgVpnvwk5chzJi1lEPLIB 4vKDrRD+NW2HBu5GURwtKIyZbdR6GH5Tdv4D+FpTqDGE7m1El0e0VwAF0wF9uayA84pZ e4emLqdHIDojccmIZE+c/CNoQ4t21O4TeQJcOf2GM4L3sWL3PisqH43HGnPFca6hzoDF 35Ig== X-Gm-Message-State: ABy/qLZAWSMOTz8u/koezkKGaRSCEZWg1P+peLRPIOrG0Sy0/gT9TxvI lVQRdFi4AELvne9anKfk/1E8Txioa8v2+PZ994JqSC3GHA== X-Google-Smtp-Source: APBJJlErtWNCaVfqknb8yWvEoSONcwRK5qIO8F17Go092uCsC7G7EUWqOZLGDSJFyTGNPGgxErr8MPIPDpBTwaO1g24= X-Received: by 2002:a05:6870:4182:b0:1b0:2f70:15a5 with SMTP id y2-20020a056870418200b001b02f7015a5mr15574670oac.52.1690932806999; Tue, 01 Aug 2023 16:33:26 -0700 (PDT) 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 References: In-Reply-To: From: Rick Macklem Date: Tue, 1 Aug 2023 16:33:16 -0700 Message-ID: Subject: Re: Review of patch that uses "volatile sig_atomic_t" To: David Chisnall Cc: FreeBSD CURRENT Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4RFryd6zJzz3mJ6 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)[]; TAGGED_FROM(0.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US] On Mon, Jul 31, 2023 at 11:33=E2=80=AFPM David Chisnall 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 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. > >