From owner-svn-src-head@freebsd.org Tue Mar 10 19:23:37 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 98E4A26DDD4; Tue, 10 Mar 2020 19:23:37 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 48cQ475CnBz4N5L; Tue, 10 Mar 2020 19:23:35 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id 02AJNQ50052679 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Tue, 10 Mar 2020 21:23:29 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua 02AJNQ50052679 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id 02AJNQRN052678; Tue, 10 Mar 2020 21:23:26 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Tue, 10 Mar 2020 21:23:26 +0200 From: Konstantin Belousov To: Mateusz Guzik Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r358733 - head/sys/sys Message-ID: <20200310192326.GC1992@kib.kiev.ua> References: <202003080022.0280MX9j055805@repo.freebsd.org> <20200309213512.GS98340@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FORGED_GMAIL_RCVD,FREEMAIL_FROM, NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on tom.home X-Rspamd-Queue-Id: 48cQ475CnBz4N5L X-Spamd-Bar: - Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=fail reason="No valid SPF, No valid DKIM" header.from=gmail.com (policy=none); spf=softfail (mx1.freebsd.org: 2001:470:d5e7:1::1 is neither permitted nor denied by domain of kostikbel@gmail.com) smtp.mailfrom=kostikbel@gmail.com X-Spamd-Result: default: False [-2.00 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-0.997,0]; RCVD_TLS_ALL(0.00)[]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; FREEMAIL_FROM(0.00)[gmail.com]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; MIME_GOOD(-0.10)[text/plain]; HAS_XAW(0.00)[]; R_SPF_SOFTFAIL(0.00)[~all]; IP_SCORE_FREEMAIL(0.00)[]; TO_DN_SOME(0.00)[]; TO_MATCH_ENVRCPT_SOME(0.00)[]; IP_SCORE(0.00)[ip: (-3.10), ipnet: 2001:470::/32(-4.65), asn: 6939(-3.59), country: US(-0.05)]; FREEMAIL_TO(0.00)[gmail.com]; FROM_EQ_ENVFROM(0.00)[]; R_DKIM_NA(0.00)[]; MIME_TRACE(0.00)[0:+]; ASN(0.00)[asn:6939, ipnet:2001:470::/32, country:US]; RCVD_COUNT_TWO(0.00)[2]; FREEMAIL_ENVFROM(0.00)[gmail.com]; DMARC_POLICY_SOFTFAIL(0.10)[gmail.com : No valid SPF, No valid DKIM,none] 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: Tue, 10 Mar 2020 19:23:37 -0000 On Tue, Mar 10, 2020 at 12:22:09AM +0100, Mateusz Guzik wrote: > On 3/9/20, Konstantin Belousov wrote: > > On Mon, Mar 09, 2020 at 01:56:17AM +0100, Mateusz Guzik wrote: > >> 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. > > I am not sure that the new code on amd64 is much nicer. > > > > But the point was that on non-amd64, i.e. armv8 and potentially on > > powerpc 3.0, the patch substitutes release store with full barrier. The > > later is (much) slower. > > > > If an arch performs something significantly more expensive here it's > probably a performance bug in the port. It is a question of how much hardware support for fine-grained fences. Relaxed architectures add some optimized operations already, but did not (yet) implemented complete C11 optimized set. I think they eventually converge. Until they did not, I think it is more important to not pessimize to-be Tier1 arches than to collapse three fast integer instructions on amd64 into one rw op. > But perhaps more importantly > there are significantly more frequent users of > atomic_thread_fence_rel, like refcount(9). If the issue is real that > should be looked at. For refcount(9) use of release fence removal, we would need atomic_fetchadd_rel_int(). > > >> > >> 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 > > > > > -- > Mateusz Guzik