Date: Wed, 25 Apr 2012 09:26:27 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Fengwei yin <yfw.bsd@gmail.com> Cc: jack.ren@intel.com, freebsd-threads@freebsd.org Subject: Re: About the memory barrier in BSD libc Message-ID: <20120425062627.GI2358@deviant.kiev.zoral.com.ua> In-Reply-To: <CAPHpMu=Lv_zYqQEjbpSpJwYsmRr04714%2B_-2jhPqXdYj_LNgvQ@mail.gmail.com> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
--REd8r87flCTRYq/l
Content-Type: text/plain; charset=koi8-r
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
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:
> >> >
> >> > +
> >> > + =9A /*
> >> > + =9A =9A* Lock the spinlock used to protect __sglue list walk in
> >> > + =9A =9A* __sfp(). =9AThe __sfp() uses fp->_flags =3D=3D 0 test as =
an
> >> > + =9A =9A* indication of the unused FILE.
> >> > + =9A =9A*
> >> > + =9A =9A* Taking the lock prevents possible compiler or processor
> >> > + =9A =9A* reordering of the writes performed before the final _flags
> >> > + =9A =9A* cleanup, making sure that we are done with the FILE before
> >> > + =9A =9A* it is considered available.
> >> > + =9A =9A*/
> >> > + =9A STDIO_THREAD_LOCK();
> >> > =9A =9A fp->_flags =3D 0; =9A =9A =9A =9A /* Release this FILE for r=
euse. */
> >> > + =9A STDIO_THREAD_UNLOCK();
> >> > =9A =9A FUNLOCKFILE(fp);
> >> > =9A =9A return (r);
> >>
> >> Is that assumption about reordering correct? =9AI.e. is FreeBSD's spin=
lock 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 before=
a LOCK
> >> operation may appear to happen after it completes." =9ASee also acq an=
d 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 memory
> > operations become visible, the acquire in the spinlock lock provides
> > the neccesary guarentee.
>=20
> IMHO, the lock to me is too heavy here. What about this patch?
>=20
> 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.
Also, the lock is not that heavy right there, and the committed patch
provides mostly zero overhead for non-threaded case.
>=20
> 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$");
>=20
> #include "namespace.h"
> #include <errno.h>
> +#include <machine/atomic.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include "un-namespace.h"
> @@ -65,6 +66,7 @@ fclose(FILE *fp)
> FREELB(fp);
> fp->_file =3D -1;
> fp->_r =3D fp->_w =3D 0; /* Mess up if reaccessed. */
> + wmb();
> fp->_flags =3D 0; /* Release this FILE for reuse. */
> FUNLOCKFILE(fp);
> 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()
> */
> THREAD_LOCK();
> for (g =3D &__sglue; g !=3D NULL; g =3D g->next) {
> - for (fp =3D g->iobs, n =3D g->niobs; --n >=3D 0; fp++)
> - if (fp->_flags =3D=3D 0)
> + for (fp =3D g->iobs, n =3D g->niobs; --n >=3D 0; fp++) {
> + int __flags =3D fp->_flags;
> + rmb();
> + /*
> + * If could see __flags is zero here, we are sure
> + * the cleanup in fclose is done.
> + */
> + if (__flags =3D=3D 0)
> goto found;
> + }
> }
> THREAD_UNLOCK(); /* don't hold lock while malloc()ing. */
> if ((g =3D moreglue(NDYNAMIC)) =3D=3D NULL)
--REd8r87flCTRYq/l
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (FreeBSD)
iEYEARECAAYFAk+XmRMACgkQC3+MBN1Mb4jXWQCfTn0XopbMM1f3hqyZQOP46LZW
1jsAn2QhZGPePVNspHwvZ5oeQvSChtWA
=ANAw
-----END PGP SIGNATURE-----
--REd8r87flCTRYq/l--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120425062627.GI2358>