From owner-svn-src-user@FreeBSD.ORG Sat Dec 29 16:37:11 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 BBE155A0; Sat, 29 Dec 2012 16:37:11 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-la0-f41.google.com (mail-la0-f41.google.com [209.85.215.41]) by mx1.freebsd.org (Postfix) with ESMTP id E88068FC12; Sat, 29 Dec 2012 16:37:10 +0000 (UTC) Received: by mail-la0-f41.google.com with SMTP id em20so2245723lab.28 for ; Sat, 29 Dec 2012 08:37:09 -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=BRIGGg0ALOYUYHjbA3TvAphgb2DKpD/9VscY54jgjmg=; b=BUjQxnMc2lmcA5Nm+gsw5hUEqpD2IaSwHmo2d87Oo4O2NzFhkHqPe2XaYoHybWOysZ 4Kc8T254/ozxDBpqLV4AQ9zHooYtGK6+5K2XhRS75jniT/47Y3OKpEpGiBpJf+Hu8gsi TCA/nSqQcUeBrufqS0aTOAjRVLi+74MTpVcDGmJ0pCEJY4lTyt5oC0f7L7ge1BSt830s 9LPX4I4MdDecash0QFJJy5FOFE0gxfO/5bAMj61ekI8bTHPD+xJ/c1pr/3Uq7uu815zK nznSkPWN+jgkWnV6DdZln4id0IIrz7wqY5cqHmAfWalUb2isIrtXE/aGZxTwHLhwQdMF V4og== MIME-Version: 1.0 Received: by 10.112.84.168 with SMTP id a8mr14049622lbz.75.1356799029352; Sat, 29 Dec 2012 08:37:09 -0800 (PST) Sender: asmrookie@gmail.com Received: by 10.112.84.193 with HTTP; Sat, 29 Dec 2012 08:37:09 -0800 (PST) In-Reply-To: <20121229160355.GD82219@kib.kiev.ua> 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> Date: Sat, 29 Dec 2012 17:37:09 +0100 X-Google-Sender-Auth: MM-kG1HU7PG4X5c_1aOScgpERrw 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: Sat, 29 Dec 2012 16:37:11 -0000 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. Attilio -- Peace can only be achieved by understanding - A. Einstein