From owner-svn-src-all@freebsd.org Thu Jun 8 22:04:51 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 11610D84E45; Thu, 8 Jun 2017 22:04:51 +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 AB70B73BB4; Thu, 8 Jun 2017 22:04:50 +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 v58M4iWX030506 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Fri, 9 Jun 2017 01:04:45 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua v58M4iWX030506 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id v58M4iMI030505; Fri, 9 Jun 2017 01:04:44 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Fri, 9 Jun 2017 01:04:44 +0300 From: Konstantin Belousov To: John Baldwin Cc: "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: <20170608220444.GW2088@kib.kiev.ua> References: <201706082047.v58KlI51079003@repo.freebsd.org> <7306919.ixyIA96xWQ@ralph.baldwin.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7306919.ixyIA96xWQ@ralph.baldwin.cx> 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: Thu, 08 Jun 2017 22:04:51 -0000 On Thu, Jun 08, 2017 at 02:49:43PM -0700, John Baldwin wrote: > On Thursday, June 08, 2017 08:47:18 PM Jonathan T. Looney wrote: > > Author: jtl > > Date: Thu Jun 8 20:47:18 2017 > > New Revision: 319720 > > URL: https://svnweb.freebsd.org/changeset/base/319720 > > > > Log: > > With EARLY_AP_STARTUP enabled, we are seeing crashes in softclock_call_cc() > > during bootup. Debugging information shows that softclock_call_cc() is > > trying to execute the vt_consdev.vd_timer callout, and the callout > > structure contains a NULL c_func. > > > > This appears to be due to a race between vt_upgrade() running > > callout_reset() and vt_resume_flush_timer() calling callout_schedule(). > > > > Fix the race by ensuring that vd_timer_armed is always set before > > attempting to (re)schedule the callout. > > > > Discussed with: emaste > > MFC after: 2 weeks > > Sponsored by: Netflix > > Differential Revision: https://reviews.freebsd.org/D9828 > > This should probably be using atomic_thread_fence_foo() in conjunction with > a simple 'vd->vd_timer_armed = 1' assignment instead of abusing > atomic_add_acq_int(). Unfortunately atomic_thread_fence_*() aren't yet > documented in atomic(9). :( The commit message that added them is below > though: > > ------------------------------------------------------------------------ > r285283 | kib | 2015-07-08 11:12:24 -0700 (Wed, 08 Jul 2015) | 22 lines > > Add the atomic_thread_fence() family of functions with intent to > provide a semantic defined by the C11 fences with corresponding > memory_order. > > atomic_thread_fence_acq() gives r | r, w, where r and w are read and > write accesses, and | denotes the fence itself. > > atomic_thread_fence_rel() is r, w | w. > > atomic_thread_fence_acq_rel() is the combination of the acquire and > release in single operation. Note that reads after the acq+rel fence > could be made visible before writes preceeding the fence. > > atomic_thread_fence_seq_cst() orders all accesses before/after the > fence, and the fence itself is globally ordered against other > sequentially consistent atomic operations. > > Reviewed by: alc > Discussed with: bde > Sponsored by: The FreeBSD Foundation > MFC after: 3 weeks > > ------------------------------------------------------------------------ > > That said, it is hard to see how a bare acquire barrier is really > sufficient for anything. Acquire barriers generally must be paired with > a release barrier in order to provide sychronization. More, it seems that the acquire barrier on this side is not what you want. Acquire is only effective for load, so in the atomic_add_acq() op there, the store action does not provide any guarantees. >From the comment, it looks like you want a typical publish/consume scheme, where vd_timer_armed is the flag, and VDF_ASYNC is the data. That is, in vt_upgrade(): vd->vd_flags |= VDF_ASYNC; atomic_add_rel_int(&vd->vd_timer_armed, 1); and in vt_resume_flush_timer(): if (!atomic_cmpset_acq_int(&vd->vd_timer_armed, 0, 1) || (vd->vd_flags & VDF_ASYNC) == 0) return; But this raises another question: aren't vd_timer_armed and VDF_ASYNC designate the same condition ?