From owner-svn-src-user@FreeBSD.ORG Fri Dec 28 23:34:41 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 A202667E; Fri, 28 Dec 2012 23:34:41 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-la0-f45.google.com (mail-la0-f45.google.com [209.85.215.45]) by mx1.freebsd.org (Postfix) with ESMTP id D1A078FC14; Fri, 28 Dec 2012 23:34:40 +0000 (UTC) Received: by mail-la0-f45.google.com with SMTP id ep20so1558705lab.32 for ; Fri, 28 Dec 2012 15:34:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:reply-to:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=O9izVuwkT29Ywuzlk/WgwEQwigV/865L6fcqzNex+pg=; b=B3WZz5ad+FryDApn0Yk9R8hYmnz4RdNN7uPPbpZlgMqcE2Uqy7dvjh0PVsA6EQ7i0A a8StaTlwiX9bWosxOq42JHpmasS+g+rm+YEMxs5ShIClAtu3IbTogE08fFXezV6hfUsY yy9A/4aidjj7W7yx4MIbN3MnHEiXn5dylYa2T3W7qkigGPRUhHGYYBX5DN12UfnfEgsP LeUcVL77s54v3ntpZWbTmqCe7EAS7M8Gp9E2h+3LqjXmaumV77Io3BMd4Qad/QXiuGJq KFfpNX3cDV6w0HsLEngmYCryogE0kM7js27Uiy+nyR2ilBgmUGwQra8tGKm+QZBLUjaV d3QQ== MIME-Version: 1.0 Received: by 10.112.87.40 with SMTP id u8mr13928139lbz.50.1356737679477; Fri, 28 Dec 2012 15:34:39 -0800 (PST) Sender: asmrookie@gmail.com Received: by 10.112.84.193 with HTTP; Fri, 28 Dec 2012 15:34:39 -0800 (PST) In-Reply-To: <20121228231649.GZ82219@kib.kiev.ua> References: <201212282218.qBSMIfX2015054@svn.freebsd.org> <20121228223259.GX82219@kib.kiev.ua> <20121228231649.GZ82219@kib.kiev.ua> Date: Fri, 28 Dec 2012 23:34:39 +0000 X-Google-Sender-Auth: WurDHs_QTw8q5QL1SJnKuNb7tjM Message-ID: Subject: Re: svn commit: r244793 - in user/attilio/membarclean/dev: drm drm2 netmap virtio From: Attilio Rao To: Konstantin Belousov Content-Type: text/plain; charset=UTF-8 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 Reply-To: attilio@FreeBSD.org 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: Fri, 28 Dec 2012 23:34:41 -0000 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. My current plan is to just keep mb() at the atomic-FreeBSD level and get rid of the others. > 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. Attilio -- Peace can only be achieved by understanding - A. Einstein