From owner-svn-src-user@FreeBSD.ORG Sat Dec 29 15:01:13 2012 Return-Path: Delivered-To: svn-src-user@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 1F79BD6; Sat, 29 Dec 2012 15:01:13 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) by mx1.freebsd.org (Postfix) with ESMTP id 7D09A8FC08; Sat, 29 Dec 2012 15:01:12 +0000 (UTC) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.14.5/8.14.5) with ESMTP id qBTF17Vb020456; Sat, 29 Dec 2012 17:01:07 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.7.3 kib.kiev.ua qBTF17Vb020456 Received: (from kostik@localhost) by tom.home (8.14.5/8.14.5/Submit) id qBTF17Xp020455; Sat, 29 Dec 2012 17:01:07 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Sat, 29 Dec 2012 17:01:07 +0200 From: Konstantin Belousov To: Attilio Rao Subject: Re: svn commit: r244793 - in user/attilio/membarclean/dev: drm drm2 netmap virtio Message-ID: <20121229150107.GC82219@kib.kiev.ua> References: <201212282218.qBSMIfX2015054@svn.freebsd.org> <20121228223259.GX82219@kib.kiev.ua> <20121228231649.GZ82219@kib.kiev.ua> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="WTO+dqXoBrlfXqg9" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on tom.home Cc: src-committers@freebsd.org, svn-src-user@freebsd.org X-BeenThere: svn-src-user@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the experimental " user" src tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 29 Dec 2012 15:01:13 -0000 --WTO+dqXoBrlfXqg9 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Dec 28, 2012 at 11:34:39PM +0000, Attilio Rao wrote: > On Fri, Dec 28, 2012 at 11:16 PM, Konstantin Belousov > 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 > >> 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 ope= ration > >> >> 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 don't= need > >> >> to use rmb(). > >> >> > >> >> All these changes remove completely rmb() from MI code, with the > >> >> 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:50 2= 012 (r244792) > >> >> +++ user/attilio/membarclean/dev/drm/drmP.h Fri Dec 28 22:18:41 2= 012 (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:50 2= 012 (r244792) > >> >> +++ user/attilio/membarclean/dev/drm2/drmP.h Fri Dec 28 22:18:41 2= 012 (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[crtc]); > >> >> *vblanktime =3D vblanktimestamp(dev, crtc, cur_vblank= ); > >> >> - rmb(); > >> >> - } while (cur_vblank !=3D atomic_read(&dev->_vblank_count[crtc= ])); > >> >> + } while (cur_vblank !=3D atomic_read_acq(&dev->_vblank_count[= crtc])); > >> >> > >> >> return cur_vblank; > >> >> } > >> > > >> > The drm/drm2 is the contributed code, maintained outside the tree, a= nd I > >> > explicitely decided to keep the original approach. I will appreciate= 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 drivers = too. > > Otherwise, the mess will reappear and continue to spread. >=20 > 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. BTW, did you looked at the lib/libc/sys/__vdso_gettimeofday.c:binuptime() ? >=20 > My current plan is to just keep mb() at the atomic-FreeBSD level and > get rid of the others. >=20 > > I do prefer the drm code to stay similar to the Linux code. >=20 > 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. --WTO+dqXoBrlfXqg9 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (FreeBSD) iQIcBAEBAgAGBQJQ3wWxAAoJEJDCuSvBvK1BwTIP/j4i6w6WT05R5YQ6jn/7PXxo UsaFav2nenDVFUyMppNlpdH+6VIrDoydGz6jPlm9/4EKsjftYMBbZe971chvxmMg cv4L03XuujyVPPy/3QCZ+tTPp4Krc28xvchSM7hCO8zMyUtPg6+w6JKWHyCn+p8B faO934FTV9srwSfy0p42QmB8RIQN1R4lpKLDce+vDCQUu15Ip83lbSrGd5WB/U7I CGlg7nv8PJ4gOP7ywR+JJKAbCFSS136FpztNoqsHD5x0TkJecHNb+h66CxFnz6sy PR7H7wIi9CDKFHVNXowyqEe4ObVR5ObOw7bovkaI0AqQvosaF9ia95lgZKLWUPlB 6OeX/W/la1jToCVWsNXZ4plJv4cWExOVt7kpWTgnXhFjV4h0wXB6w9IBD3B5QR4A MBBKEdpUCCkcuK+Nl3D2cs3+VRpOCv5Fp88mkwxpx9p4kmJWqj0SScoHwokI61Yg PgDi1fnRzL/h1nMJc2uvU1SRSYBe/RX3USXXSG5ucRTjn+EAVjlNRW7qkxJh2dHl owzikJ870GNDPl+qKktwbzoulF8m2202A/Sc1gvS12aOKYlNf7kFuS29hpzWYkBR c88QSpch2UoKo9++/xRCD6IaG5+FoNvQZHIDjmvnvz4VUhmW270V9objxgnmbKF9 STtOfIolQmiwI5XskT8w =5+v3 -----END PGP SIGNATURE----- --WTO+dqXoBrlfXqg9--