From owner-svn-src-user@FreeBSD.ORG Sat Dec 29 00:25:28 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 E1F6B543; Sat, 29 Dec 2012 00:25:28 +0000 (UTC) (envelope-from mr.kodiak@gmail.com) Received: from mail-ia0-f178.google.com (mail-ia0-f178.google.com [209.85.210.178]) by mx1.freebsd.org (Postfix) with ESMTP id 838668FC08; Sat, 29 Dec 2012 00:25:28 +0000 (UTC) Received: by mail-ia0-f178.google.com with SMTP id k25so9349886iah.9 for ; Fri, 28 Dec 2012 16:25:28 -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=4tp6Jn3Du12AB2RXD5rKVe2uN+/5A6qK/HKHyr3by1M=; b=Ppr4SoDknvh7WERyxEqNAJWPIDUGC/t9JeTTUQHvYGjWZr8Tx+C7a2yV1NFywXlpFX FE5W7CuHL0ip5Db0hlrAZxhS/LjRYDopnjswa/v3ptSq/HpSOwHhwsGKCOLK6Yn+C3eN Rh85YxTw2O6p+Q3XLfmbX2PvaVIUOisRqJc4ARujIhmpDxpHqVXY8EkBK9OcVjIfC/Ov rTZzwoLY/AqRsKUZIU3GqbP/qpth1G9gaEDHrm8SFdpMSwNNxnPZE0u9wD9LGNLhbcuT 4cA7xKqmLP2is7fvqtVUpIs7U3kDgmPJYRCuDfGfSC9tEIIgilm1gT0yvXZoxuayHmVh yNVQ== Received: by 10.50.53.230 with SMTP id e6mr29644013igp.46.1356740356731; Fri, 28 Dec 2012 16:19:16 -0800 (PST) MIME-Version: 1.0 Sender: mr.kodiak@gmail.com Received: by 10.64.142.198 with HTTP; Fri, 28 Dec 2012 16:18:46 -0800 (PST) In-Reply-To: References: <201212282218.qBSMIfX2015054@svn.freebsd.org> From: Bryan Venteicher Date: Fri, 28 Dec 2012 18:18:46 -0600 X-Google-Sender-Auth: KU5x3GYRKn9pTrEb-Gn6q3EpGVc 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: Sat, 29 Dec 2012 00:25:29 -0000 On Fri, Dec 28, 2012 at 4:58 PM, Attilio Rao wrote: > On Fri, Dec 28, 2012 at 10:43 PM, Bryan Venteicher > wrote: >> On Fri, Dec 28, 2012 at 4:18 PM, 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; >>> } >>> >>> Modified: user/attilio/membarclean/dev/netmap/netmap.c >>> ============================================================================== >>> --- user/attilio/membarclean/dev/netmap/netmap.c Fri Dec 28 22:06:50 2012 (r244792) >>> +++ user/attilio/membarclean/dev/netmap/netmap.c Fri Dec 28 22:18:41 2012 (r244793) >>> @@ -98,6 +98,9 @@ MALLOC_DEFINE(M_NETMAP, "netmap", "Netwo >>> #include >>> #include >>> >>> +#define MEMFETCH(var) \ >>> + (__DEVOLATILE(__typeof(var), *((volatile __typeof(var) *)&var))) >>> + >>> u_int netmap_total_buffers; >>> u_int netmap_buf_size; >>> char *netmap_buffer_base; /* address of an invalid buffer */ >>> @@ -1081,10 +1084,7 @@ error: >>> error = ENXIO; >>> break; >>> } >>> - rmb(); /* make sure following reads are not from cache */ >>> - >>> - >>> - ifp = priv->np_ifp; /* we have a reference */ >>> + ifp = MEMFETCH(priv->np_ifp); >>> >>> if (ifp == NULL) { >>> D("Internal error: nifp != NULL && ifp == NULL"); >>> @@ -1194,9 +1194,7 @@ netmap_poll(struct cdev *dev, int events >>> D("No if registered"); >>> return POLLERR; >>> } >>> - rmb(); /* make sure following reads are not from cache */ >>> - >>> - ifp = priv->np_ifp; >>> + ifp = MEMFETCH(priv->np_ifp); >>> // XXX check for deleting() ? >>> if ( (ifp->if_capenable & IFCAP_NETMAP) == 0) >>> return POLLERR; >>> >>> Modified: user/attilio/membarclean/dev/virtio/virtqueue.c >>> ============================================================================== >>> --- user/attilio/membarclean/dev/virtio/virtqueue.c Fri Dec 28 22:06:50 2012 (r244792) >>> +++ user/attilio/membarclean/dev/virtio/virtqueue.c Fri Dec 28 22:18:41 2012 (r244793) >>> @@ -525,8 +525,7 @@ virtqueue_dequeue(struct virtqueue *vq, >>> used_idx = vq->vq_used_cons_idx++ & (vq->vq_nentries - 1); >>> uep = &vq->vq_ring.used->ring[used_idx]; >>> >>> - rmb(); >>> - desc_idx = (uint16_t) uep->id; >>> + desc_idx = (uint16_t)atomic_load_acq_32(&uep->id); >>> if (len != NULL) >>> *len = uep->len; >>> >> >> VirtIO uses 16-bit fields which makes it hard to adapt to MI >> atomic(9). The field here happens to be 32-bits, but that is for >> alignment purposes. I have a patch from a couple months back that >> switches VirtIO to atomic_*(), but the mb()'s are still #ifdef in >> there for the eventual support of arch's without 16-bit atomic(9) (and >> the patch needs a bit of followup work). > > I think you are referring to the wmb() usage. > However, right now we don't guarantee _8 and _16 atomic ops on every > architecture and we should stay that way. This means that even if you > introduce the _16 variant for all the architecture this is not enough > to yet use it in virtio. > I have a couple of ideas on how to fix the wmb() one, I will send you > an e-mail when I get to it. > I was referring to the use of wmb() and mb() elsewhere in the virtqueue.c. If atomic_*_16() was guaranteed to be MI, I would have already made the switch (I'm not proposing we do so, doubtful even if all arch's could support it). >From your subsequent emails, it seems you'll be removing wmb()/rmb(), but keeping mb()? I'm not a huge fan of the resulting asymmetry from using both atomic(9) and mb() in the same file. > Attilio > > > -- > Peace can only be achieved by understanding - A. Einstein