Re: xen_rmb, xen_wmb

["Mathew\, Cherry G." <c%bow.st@localhost>]

Taylor R Campbell <riastradh%NetBSD.org@localhost> writes:

> In sys/arch/xen/include/hypervisor.h rev. 1.48 and
> sys/arch/xen/include/xenring.h rev. 1.2, the macros xen_rmb and
> xen_wmb were defined as follows:
>
> #define xen_rmb() membar_producer()
> #define xen_wmb() membar_consumer()
>
> This definition doesn't make much sense on its face, because
> membar_producer orders writes (and may have no effect on reads), while
> membar_consumer orders reads (and may have no effect on writes).


[...]


Thanks Taylor for catching this - I totally got the semantics wrong and
and as you say, we're lucky on x86 - which is probably why it slipped
through. 



> I suggest that:
>
> 1. xen_rmb be redefined as membar_acquire (load-before-load/store),
> 2. xen_wmb be redefined as membar_release (load/store-before-store),
> and
> 3. all uses be audited to confirm they have the right semantics;
>
> or, alternatively, that the macros be eliminated and the uses be
> replaced by the appropriate membar_*.
>
>

I like the second idea better, because this is NetBSD specific code, and
we shouldn't be re-inventing APIs for which well thought out and
portable alternatives exist within the kernel.

Please feel free to interim fix with your first suggestion, if that's a
quick fix. (My commit bit is inactive, due to various reasons - and I'm
unable to test at this point, as I don't have an active NetBSD dev. env
at hand.)

Thanks again.

-- 
~cherry