Date: Mon, 31 Dec 2012 12:28:02 -0600 From: Bryan Venteicher <bryan.venteicher@gmail.com> To: attilio@freebsd.org 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: <CAGaYwLfKwdc4aAChX-=HTfenOaWpH_c3wAT-NFMUfYyv7sOFbQ@mail.gmail.com> In-Reply-To: <CAJ-FndAPdgL4XLbLRyZHw_tuu5jUPBJXQHrhPxaypQrikMmQsQ@mail.gmail.com> References: <201212282218.qBSMIfX2015054@svn.freebsd.org> <20121228223259.GX82219@kib.kiev.ua> <CAJ-FndB5aLcO6k3JOi7SuyvNLzvttD-pEE0MQXgMiP0hJKTyRA@mail.gmail.com> <20121228231649.GZ82219@kib.kiev.ua> <CAJ-FndAq1hNmsAEUy32RfxM2aN0CquocHhOPbohchF%2BT%2B9zttA@mail.gmail.com> <20121229150107.GC82219@kib.kiev.ua> <CAJ-FndAyDgae%2B41L7psr7JR3hc%2Bkmbx9EkmJ8t441VNyXQOy_w@mail.gmail.com> <20121229160355.GD82219@kib.kiev.ua> <CAJ-FndDhs%2BeKFVdyOV-edfbDCbqJ22zQPFgC2fKqQTbjarXY%2BQ@mail.gmail.com> <CAGaYwLfC2jLo%2BBDnsVqENrpZfFghC4Pn0_3%2B0CH%2B02jK-tx=PQ@mail.gmail.com> <CAJ-FndAPdgL4XLbLRyZHw_tuu5jUPBJXQHrhPxaypQrikMmQsQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Dec 30, 2012 at 7:33 PM, Attilio Rao <attilio@freebsd.org> wrote: > On Sun, Dec 30, 2012 at 9:00 PM, Bryan Venteicher <bryanv@freebsd.org> wrote: >> On Sat, Dec 29, 2012 at 10:37 AM, Attilio Rao <attilio@freebsd.org> wrote: >>> On Sat, Dec 29, 2012 at 5:03 PM, Konstantin Belousov >>> <kostikbel@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@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@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@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGaYwLfKwdc4aAChX-=HTfenOaWpH_c3wAT-NFMUfYyv7sOFbQ>