Date: Fri, 28 Dec 2012 22:58:51 +0000 From: Attilio Rao <attilio@freebsd.org> To: Bryan Venteicher <bryan.venteicher@gmail.com> Cc: src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r244793 - in user/attilio/membarclean/dev: drm drm2 netmap virtio Message-ID: <CAJ-FndDF4TXRu%2BTh87dYn6kEBi4zHp_0pcpD%2BsNi%2BroZ06tAJg@mail.gmail.com> In-Reply-To: <CAGaYwLcJ0S=AmqPZoBS_ZzwrKVJZEpmGyq0yJm5H-kW01susZQ@mail.gmail.com> References: <201212282218.qBSMIfX2015054@svn.freebsd.org> <CAGaYwLcJ0S=AmqPZoBS_ZzwrKVJZEpmGyq0yJm5H-kW01susZQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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. Attilio -- Peace can only be achieved by understanding - A. Einstein
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndDF4TXRu%2BTh87dYn6kEBi4zHp_0pcpD%2BsNi%2BroZ06tAJg>