Date: Wed, 25 Apr 2012 15:05:54 +0800 From: Fengwei yin <yfw.bsd@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: jack.ren@intel.com, freebsd-threads@freebsd.org Subject: Re: About the memory barrier in BSD libc Message-ID: <CAPHpMunhHr-yGR3vBZb176xFv_gugapm7w87P-LTciEbx%2BJHGg@mail.gmail.com> In-Reply-To: <20120425062627.GI2358@deviant.kiev.zoral.com.ua> References: <20120423084120.GD76983@zxy.spb.ru> <201204241343.q3ODhe2C032683@higson.cam.lispworks.com> <20120424140348.GY2358@deviant.kiev.zoral.com.ua> <201204241110.14017.jhb@freebsd.org> <20120424165842.GZ2358@deviant.kiev.zoral.com.ua> <201204241800.q3OI0X89007384@higson.cam.lispworks.com> <20120424181302.GB2358@deviant.kiev.zoral.com.ua> <CAPHpMu=Lv_zYqQEjbpSpJwYsmRr04714%2B_-2jhPqXdYj_LNgvQ@mail.gmail.com> <20120425062627.GI2358@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Apr 25, 2012 at 2:26 PM, Konstantin Belousov <kostikbel@gmail.com> wrote: > On Wed, Apr 25, 2012 at 11:25:40AM +0800, Fengwei yin wrote: >> On Wed, Apr 25, 2012 at 2:13 AM, Konstantin Belousov >> <kostikbel@gmail.com> wrote: >> > On Tue, Apr 24, 2012 at 07:00:33PM +0100, Martin Simmons wrote: >> >> >>>>> On Tue, 24 Apr 2012 19:58:42 +0300, Konstantin Belousov said: >> >> > >> >> > + >> >> > + =A0 /* >> >> > + =A0 =A0* Lock the spinlock used to protect __sglue list walk in >> >> > + =A0 =A0* __sfp(). =A0The __sfp() uses fp->_flags =3D=3D 0 test as= an >> >> > + =A0 =A0* indication of the unused FILE. >> >> > + =A0 =A0* >> >> > + =A0 =A0* Taking the lock prevents possible compiler or processor >> >> > + =A0 =A0* reordering of the writes performed before the final _fla= gs >> >> > + =A0 =A0* cleanup, making sure that we are done with the FILE befo= re >> >> > + =A0 =A0* it is considered available. >> >> > + =A0 =A0*/ >> >> > + =A0 STDIO_THREAD_LOCK(); >> >> > =A0 =A0 fp->_flags =3D 0; =A0 =A0 =A0 =A0 /* Release this FILE for = reuse. */ >> >> > + =A0 STDIO_THREAD_UNLOCK(); >> >> > =A0 =A0 FUNLOCKFILE(fp); >> >> > =A0 =A0 return (r); >> >> >> >> Is that assumption about reordering correct? =A0I.e. is FreeBSD's spi= nlock a >> >> two-way barrier? >> >> >> >> It isn't unusual for the locking primitive to be a one-way barrier, i= .e. (from >> >> Linux kernel memory-barriers.txt) "Memory operations that occur befor= e a LOCK >> >> operation may appear to happen after it completes." =A0See also acq a= nd rel in >> >> atomic(9). >> > Taking the lock prevents the __sfp from iterating the list until the >> > spinlock is released. Since release makes sure that all previous memor= y >> > operations become visible, the acquire in the spinlock lock provides >> > the neccesary guarentee. >> >> IMHO, the lock to me is too heavy here. What about this patch? >> >> NOTE: patch just show thoughts. I didn't even check build checking. > Yes, it might be correct. But FreeBSD does prefer the acq/rel barriers > over the rmb/wmb. > There is no stand alone acq/rel APIs (Sorry, as new guy to FreeBSD, don't know too much APIs). They are bound to atomic operations. And yes, atomic_acq/rel should work also. > Also, the lock is not that heavy right there, and the committed patch > provides mostly zero overhead for non-threaded case. But lock could introduced contention in SMP case which could be avoid with rmb/wmb. I wonder whether the rmb/wmb could be defined as a compiler barrier on non-threaded case. Like STDIO_THREAD_LOCK which has version for thread/non-thread case. >> >> diff --git a/lib/libc/stdio/fclose.c b/lib/libc/stdio/fclose.c >> index f0629e8..a26f944 100644 >> --- a/lib/libc/stdio/fclose.c >> +++ b/lib/libc/stdio/fclose.c >> @@ -38,6 +38,7 @@ __FBSDID("$FreeBSD$"); >> >> =A0#include "namespace.h" >> =A0#include <errno.h> >> +#include <machine/atomic.h> >> =A0#include <stdio.h> >> =A0#include <stdlib.h> >> =A0#include "un-namespace.h" >> @@ -65,6 +66,7 @@ fclose(FILE *fp) >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 FREELB(fp); >> =A0 =A0 =A0 fp->_file =3D -1; >> =A0 =A0 =A0 fp->_r =3D fp->_w =3D 0; =A0 =A0/* Mess up if reaccessed. */ >> + =A0 =A0 wmb(); >> =A0 =A0 =A0 fp->_flags =3D 0; =A0 =A0 =A0 =A0 /* Release this FILE for r= euse. */ >> =A0 =A0 =A0 FUNLOCKFILE(fp); >> =A0 =A0 =A0 return (r); >> diff --git a/lib/libc/stdio/findfp.c b/lib/libc/stdio/findfp.c >> index 89c0536..03b2945 100644 >> --- a/lib/libc/stdio/findfp.c >> +++ b/lib/libc/stdio/findfp.c >> @@ -129,9 +129,16 @@ __sfp() >> =A0 =A0 =A0 =A0*/ >> =A0 =A0 =A0 THREAD_LOCK(); >> =A0 =A0 =A0 for (g =3D &__sglue; g !=3D NULL; g =3D g->next) { >> - =A0 =A0 =A0 =A0 =A0 =A0 for (fp =3D g->iobs, n =3D g->niobs; --n >=3D = 0; fp++) >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (fp->_flags =3D=3D 0) >> + =A0 =A0 =A0 =A0 =A0 =A0 for (fp =3D g->iobs, n =3D g->niobs; --n >=3D = 0; fp++) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int __flags =3D fp->_flags; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rmb(); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* If could see __flags is z= ero here, we are sure >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* the cleanup in fclose is = done. >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/ >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (__flags =3D=3D 0) >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto found; >> + =A0 =A0 =A0 =A0 =A0 =A0 } >> =A0 =A0 =A0 } >> =A0 =A0 =A0 THREAD_UNLOCK(); =A0 =A0 =A0 =A0/* don't hold lock while mal= loc()ing. */ >> =A0 =A0 =A0 if ((g =3D moreglue(NDYNAMIC)) =3D=3D NULL)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAPHpMunhHr-yGR3vBZb176xFv_gugapm7w87P-LTciEbx%2BJHGg>