Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 10 Jun 2017 21:20:39 +0300
From:      Konstantin Belousov <kostikbel@gmail.com>
To:        Jonathan Looney <jonlooney@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:  <20170610182039.GL2088@kib.kiev.ua>
In-Reply-To: <CADrOrmvUwW30ztKjeTa%2BnZ51L%2B4GPxOHo7oDPHE8TA5gTtJHQg@mail.gmail.com>
References:  <201706082047.v58KlI51079003@repo.freebsd.org> <7306919.ixyIA96xWQ@ralph.baldwin.cx> <CADrOrmvH9vL6X7yZ2-djDAV92%2Be4W84z21-y2O4RFfxei8oT%2BQ@mail.gmail.com> <20170610091203.GH2088@kib.kiev.ua> <CADrOrmvUwW30ztKjeTa%2BnZ51L%2B4GPxOHo7oDPHE8TA5gTtJHQg@mail.gmail.com>

next in thread | previous in thread | raw e-mail | index | archive | help
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 <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.
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.



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170610182039.GL2088>