From owner-svn-src-all@freebsd.org Sat Jun 10 18:22:00 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 31CBCBF37E7; Sat, 10 Jun 2017 18:22:00 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 524B78264E; Sat, 10 Jun 2017 18:21:59 +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 v5AIKdnZ043779 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Sat, 10 Jun 2017 21:20:39 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua v5AIKdnZ043779 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id v5AIKdhI043778; Sat, 10 Jun 2017 21:20:39 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Sat, 10 Jun 2017 21:20:39 +0300 From: Konstantin Belousov To: Jonathan Looney Cc: John Baldwin , "Jonathan T. Looney" , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r319720 - head/sys/dev/vt Message-ID: <20170610182039.GL2088@kib.kiev.ua> References: <201706082047.v58KlI51079003@repo.freebsd.org> <7306919.ixyIA96xWQ@ralph.baldwin.cx> <20170610091203.GH2088@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.8.2 (2017-04-18) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 10 Jun 2017 18:22:00 -0000 On Sat, Jun 10, 2017 at 09:33:48AM -0700, Jonathan Looney wrote: > Hi Konstantin, > > On Sat, Jun 10, 2017 at 2:12 AM, Konstantin Belousov > wrote: > > No, acquire is only specified for loads, and release for stores. In other > > words, on some hypothetical ll/sc architecture, the atomic_add_acq() > > could be implemented as follows, in asm-pseudocode > > atomic_add_acq(int x): > > ll x, r1 > > acq x > > add 1, r > > sc r1, x > > Your use of the atomic does not prevent stores reordering. > > If this is true, it sounds like the atomic(9) man page needs some revision. > It says: > > When an atomic operation has acquire semantics, the effects of the > operation must have completed before any subsequent load or store (by > program order) is performed. Conversely, acquire semantics do not > require that prior loads or stores have completed before the atomic > operation is performed. > > Up until now, I have relied on this description of the way acquire barriers > work. If this description is incorrect, I think it is important to fix it > quickly. There are many issues with the atomic(9) man page, and they are not easy to fix. In essence, useful, implementable and rigorous specification of the atomics behaviour or the memory model as whole (which cannot be avoided if atomics are specified) seems to be still somewhat unsolved scientific problem. As is, I strongly suggest to rely on the standard C or C++ specifications and augment it with some code reading of arch/include/atomic.h. Despite it is unfeasible to use compiler-provided atomics in kernel in C runtime, the intended behaviour as specified in standards give us a strong base to get something that does not have inherent design mistakes. Unfortunately, this is the only way to get it correct now. > > > > I'm not claiming that this fixes all bugs in this area. (In fact, I > > > specifically disclaim this.) But, it does stop the crash from occurring. > > > > > > If you still feel there are better mechanisms to achieve the desired > > > ordering, please let me know and I'll be happy to fix and/or improve > > this. > > See the pseudocode I posted in my original reply, which uses acq/rel pair. > > > > The code you posted for vt_resume_flush_timer() would switch vd_timer_armed > from 0 to 1 even if VDF_ASYNC is not set; however, that is not what we > want. vd_timer_armed should be left untouched if VDF_ASYNC is not set. > > It sounds like what I want is some sort of fence in vt_upgrade() like jhb@ > specified in his email. For example: > > vd->vd_timer_armed = 1; > atomic_thread_fence_rel(); > vd->vd_flags |= VDF_ASYNC; > > I recognize a fence would be cleaner since I really only needed the barrier > and not the atomic operation. Do you agree the above would be sufficient to > ensure store ordering? No, it is not sufficient. The release barrier on write needs a dual acquire barrier on read to have an effect. So if you prefer to use fence_rel(), the reader should issue fence_acq() between reading vd_flags and doing cmpset on vd_timer_armed.