From owner-svn-src-user@FreeBSD.ORG Sat Dec 29 09:57:44 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 35C6A553; Sat, 29 Dec 2012 09:57:44 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-la0-f47.google.com (mail-la0-f47.google.com [209.85.215.47]) by mx1.freebsd.org (Postfix) with ESMTP id 3A6688FC0A; Sat, 29 Dec 2012 09:57:42 +0000 (UTC) Received: by mail-la0-f47.google.com with SMTP id fh20so1895989lab.20 for ; Sat, 29 Dec 2012 01:57:41 -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=Bn8TXS0vNYv1/idde+6BKf1Ajgbcbqglm8ZOa9ls8MY=; b=nLMX2L7qM1HAB9wZHNp/OdRwwYdabWH1hsTVKOlRXmAwJLuUNL1RlUbu15FpfY9hUs MHq+pFbCOI+z1dTOdd/jDotPiT5xr/1Ae9j3tJ9QWLD7R2oQjlq2TyV3M9Zo6+P95GwB yMo3tL6Suclsk1kbmu2Mr2ntCoepJJE5FCgi0/P+Nj4CJXuMPkY+XlPEfFDExBe9GbdG T57Lx1RK3DOYwdb6dZheY7k40/6EtL+ckHqpQKjlMxzToHowqNcOfGzCMj92Qc9QUZSB PgaqfZBfgTQsn4JIl3ug+UyvwEGxLU9xcdmAZTI4QmaNofUkfOTBdPigkClmQFMzF5FV GoOw== MIME-Version: 1.0 Received: by 10.152.131.137 with SMTP id om9mr33133022lab.18.1356775061443; Sat, 29 Dec 2012 01:57:41 -0800 (PST) Sender: asmrookie@gmail.com Received: by 10.112.84.193 with HTTP; Sat, 29 Dec 2012 01:57:41 -0800 (PST) In-Reply-To: References: <201212282218.qBSMIfX2015054@svn.freebsd.org> Date: Sat, 29 Dec 2012 10:57:41 +0100 X-Google-Sender-Auth: PKdVWgR5NHMoKVGHqLhcbRjgrew Message-ID: Subject: Re: svn commit: r244793 - in user/attilio/membarclean/dev: drm drm2 netmap virtio From: Attilio Rao To: Bryan Venteicher 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 09:57:44 -0000 On Sat, Dec 29, 2012 at 1:18 AM, Bryan Venteicher wrote: > 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). Yes, that is what I meant. > > 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. I'm not sure what are you critics about having mb() in atomic(9), however the baseline is simple: wmb() and rmb() have very linux-centric pre-requisite and they don't fit into FreeBSD membar scheme either technically or philosophically. mb() may have more sense, though, if you we assume as prerequisite a complete barrier, both pre- and post-, for both loads and stores and enforcing global visibility among CPUs. Thanks, Attilio -- Peace can only be achieved by understanding - A. Einstein