Date: Sat, 10 Jun 2017 09:33:48 -0700 From: Jonathan Looney <jonlooney@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: John Baldwin <jhb@freebsd.org>, "Jonathan T. Looney" <jtl@freebsd.org>, 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: <CADrOrmvUwW30ztKjeTa%2BnZ51L%2B4GPxOHo7oDPHE8TA5gTtJHQg@mail.gmail.com> In-Reply-To: <20170610091203.GH2088@kib.kiev.ua> References: <201706082047.v58KlI51079003@repo.freebsd.org> <7306919.ixyIA96xWQ@ralph.baldwin.cx> <CADrOrmvH9vL6X7yZ2-djDAV92%2Be4W84z21-y2O4RFfxei8oT%2BQ@mail.gmail.com> <20170610091203.GH2088@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Konstantin,
On Sat, Jun 10, 2017 at 2:12 AM, Konstantin Belousov <kostikbel@gmail.com>
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.
> 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?
Jonathan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CADrOrmvUwW30ztKjeTa%2BnZ51L%2B4GPxOHo7oDPHE8TA5gTtJHQg>