svn commit: r244793 - in user/attilio/membarclean/dev: drm drm2 netmap virtio
Konstantin Belousov
kostikbel at gmail.com
Sat Dec 29 15:01:13 UTC 2012
On Fri, Dec 28, 2012 at 11:34:39PM +0000, Attilio Rao wrote:
> On Fri, Dec 28, 2012 at 11:16 PM, Konstantin Belousov
> <kostikbel at gmail.com> wrote:
> > On Fri, Dec 28, 2012 at 10:55:47PM +0000, Attilio Rao wrote:
> >> On Fri, Dec 28, 2012 at 10:32 PM, Konstantin Belousov
> >> <kostikbel at gmail.com> wrote:
> >> > On Fri, Dec 28, 2012 at 10:18:41PM +0000, 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;
> >> >> }
> >> >
> >> > The drm/drm2 is the contributed code, maintained outside the tree, and I
> >> > explicitely decided to keep the original approach. I will appreciate if
> >> > this would be left as it is.
> >>
> >> Aren't the stubs in atomic FreeBSD specific?
> >> So it does rely on some sort of Linux layer emulation?
> >> Right now my target is to get rid of rmb() and wmb() and garbage
> >> collect them. If drm2 is Linux specific and needs to stay that way I
> >> will make a different patch.
> >
> > *mb() are the Linux KPI, and almost any non-trivial ported driver
> > depends on them. The mess which we already have in tree with the
> > duplicated and sometimes wrong definitions of the barries is because we
> > get this compat functions too late, and the driver authors added them
> > locally.
> >
> > The functions should stay there, and their usage in the ported drivers too.
> > Otherwise, the mess will reappear and continue to spread.
>
> I don't agree with "the functions should stay there" just for your
> premise: they are not a FreeBSD KPI and they should be defined in
> atomic.h. They violate our atomic/membar model and they are only
> useful for Linux-feature emulation purposes. More importantly, they
> are mostly wrong and the requirements are niether clear or precise
> because they makes just no sense in FreeBSD.
In what way they are wrong ?
I am fine with removing them from the machine/atomic.h and putting
under any other namespace, even into machine/never_use_the_linuxisms.h.
But driver authors must not be forced to reimplement the same KPI
Nth time.
BTW, did you looked at the lib/libc/sys/__vdso_gettimeofday.c:binuptime() ?
>
> My current plan is to just keep mb() at the atomic-FreeBSD level and
> get rid of the others.
>
> > I do prefer the drm code to stay similar to the Linux code.
>
> This is a different point and you are the maintainer, so you decide. I
> will respect your opinion on this.
No, the point is the same. The linux-style barriers are not needed
anywhere except the contributed code. Its use in the native FreeBSD code
is erronous, but it is not for the third-party drivers.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-user/attachments/20121229/3850448f/attachment.sig>
More information about the svn-src-user
mailing list