svn commit: r244793 - in user/attilio/membarclean/dev: drm drm2 netmap virtio
Bryan Venteicher
bryan.venteicher at gmail.com
Mon Dec 31 18:28:39 UTC 2012
On Sun, Dec 30, 2012 at 7:33 PM, Attilio Rao <attilio at freebsd.org> wrote:
> On Sun, Dec 30, 2012 at 9:00 PM, Bryan Venteicher <bryanv at freebsd.org> wrote:
>> On Sat, Dec 29, 2012 at 10:37 AM, Attilio Rao <attilio at freebsd.org> wrote:
>>> On Sat, Dec 29, 2012 at 5:03 PM, Konstantin Belousov
>>> <kostikbel at gmail.com> wrote:
>>>> On Sat, Dec 29, 2012 at 07:13:57AM -0800, Attilio Rao wrote:
>>>>> On Sat, Dec 29, 2012 at 7:01 AM, Konstantin Belousov
>>>>> <kostikbel at gmail.com> wrote:
>>>>> > 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.
>>>>>
>>>>> This is fine.
>>>>> They are wrong because Linux requirements about write memory barriers
>>>>> are different than our _rel barriers and their requirements about read
>>>>> memory barriers are different than our _acq barriers.
>>>>> Per-Linux doc, infact, their write barriers only serialize over other
>>>>> pre-stores while their read barriers only serialize over post-loads.
>>>>> FreeBSD serializes over pre-stores/loads (for _rel) and
>>>>> post-stores/loads (for _acq).
>>>> Yes, but this does not make Linux-style rmb/wmb barriers incompatible with
>>>> the FreeBSD acq/rel barriers. They pairs have obviously different
>>>> semantic, they are not interchangable, but there is nothing new to state.
>>>> I do not see how the rmb/wmb can be claimed incompatible or wrong.
>>>> They are only different.
>>>
>>> As first thing they are undocumented in our world. Second thing they
>>> are different by freebsd philosophy. And I hardly believe all drivers
>>> developer understands this nit differences between freebsd _rel/_acq
>>> and wmb()/acq(), leaving alone architecture porters.
>>>
>>>>>
>>>>> Having the wmb() and rmb() in linux compat stub, that all the drivers
>>>>> can reuse, is a perfectly acceptable compromise.
>>>>> However such layer doesn't exist nowadays. I think there were some
>>>>> resistence in the past about implementing it, because people were
>>>>> freightened that drivers weren't going to be ported natively to
>>>>> FreeBSD anymore. However for simple stuff like membars this should not
>>>>> withstand.
>>>> Ok.
>>>>
>>>>>
>>>>> > 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.
>>>> Still, there are situations where FreeBSD implementation does not work,
>>>> see above example from libc.
>>>
>>> I still need to look into it, but there are also other cases, like the
>>> virtio ones, where we want to get _rel or _acq semantic for smaller
>>> than int read and writes. I think I will make a followup soon on all
>>> the cases where the wmb() and rmb() are used, try to see where our
>>> initial model is failing, if any, and try to formalize the situation
>>> some more. This is a first attempt.
>>> But supporting wmb() and rmb() at the architecture level only because
>>> "Linux does it" is wrong. If they belong to a compat layer, it is
>>> acceptable.
>>>
>>
>> For VirtIO, I think I would be fine with reverting r240427 to make
>> everything just straight mb()'s, and then #ifdef'ing atomic(9) for
>> those arch's with 16-bit support.
>
> Can you take care of doing so? I don't plan any immediate action on
> this item in the next weeks so any help will be appreciated.
> At least add some comments saying while the wmb(), rmb() are used
> would be helpful in understanding what are the constraints in general.
>
I'm currently swamped with work, but should be able take care of it in
the next few weeks.
> Attilio
>
>
> --
> Peace can only be achieved by understanding - A. Einstein
More information about the svn-src-user
mailing list