svn commit: r244793 - in user/attilio/membarclean/dev: drm drm2 netmap virtio
Bryan Venteicher
bryanv at freebsd.org
Sat Dec 29 00:25:29 UTC 2012
On Fri, Dec 28, 2012 at 4:58 PM, Attilio Rao <attilio at freebsd.org> wrote:
> On Fri, Dec 28, 2012 at 10:43 PM, Bryan Venteicher
> <bryan.venteicher at gmail.com> wrote:
>> On Fri, Dec 28, 2012 at 4:18 PM, Attilio Rao <attilio at 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).
>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
More information about the svn-src-user
mailing list