From owner-svn-src-user@FreeBSD.ORG Sun Dec 30 20:01:02 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 DF455BF5; Sun, 30 Dec 2012 20:01:02 +0000 (UTC) (envelope-from mr.kodiak@gmail.com) Received: from mail-ia0-f174.google.com (mail-ia0-f174.google.com [209.85.210.174]) by mx1.freebsd.org (Postfix) with ESMTP id 7F30A8FC0A; Sun, 30 Dec 2012 20:01:02 +0000 (UTC) Received: by mail-ia0-f174.google.com with SMTP id y25so9985463iay.19 for ; Sun, 30 Dec 2012 12:01:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:from:date :x-google-sender-auth:message-id:subject:to:cc:content-type; bh=SNXIS6fDW2kchO2qpF5RQdbK4aAZn3ahoTK5FOS7hnc=; b=y2ro9DldA1hE9TRxWQaNbvzsiyd1zrBEc8Yu0iq62LwOfOf4oxr3+D0hCrNQt7/I5+ 2ETVvi3OCUqpBbWqXyTMHPMVO7QuizCQzkpDGo4FrFC3bLkltfIuMSgw8iS0ykqQsHdl OGv/xbqFBPYzhjScpq9KQcsEKGcIJxB4yOTdCREkPsBiftKyFD2eO/nLoGZKPi7Kuk4g Ds3Yz1gto7BF3XWspVo9k4t0WlfL1l4R81+wDd9IYHwkFjOeGgN8nHlSp0bDuSxpYT2L +3Olsys3CkG/Kr5s1Xm9wwl3GiaPS0N6CP+BYw0kOqhOBHndDgrCmCgIELW1JdyC7z9G 5PXA== Received: by 10.50.156.196 with SMTP id wg4mr34933579igb.25.1356897661694; Sun, 30 Dec 2012 12:01:01 -0800 (PST) MIME-Version: 1.0 Sender: mr.kodiak@gmail.com Received: by 10.64.142.198 with HTTP; Sun, 30 Dec 2012 12:00:31 -0800 (PST) In-Reply-To: References: <201212282218.qBSMIfX2015054@svn.freebsd.org> <20121228223259.GX82219@kib.kiev.ua> <20121228231649.GZ82219@kib.kiev.ua> <20121229150107.GC82219@kib.kiev.ua> <20121229160355.GD82219@kib.kiev.ua> From: Bryan Venteicher Date: Sun, 30 Dec 2012 14:00:31 -0600 X-Google-Sender-Auth: PGYP_FV9l7nmUPkNPQazPCAhXq8 Message-ID: Subject: Re: svn commit: r244793 - in user/attilio/membarclean/dev: drm drm2 netmap virtio To: attilio@freebsd.org Content-Type: text/plain; charset=ISO-8859-1 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: Sun, 30 Dec 2012 20:01:02 -0000 On Sat, Dec 29, 2012 at 10:37 AM, Attilio Rao wrote: > On Sat, Dec 29, 2012 at 5:03 PM, Konstantin Belousov > wrote: >> On Sat, Dec 29, 2012 at 07:13:57AM -0800, Attilio Rao wrote: >>> On Sat, Dec 29, 2012 at 7:01 AM, Konstantin Belousov >>> 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 >>> >> 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 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 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 >>> >> >> >> ============================================================================== >>> >> >> >> --- user/attilio/membarclean/dev/drm/drmP.h Fri Dec 28 22:06:50 2012 (r244792) >>> >> >> >> +++ user/attilio/membarclean/dev/drm/drmP.h Fri Dec 28 22:18:41 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 >>> >> >> >> ============================================================================== >>> >> >> >> --- user/attilio/membarclean/dev/drm2/drmP.h Fri Dec 28 22:06:50 2012 (r244792) >>> >> >> >> +++ user/attilio/membarclean/dev/drm2/drmP.h Fri Dec 28 22:18:41 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 >>> >> >> >> ============================================================================== >>> >> >> >> --- 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) = (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 >>> >> >> >> ============================================================================== >>> >> >> >> --- 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 = atomic_read(&dev->_vblank_count[crtc]); >>> >> >> >> *vblanktime = vblanktimestamp(dev, crtc, cur_vblank); >>> >> >> >> - rmb(); >>> >> >> >> - } while (cur_vblank != atomic_read(&dev->_vblank_count[crtc])); >>> >> >> >> + } while (cur_vblank != atomic_read_acq(&dev->_vblank_count[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 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. >>> >> >>> >> 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. >>> >>> 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. > > As first thing they are undocumented in our world. Second thing they > are different by freebsd philosophy. And I hardly believe all drivers > developer understands this nit differences between freebsd _rel/_acq > and wmb()/acq(), leaving alone architecture porters. > >>> >>> 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. >> >>> >>> > 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 still need to look into it, but there are also other cases, like the > virtio ones, where we want to get _rel or _acq semantic for smaller > than int read and writes. I think I will make a followup soon on all > the cases where the wmb() and rmb() are used, try to see where our > initial model is failing, if any, and try to formalize the situation > some more. This is a first attempt. > But supporting wmb() and rmb() at the architecture level only because > "Linux does it" is wrong. If they belong to a compat layer, it is > acceptable. > For VirtIO, I think I would be fine with reverting r240427 to make everything just straight mb()'s, and then #ifdef'ing atomic(9) for those arch's with 16-bit support. > Attilio > > > -- > Peace can only be achieved by understanding - A. Einstein