From owner-svn-src-head@freebsd.org Mon Mar 9 00:56:23 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 7A7EE253175; Mon, 9 Mar 2020 00:56:23 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wr1-x432.google.com (mail-wr1-x432.google.com [IPv6:2a00:1450:4864:20::432]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 48bKY102Skz454p; Mon, 9 Mar 2020 00:56:20 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wr1-x432.google.com with SMTP id n15so8938044wrw.13; Sun, 08 Mar 2020 17:56:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=wkBBP8PwrxlCeKq3BNTpi1zSjD9vEYDgpWqKwKzu5qE=; b=N+NWIeEVgDMQp3FcZWb8dLmK2s6Jz+7vFPE6hheWCtBSU0yeii2zTOnMVMnfzGupmK y3HBmIPZpJgDPb+iN8QtXP8M0gembLSnEORUOMGIZdAC4Pkw95yjJxKz4vNB/kfQgW/7 eyXwGlHqBlei9HySHxDUOoqI6rHRpOSm/S5UowQU6MxeCxU39ZdFHGo49x80E9CyhEEz rLpNHauUc2vC7GmyWjcZ/WjLGFnSoQLt+RwPEW1etLY/mdDyU3T5yuH8ZTMuEB5KDjoX VP2EXEKQC/f12NYIG7c6upsnab5EAWK+FYVNtkf8l4K9bC5vftTVKL+SUKKtOzdE/59G bmRQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=wkBBP8PwrxlCeKq3BNTpi1zSjD9vEYDgpWqKwKzu5qE=; b=Xatm9iGCMZQwxxtwXcuUC1xVcxqtE7sD9GYZJgxplIh3s71avzGdIqDQVsUF4EzP8F coKF2TtMF199Oeia+219tDuq5W03pUEPaF/3A4Uy96nKkxaw6qwX3vQMYx5AvASQjiIg y2GA54+AlPhO0hcjfj0WK1CGtF7QuOePtOMNE+QGEsrGfgxWFqNLDq3DGC36ze4atj5G qP1W5PP8fLXIm3wL69M0L7A5z4V9JsIKucAg/5gzVP1jjQis4dakp/0xvwd0XXPyjEIx JDJy7V1FqdnnM2ZsqodiZp0KmKGKoPoTWW0ToXSoESO07/UaZj8qiE/q+U95fPw6HXMP 5y/Q== X-Gm-Message-State: ANhLgQ2SnYrSesyQPbdFbEjcF5JEVXV7mvvrumyM7ablfrCAdPCvZc9u GX892PzsBhRs3KN4s+tbhw8r1hD7MPMO3w6XmsLTqQ== X-Google-Smtp-Source: ADFU+vvsgUydaXFI1u8Rgy0UGz8wcsYcHjNlsiWOVlDMwf50WVmoN9JM+RZZatdF7SPLrtfVS0zaoFoMOlNT/9v3w4U= X-Received: by 2002:a5d:5702:: with SMTP id a2mr11276618wrv.17.1583715378302; Sun, 08 Mar 2020 17:56:18 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:adf:f089:0:0:0:0:0 with HTTP; Sun, 8 Mar 2020 17:56:17 -0700 (PDT) In-Reply-To: <202003080022.0280MX9j055805@repo.freebsd.org> References: <202003080022.0280MX9j055805@repo.freebsd.org> From: Mateusz Guzik Date: Mon, 9 Mar 2020 01:56:17 +0100 Message-ID: Subject: Re: svn commit: r358733 - head/sys/sys To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 48bKY102Skz454p X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20161025 header.b=N+NWIeEV; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (mx1.freebsd.org: domain of mjguzik@gmail.com designates 2a00:1450:4864:20::432 as permitted sender) smtp.mailfrom=mjguzik@gmail.com X-Spamd-Result: default: False [-3.00 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; R_DKIM_ALLOW(-0.20)[gmail.com:s=20161025]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[3]; R_SPF_ALLOW(-0.20)[+ip6:2a00:1450:4000::/36]; FREEMAIL_FROM(0.00)[gmail.com]; MIME_GOOD(-0.10)[text/plain]; TO_DN_NONE(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; IP_SCORE_FREEMAIL(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; IP_SCORE(0.00)[ip: (-9.27), ipnet: 2a00:1450::/32(-2.40), asn: 15169(-1.65), country: US(-0.05)]; DKIM_TRACE(0.00)[gmail.com:+]; DMARC_POLICY_ALLOW(-0.50)[gmail.com,none]; RCVD_IN_DNSWL_NONE(0.00)[2.3.4.0.0.0.0.0.0.0.0.0.0.0.0.0.0.2.0.0.4.6.8.4.0.5.4.1.0.0.a.2.list.dnswl.org : 127.0.5.0]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; FREEMAIL_ENVFROM(0.00)[gmail.com]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US]; RCVD_COUNT_TWO(0.00)[2]; RCVD_TLS_ALL(0.00)[]; DWL_DNSWL_NONE(0.00)[gmail.com.dwl.dnswl.org : 127.0.5.0] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Mar 2020 00:56:23 -0000 On 3/8/20, Mateusz Guzik wrote: > Author: mjg > Date: Sun Mar 8 00:22:32 2020 > New Revision: 358733 > URL: https://svnweb.freebsd.org/changeset/base/358733 > > Log: > seqc: tidy up > > - avoid hand-rolled read > - match begin/end in terms of fence style > There were off lists questions about this so let me clarify. The first bit is a cosmetic change, but the second one is not. > Modified: > head/sys/sys/seqc.h > > Modified: head/sys/sys/seqc.h > ============================================================================== > --- head/sys/sys/seqc.h Sat Mar 7 15:37:23 2020 (r358732) > +++ head/sys/sys/seqc.h Sun Mar 8 00:22:32 2020 (r358733) > @@ -66,7 +66,8 @@ static __inline void > seqc_write_end(seqc_t *seqcp) > { > > - atomic_store_rel_int(seqcp, *seqcp + 1); > + atomic_thread_fence_rel(); > + *seqcp += 1; > MPASS(!seqc_in_modify(*seqcp)); > critical_exit(); > } For correct operation the counter has to be modified *before* any work is done and *after* it is completed. Consider a trivial case: seqc++; foo[0] = 0; foo[1] = 1; seqc++; There are 2 ways in which this can be mucked with: - the compiler can be looking at reordering or reworking the increment (e.g., to collapse it to just += 2 instead). a compiler barrier prevents that. - even with generated machine code which increments in correct places the cpu can be looking at reordering the operation (which is heavily dependent on architecture), in particular it could end up issuing seqc++; foo[1] = 1; which would defeat the point. This is where release fences come in. With the patched code there is: seqc++; atomic_thread_fence_rel(); /* make sure seqc++ is issued before foo starts getting modified */ foo[0] = 0; foo[1] = 1; atomic_thread_fence_rel(); /* make sure modifications to foo don't leak past this spot */ seqc++; In comparison, the previous code was: seqc++; atomic_thread_fence_rel(); /* make sure seqc++ is issued before foo starts getting modified */ foo[0] = 0; foo[1] = 1; atomic_store_rel_int(seqcp, *seqcp + 1); /* make sure modifications to too don't leak past this spot and update seqc immediately */ There are 2 differences here -- one is an improvement and second one does not matter: the win is: the previous code forces the compiler to compute an incremented value and then store it. On amd64 this translates to the following (with rdx holding the address of the counter): mov (%rdx),%eax /* load the value */ add $0x1,%eax /* add 1 */ mov %eax,(%rdx) /* store it */ On patched kernel this is: addl $0x1,(%rdx) which is clearly much nicer. the not a problem: the code does not issue the release fence after incrementing the counter the second time, meaning that in principle it can happen arbitrarily late, possibly disturbing code which waits for the counter to become even. This is not a problem because in whoever modifies it is supposed to have a lock and release it shortly after, which also posts the release fence and consequently makes sure the counter update is visible. Also note these routines come with preemption disablement around the modification -- if the thread gets preempted as a result of critical_exit from seqc_write_end, it will also publish the updated counter. If the updater never does anything which flushes the store buffer then in the absolute worst case they will get preempted, at which point once more we are covered. Note that code which wants more deterministic behavior will want to seqc_read_any, test the counter once and if it is caught uneven resort to locking. > @@ -84,7 +85,7 @@ seqc_read(const seqc_t *seqcp) > seqc_t ret; > > for (;;) { > - ret = atomic_load_acq_int(__DECONST(seqc_t *, seqcp)); > + ret = seqc_read_any(seqcp); > if (__predict_false(seqc_in_modify(ret))) { > cpu_spinwait(); > continue; > _______________________________________________ > svn-src-all@freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/svn-src-all > To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" > -- Mateusz Guzik