From owner-svn-src-head@freebsd.org Mon Mar 9 23:22:14 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 0E0AF250064; Mon, 9 Mar 2020 23:22:14 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wr1-x444.google.com (mail-wr1-x444.google.com [IPv6:2a00:1450:4864:20::444]) (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 48bvPw0vbyz3CXK; Mon, 9 Mar 2020 23:22:11 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wr1-x444.google.com with SMTP id l18so3356553wru.11; Mon, 09 Mar 2020 16:22:11 -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 :cc; bh=DBx91N8TtDwUsF3fOOaKa0y9+kpGKFjnVrJkZBuLDZc=; b=UMBKX2f/2y+G+2Dt5Oxf8yRRZyP1Hmipi+sMnB1/QfIJHHPf9brEv81P1cSZSHOLb9 4d0TblOg22urqNUjQRi5zmrtzXACO9YChhxrdUztjQIsl0mODxeLyn8MyxjyVN0IQHOP UWgiFsbkUwHeb3ux0m/bh1+6lSLL0a4myV6LlSju9bek5inFQzVWmBklHMCTLqj3PPNt 8ye8k/38NwcWKmq/0k8FpDI5eUmi7Eww+y5EahljmwIO4BI58GsXeEJeryqTeTUNeuES 8Y3nwPAsuJfYxj7h3OqFNpOeFN7UHy7NL8xZyn8bYAuQQLOL0CkO1N3ACnomg2ipmBNZ 0hew== 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:cc; bh=DBx91N8TtDwUsF3fOOaKa0y9+kpGKFjnVrJkZBuLDZc=; b=HyhpUCp70Tb8lunljhw5W2Gzh0DjZVgDmrr+UKqB705xCuXpni4ZSZiij8qHe4Qr8S Nj0az4EsmszbybIo3gGcE83iS1WrG/fe2P4wPCEHS1AW6pPKByLImaafLiapd4Z0CHH5 gbV0i8Biz7fHUyU5AwamiZiOocs0kovvfrioY3si6eEsUGmG42w6pbTryXd+06lvXM73 7aXvQLBAykfPUX6oCeq2h9m4VkXD3bOP98YrgcOEj1AoTvu5+t3kxRFmS7CdwsRjvI6S KzdsYHT4Jdi2BNS1pn+uT41MvRxe44xg0UX+sjln/+FhTZNKjRk19WkZccaPczdE5e5D SGlg== X-Gm-Message-State: ANhLgQ2eyC5h3LlpsgQSbhf5oc1p5I2maubC2CsD/TEB3FPGGVCnfczO hB5AbtqidO8ynmpmThnsRhIVoWYX0WxUl1wkfM8= X-Google-Smtp-Source: ADFU+vsdOBwj+fh4/wnyGRHs10EjitLYze8AuA9h67FT4sjiEq+c/fWaq5HoDarICw6LVmyHDONNfQXbDA5eaKNdbow= X-Received: by 2002:a5d:628c:: with SMTP id k12mr23454512wru.237.1583796130083; Mon, 09 Mar 2020 16:22:10 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:adf:f089:0:0:0:0:0 with HTTP; Mon, 9 Mar 2020 16:22:09 -0700 (PDT) In-Reply-To: <20200309213512.GS98340@kib.kiev.ua> References: <202003080022.0280MX9j055805@repo.freebsd.org> <20200309213512.GS98340@kib.kiev.ua> From: Mateusz Guzik Date: Tue, 10 Mar 2020 00:22:09 +0100 Message-ID: Subject: Re: svn commit: r358733 - head/sys/sys To: Konstantin Belousov Cc: 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: 48bvPw0vbyz3CXK X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20161025 header.b=UMBKX2f/; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (mx1.freebsd.org: domain of mjguzik@gmail.com designates 2a00:1450:4864:20::444 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)[-0.999,0]; R_DKIM_ALLOW(-0.20)[gmail.com:s=20161025]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; FREEMAIL_FROM(0.00)[gmail.com]; R_SPF_ALLOW(-0.20)[+ip6:2a00:1450:4000::/36]; MIME_GOOD(-0.10)[text/plain]; IP_SCORE(0.00)[ip: (2.86), ipnet: 2a00:1450::/32(-2.40), asn: 15169(-1.65), country: US(-0.05)]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; TO_DN_SOME(0.00)[]; IP_SCORE_FREEMAIL(0.00)[]; TO_MATCH_ENVRCPT_SOME(0.00)[]; DKIM_TRACE(0.00)[gmail.com:+]; DMARC_POLICY_ALLOW(-0.50)[gmail.com,none]; RCVD_IN_DNSWL_NONE(0.00)[4.4.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]; FREEMAIL_TO(0.00)[gmail.com]; 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 23:22:14 -0000 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. 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. >> >> 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