Date: Sat, 29 Dec 2012 18:03:55 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Attilio Rao <attilio@freebsd.org> Cc: src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r244793 - in user/attilio/membarclean/dev: drm drm2 netmap virtio Message-ID: <20121229160355.GD82219@kib.kiev.ua> In-Reply-To: <CAJ-FndAyDgae%2B41L7psr7JR3hc%2Bkmbx9EkmJ8t441VNyXQOy_w@mail.gmail.com> References: <201212282218.qBSMIfX2015054@svn.freebsd.org> <20121228223259.GX82219@kib.kiev.ua> <CAJ-FndB5aLcO6k3JOi7SuyvNLzvttD-pEE0MQXgMiP0hJKTyRA@mail.gmail.com> <20121228231649.GZ82219@kib.kiev.ua> <CAJ-FndAq1hNmsAEUy32RfxM2aN0CquocHhOPbohchF%2BT%2B9zttA@mail.gmail.com> <20121229150107.GC82219@kib.kiev.ua> <CAJ-FndAyDgae%2B41L7psr7JR3hc%2Bkmbx9EkmJ8t441VNyXQOy_w@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--3cI6DWK3Xt33P8nt Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Dec 29, 2012 at 07:13:57AM -0800, Attilio Rao wrote: > On Sat, Dec 29, 2012 at 7:01 AM, Konstantin Belousov > <kostikbel@gmail.com> wrote: > > On Fri, Dec 28, 2012 at 11:34:39PM +0000, Attilio Rao wrote: > >> On Fri, Dec 28, 2012 at 11:16 PM, Konstantin Belousov > >> <kostikbel@gmail.com> wrote: > >> > On Fri, Dec 28, 2012 at 10:55:47PM +0000, Attilio Rao wrote: > >> >> On Fri, Dec 28, 2012 at 10:32 PM, Konstantin Belousov > >> >> <kostikbel@gmail.com> wrote: > >> >> > On Fri, Dec 28, 2012 at 10:18:41PM +0000, Attilio Rao wrote: > >> >> >> Author: attilio > >> >> >> Date: Fri Dec 28 22:18:41 2012 > >> >> >> New Revision: 244793 > >> >> >> URL: http://svnweb.freebsd.org/changeset/base/244793 > >> >> >> > >> >> >> Log: > >> >> >> - Remove rmb() usage from netmap and replace it with intended = operation > >> >> >> to do actual memory fetching reads. > >> >> >> - GC unused DRM_WRITEMEMORYBARRIER() from drm and drm2. > >> >> >> - Use atomic_load_acq_*() in virtio and drm2 in places that do= n't need > >> >> >> to use rmb(). > >> >> >> > >> >> >> All these changes remove completely rmb() from MI code, with t= he > >> >> >> exception of cxgbe which will be hammered in a followup commit. > >> >> >> > >> >> >> Modified: > >> >> >> user/attilio/membarclean/dev/drm/drmP.h > >> >> >> user/attilio/membarclean/dev/drm2/drmP.h > >> >> >> user/attilio/membarclean/dev/drm2/drm_atomic.h > >> >> >> user/attilio/membarclean/dev/drm2/drm_irq.c > >> >> >> user/attilio/membarclean/dev/netmap/netmap.c > >> >> >> user/attilio/membarclean/dev/virtio/virtqueue.c > >> >> >> > >> >> >> Modified: user/attilio/membarclean/dev/drm/drmP.h > >> >> >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D > >> >> >> --- user/attilio/membarclean/dev/drm/drmP.h Fri Dec 28 22:06:5= 0 2012 (r244792) > >> >> >> +++ user/attilio/membarclean/dev/drm/drmP.h Fri Dec 28 22:18:4= 1 2012 (r244793) > >> >> >> @@ -241,11 +241,9 @@ typedef u_int32_t u32; > >> >> >> typedef u_int16_t u16; > >> >> >> typedef u_int8_t u8; > >> >> >> > >> >> >> -/* DRM_READMEMORYBARRIER() prevents reordering of reads. > >> >> >> - * DRM_WRITEMEMORYBARRIER() prevents reordering of writes. > >> >> >> +/* DRM_WRITEMEMORYBARRIER() prevents reordering of writes. > >> >> >> * DRM_MEMORYBARRIER() prevents reordering of reads and writes. > >> >> >> */ > >> >> >> -#define DRM_READMEMORYBARRIER() rmb() > >> >> >> #define DRM_WRITEMEMORYBARRIER() wmb() > >> >> >> #define DRM_MEMORYBARRIER() mb() > >> >> >> > >> >> >> > >> >> >> Modified: user/attilio/membarclean/dev/drm2/drmP.h > >> >> >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D > >> >> >> --- user/attilio/membarclean/dev/drm2/drmP.h Fri Dec 28 22:06:5= 0 2012 (r244792) > >> >> >> +++ user/attilio/membarclean/dev/drm2/drmP.h Fri Dec 28 22:18:4= 1 2012 (r244793) > >> >> >> @@ -263,11 +263,9 @@ typedef int32_t s32; > >> >> >> typedef int16_t s16; > >> >> >> typedef int8_t s8; > >> >> >> > >> >> >> -/* DRM_READMEMORYBARRIER() prevents reordering of reads. > >> >> >> - * DRM_WRITEMEMORYBARRIER() prevents reordering of writes. > >> >> >> +/* DRM_WRITEMEMORYBARRIER() prevents reordering of writes. > >> >> >> * DRM_MEMORYBARRIER() prevents reordering of reads and writes. > >> >> >> */ > >> >> >> -#define DRM_READMEMORYBARRIER() rmb() > >> >> >> #define DRM_WRITEMEMORYBARRIER() wmb() > >> >> >> #define DRM_MEMORYBARRIER() mb() > >> >> >> > >> >> >> > >> >> >> Modified: user/attilio/membarclean/dev/drm2/drm_atomic.h > >> >> >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D > >> >> >> --- user/attilio/membarclean/dev/drm2/drm_atomic.h Fri Dec 28= 22:06:50 2012 (r244792) > >> >> >> +++ user/attilio/membarclean/dev/drm2/drm_atomic.h Fri Dec 28= 22:18:41 2012 (r244793) > >> >> >> @@ -38,6 +38,7 @@ typedef u_int32_t atomic_t; > >> >> >> > >> >> >> #define atomic_set(p, v) (*(p) =3D (v)) > >> >> >> #define atomic_read(p) (*(p)) > >> >> >> +#define atomic_read_acq(p) atomic_load_acq_int(p) > >> >> >> #define atomic_inc(p) atomic_add_int(p, 1) > >> >> >> #define atomic_dec(p) atomic_subtract_int(p, 1) > >> >> >> #define atomic_add(n, p) atomic_add_int(p, n) > >> >> >> > >> >> >> Modified: user/attilio/membarclean/dev/drm2/drm_irq.c > >> >> >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D > >> >> >> --- user/attilio/membarclean/dev/drm2/drm_irq.c Fri Dec 28= 22:06:50 2012 (r244792) > >> >> >> +++ user/attilio/membarclean/dev/drm2/drm_irq.c Fri Dec 28= 22:18:41 2012 (r244793) > >> >> >> @@ -701,8 +701,7 @@ u32 drm_vblank_count_and_time(struct drm > >> >> >> do { > >> >> >> cur_vblank =3D atomic_read(&dev->_vblank_count[crt= c]); > >> >> >> *vblanktime =3D vblanktimestamp(dev, crtc, cur_vbl= ank); > >> >> >> - rmb(); > >> >> >> - } while (cur_vblank !=3D atomic_read(&dev->_vblank_count[c= rtc])); > >> >> >> + } while (cur_vblank !=3D atomic_read_acq(&dev->_vblank_cou= nt[crtc])); > >> >> >> > >> >> >> return cur_vblank; > >> >> >> } > >> >> > > >> >> > The drm/drm2 is the contributed code, maintained outside the tree= , and I > >> >> > explicitely decided to keep the original approach. I will appreci= ate if > >> >> > this would be left as it is. > >> >> > >> >> Aren't the stubs in atomic FreeBSD specific? > >> >> So it does rely on some sort of Linux layer emulation? > >> >> Right now my target is to get rid of rmb() and wmb() and garbage > >> >> collect them. If drm2 is Linux specific and needs to stay that way I > >> >> will make a different patch. > >> > > >> > *mb() are the Linux KPI, and almost any non-trivial ported driver > >> > depends on them. The mess which we already have in tree with the > >> > duplicated and sometimes wrong definitions of the barries is because= we > >> > get this compat functions too late, and the driver authors added them > >> > locally. > >> > > >> > The functions should stay there, and their usage in the ported drive= rs too. > >> > Otherwise, the mess will reappear and continue to spread. > >> > >> I don't agree with "the functions should stay there" just for your > >> premise: they are not a FreeBSD KPI and they should be defined in > >> atomic.h. They violate our atomic/membar model and they are only > >> useful for Linux-feature emulation purposes. More importantly, they > >> are mostly wrong and the requirements are niether clear or precise > >> because they makes just no sense in FreeBSD. > > In what way they are wrong ? > > > > I am fine with removing them from the machine/atomic.h and putting > > under any other namespace, even into machine/never_use_the_linuxisms.h. > > But driver authors must not be forced to reimplement the same KPI > > Nth time. >=20 > This is fine. > They are wrong because Linux requirements about write memory barriers > are different than our _rel barriers and their requirements about read > memory barriers are different than our _acq barriers. > Per-Linux doc, infact, their write barriers only serialize over other > pre-stores while their read barriers only serialize over post-loads. > FreeBSD serializes over pre-stores/loads (for _rel) and > post-stores/loads (for _acq). Yes, but this does not make Linux-style rmb/wmb barriers incompatible with the FreeBSD acq/rel barriers. They pairs have obviously different semantic, they are not interchangable, but there is nothing new to state. I do not see how the rmb/wmb can be claimed incompatible or wrong. They are only different. >=20 > Having the wmb() and rmb() in linux compat stub, that all the drivers > can reuse, is a perfectly acceptable compromise. > However such layer doesn't exist nowadays. I think there were some > resistence in the past about implementing it, because people were > freightened that drivers weren't going to be ported natively to > FreeBSD anymore. However for simple stuff like membars this should not > withstand. Ok. >=20 > > BTW, did you looked at the lib/libc/sys/__vdso_gettimeofday.c:binuptime= () ? > > > >> > >> My current plan is to just keep mb() at the atomic-FreeBSD level and > >> get rid of the others. Still, there are situations where FreeBSD implementation does not work, see above example from libc. > >> > >> > I do prefer the drm code to stay similar to the Linux code. > >> > >> This is a different point and you are the maintainer, so you decide. I > >> will respect your opinion on this. > > No, the point is the same. The linux-style barriers are not needed > > anywhere except the contributed code. Its use in the native FreeBSD code > > is erronous, but it is not for the third-party drivers. >=20 > That is one of the points I want to make, really, so I agree. >=20 > Attilio >=20 >=20 > --=20 > Peace can only be achieved by understanding - A. Einstein --3cI6DWK3Xt33P8nt Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (FreeBSD) iQIcBAEBAgAGBQJQ3xRqAAoJEJDCuSvBvK1BjroQAJ1CBGGsfHaNJvFm2VnAKHDY f2eqxjwnqF71rcGWYTp7c0MxMOt00vCt1/GDph9pqMxpewBMJIzmnF63NoQOtLIy 27eBV6GFe68+JLRPdYrBPKEU53Xal4TuLf4nLSXBAJ9GccQ2gIrZh/8FUt75ZPoH y8Vl5MpV2veM4z449yKWEGlwY503wAi53xJSW4Wvpe1t51LZY+sjvILRexnXKwD8 Sy1QHwVbZz7mrcHovSng98Ti2LEWfqfGw++dL0255fUdqBWqOwajH9811VtasZTM rVUX7ogkOucw/kz0f35Dzob/kZATPhT58i2ZuSTj5nV/er5gOA4VXfE9oCZmKPav wD9JD8FzoQsDH17GgSiPJsFWtruIsRJWpE5rGB1HiuPwONedfdGgA3P15FSIM4LM cgCu9S/O7zCfbOfwqUu/f5lRcAlYJ0cnmTxp5oLH1qP+CS1f+/TadT7pozYpzg1J IPr9hT4C/UFp9dRw0kvBmr5e973fhbkI6+DRUm3bLdAfE8ue4Q4yw1+B2z9ieco5 IOZZRWFrTzpWQAF8ACG14M3ct+IFCCv6n6eaN65iCCo7kUcyw9NbFDXoXF4zR+CH jcfbXiPLUs/YUSm3BpZFfSjauYaHSSmvqEQeovFeQpqYR8VUc2C4q8dIA8M4l8On ckCO57xZwsLDDx9yRCfw =1Oi7 -----END PGP SIGNATURE----- --3cI6DWK3Xt33P8nt--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121229160355.GD82219>