From nobody Thu Aug 03 02:25:50 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 4RGXlh4fQ8z4dJMX for ; Thu, 3 Aug 2023 02:26:03 +0000 (UTC) (envelope-from rick.macklem@gmail.com) Received: from mail-oi1-x235.google.com (mail-oi1-x235.google.com [IPv6:2607:f8b0:4864:20::235]) (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 4RGXlG2lyBz47Sp; Thu, 3 Aug 2023 02:26:02 +0000 (UTC) (envelope-from rick.macklem@gmail.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-oi1-x235.google.com with SMTP id 5614622812f47-3a44fae863fso342908b6e.0; Wed, 02 Aug 2023 19:26:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1691029560; x=1691634360; 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=Qa2lDLEyUW+TWVGbNeIXSvlymIHKZsJsCn6GNfiBfmo=; b=OKh8Non6YDwXaZBv/31HdEVZxyZGHccURC5B80sHqb4oQBzgkHiPy9C20w5nx6AG+U Hwt2g8dJEaPfIi2NBDl5fWdx3x8TzP6ZoA07Lpl3WfQ/gL9cVb2XVYqaH0iOy+ncGOJ+ f3M28zD8e7FFSbQCMjHKp9S7po+R7LBQR6ae/JPgOBdHGIxyRsg+XCmhMMywUoff51LF HggwpJ9OH1oMlxSiYonWz/CwIgR4EBRqOjyQMP27ABg8+RY86wptzXblANrgyA/xB9TY aaNNM/brUd1TA1JGvcoD6Un2KjUF8yGUDoftobWnyffRVKwzPQagBW/jspfaxKsz5zvL l0Aw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1691029560; x=1691634360; 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=Qa2lDLEyUW+TWVGbNeIXSvlymIHKZsJsCn6GNfiBfmo=; b=PA6n3n0lkzNIAnRPHVsvW7fVSodVV/wreIHmEhY8I1GYTwM8iJ7COzc+6+LYauJanN a6clwy2dHbi/I65EVrRkZg9O+Ex3h13JoaKJ9R/b166e2JGdLSkurWcOsiOHJvqdjoP3 dSiUOe6oFdL8ZNOoDo7FfRIN8tRLQi0CdUozijMNrWLnqBI39oqNb3f04ei0ktGS1KBz 4+vX3Aaww7N42TtxdeWECy+Kn2976Mk/Nk6UN3oEXQ2hkZKlU6hiOnGxYQNCt+5PLV5n jsGQoOOl0u3TUukyUhwjmM4JvNRBXg1Nq+H1vgblGHxmakr/F9mlxENNpsS3SXwdyRBO 09xw== X-Gm-Message-State: ABy/qLYRAg8qGFc71mbBRCAgc75S1zhSgPJshyIdfKsnzXD4td5SiAOS 5qsVgC7/euWlej02206/x9xcudknhA1OpHPa5A== X-Google-Smtp-Source: APBJJlF/aSyJhEgAVltsdkCaUen5yM6yo9H0uD0SGaLW/Vhp1pdUrlh2AwnzaIO5C/KLYEoHceX3ZJWpx2A2oQ29yb0= X-Received: by 2002:aca:1e04:0:b0:3a3:6f20:39b4 with SMTP id m4-20020aca1e04000000b003a36f2039b4mr17192856oic.29.1691029560160; Wed, 02 Aug 2023 19:26:00 -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: Wed, 2 Aug 2023 19:25:50 -0700 Message-ID: Subject: Re: Review of patch that uses "volatile sig_atomic_t" To: Konstantin Belousov Cc: David Chisnall , FreeBSD CURRENT Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4RGXlG2lyBz47Sp 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:15169, ipnet:2607:f8b0::/32, country:US]; TAGGED_FROM(0.00)[] On Wed, Aug 2, 2023 at 6:14=E2=80=AFPM Konstantin Belousov wrote: > > On Tue, Aug 01, 2023 at 04:33:16PM -0700, Rick Macklem wrote: > > 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 de= sire to return volatile to its original use and make any use of volatile ot= her 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 lo= ads or stores and may not narrow them (I am not sure if it=E2=80=99s allowe= d to widen them). Loads and stores to a volatile variable may not be reorde= red with respect to other loads or stores to the same object but *may* be r= eordered with respect to any other accesses. > > > > > > The sig_atomic_t typedef just indicates an integer type that can be l= oaded and stored with a single instruction and so is immune to tearing if u= pdated from a signal handler. There is no requirement to use this from sign= al handlers in preference to int on FreeBSD (whether other types work is im= plementation defined and int works on all supported architectures for us). > > > > > > The weak ordering guarantees for volatile mean that any code using vo= latile for detecting whether a signal has fired is probably wrong if if doe= s not include a call to automic_signal_fence(). This guarantees that the co= mpiler will not reorder the load of the volatile with respect to other acce= sses. In practice, compilers tend to be fairly conservative about reorderin= g volatile accesses and so it probably won=E2=80=99t break until you upgrad= e your compiler in a few years time. > > > > > > My general recommendation is to use _Atomic(int) (or ideally a enum t= ype) for this. If you just use it like a normal int, you will get sequentia= lly consistent atomics. On a weakly ordered platform like Arm this will inc= lude some more atomic barrier instructions but it will do the right thing i= f you add additional threads monitoring the same variable later. In somethi= ng like mountd, the extra performance overhead from the barriers is unlikel= y to be measurable, if it is then you can weaken the atomicity (sequentiall= y consistent unless specified otherwise is a good default in C/C++, for onc= e prioritising correctness over performance). > > > > Just trying to understand what you are suggesting... > > 1 - Declare the variable _Atomic(int) OR atomic_int (is there a prefere= nce) and > > not volatile. > Really does not matter. > > > 2 - Is there a need for signal_atomic_fence(memory_order_acquire); befo= re 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. > In mountd, there are two signal handlers. One for SIGHUP, and another fo= r > SIGTERM. They are very different, let me explain. > > For SIGHUP, the only action in the signal handler is the assignment to > the variable. The assignment itself is atomic on FreeBSD. But what is m= ore > important, there is no ordering issues between this assignment and any ot= her > action. Eventually the main execution context notices that the variable = is > set and does something (re-read config). As consequence, there is no nee= d > in any fencing of the SIGHUP handler. The only concern would be that the setting of "got_sighup" would get lost d= ue to some compiler optimization and it sounds like _Atomic(int) is sufficient= to guarantee that will not happen. rick > > > 3 - Is there any need for other atomic_XXX() calls where the variable i= s used > > outside of the signal handler? > > The SIGTERM is different, it does some rpc bind calls to unregister itsel= f > as a mount program. If these actions can interfere with the main context > execution (I believe they are), then userspace rpc bind client state can > be corrupted, resulting in failing attempt to unregister. > > No amount of atomics or fencing would fix it, the unregister action shoul= d > be either moved out of the signal handler context to main context. Anoth= er > option might be to block SIGTERM, and only unblock it in places where it > is safe to do rpcb calls from interrupt. This is approximately what dd(1= ) > does. Yes, I think moving the termination into the main context should work. I'll do that as a separate commit/review. I suspect that failing to de-register doesn't cause too much of a problem. since the client won't be able to connect to the port# after mountd has terminated and will fail (just maybe not a gracefully). Same would happen if mountd crashes for some reason (and not terminated via SIGTERM). rick > > > > > 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? > > > > Thanks, rick > > > > > > David > > > > > > > On 1 Aug 2023, at 06:14, Rick Macklem wrot= e: > > > > > > > > =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. > > > > > > >