From owner-svn-src-user@FreeBSD.ORG  Sat Dec 29 09:57:44 2012
Return-Path: <owner-svn-src-user@FreeBSD.ORG>
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 <multiple recipients>; 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: <CAGaYwLdbU7Dmh5fVX5Eqn5j4O1PAXsqxQX_eAP5HV-xqfLyjpg@mail.gmail.com>
References: <201212282218.qBSMIfX2015054@svn.freebsd.org>
 <CAGaYwLcJ0S=AmqPZoBS_ZzwrKVJZEpmGyq0yJm5H-kW01susZQ@mail.gmail.com>
 <CAJ-FndDF4TXRu+Th87dYn6kEBi4zHp_0pcpD+sNi+roZ06tAJg@mail.gmail.com>
 <CAGaYwLdbU7Dmh5fVX5Eqn5j4O1PAXsqxQX_eAP5HV-xqfLyjpg@mail.gmail.com>
Date: Sat, 29 Dec 2012 10:57:41 +0100
X-Google-Sender-Auth: PKdVWgR5NHMoKVGHqLhcbRjgrew
Message-ID: <CAJ-FndAeMHELPCOyabLty1EP-GSq6K8_CESqmasvT6uCpN9CEw@mail.gmail.com>
Subject: Re: svn commit: r244793 - in user/attilio/membarclean/dev: drm drm2
 netmap virtio
From: Attilio Rao <attilio@freebsd.org>
To: Bryan Venteicher <bryanv@freebsd.org>
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 &quot; user&quot;
 src tree" <svn-src-user.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/options/svn-src-user>,
 <mailto:svn-src-user-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-user>
List-Post: <mailto:svn-src-user@freebsd.org>
List-Help: <mailto:svn-src-user-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-user>,
 <mailto:svn-src-user-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Sat, 29 Dec 2012 09:57:44 -0000

On Sat, Dec 29, 2012 at 1:18 AM, Bryan Venteicher <bryanv@freebsd.org> wrote:
> On Fri, Dec 28, 2012 at 4:58 PM, Attilio Rao <attilio@freebsd.org> wrote:
>> On Fri, Dec 28, 2012 at 10:43 PM, Bryan Venteicher
>> <bryan.venteicher@gmail.com> wrote:
>>> On Fri, Dec 28, 2012 at 4:18 PM, Attilio Rao <attilio@freebsd.org> 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 <net/netmap.h>
>>>>  #include <dev/netmap/netmap_kern.h>
>>>>
>>>> +#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