visorbus: wmb issues

[selltc]

Source: Thomas Gleixner tglx@linutronix.de 5/18/2016:

Completely undocumented wmb()s without corresponding rmb()s to do obscure protection of that periodic work stuff.

See KanBoard-1249.

[selltc]

Hmmm... I don't see any wmb or rmb in ANY of our code. So I think perhaps tglx was looking at an old version of our code.

[selltc]

Yes, wmb was removed way back on March 31 with commit 64938182e7836650feeb9b2b9dadd510ed4b0dad.

[davidker]

So he was looking at an older version of the code. I also wonder if for-later fixes any of his issues.

[selltc]

I looked at the other issues, and they DO still appear to be valid with the latest version of our code.

[selltc]

tglx:

Re wmb/rmb: I believe you must have been looking at an older
version of our code in Linus' tree, rather than the latest version in
Greg's staging-next tree.  Reason is, Linus' tree only contains our
source code thru 3/11 (i.e., see
(http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/log/drivers/staging/unisys),
and wmb() was removed in Greg's staging-next tree on 3/30 with
commit 64938182e7836650feeb9b2b9dadd510ed4b0dad
(https://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/commit/drivers/staging?h=staging-next&id=64938182e7836650feeb9b2b9dadd510ed4b0dad).

We've made a LOT of changes in Greg's staging-next since 3/11.
However, the other issues you bring up still look to be valid in our
latest source code (in Greg's staging-next).

Tim Sell

[mriswyth]

@davidker and @selltc I see there is a wmb() call sitting in for-later introduced at 3d69561d18478fbf98302c78b431e2754ba56789. Do either of you know if this one is necessary?

[selltc]

I think this is needed, but for some reason the community doesn't like these, and I have thus far been unwilling to start an argument. (My record for winning community arguments wouldn't even make a good batting average.)

To be honest, I think that technically we need several more wmb()s than we already have. How else do you guarantee that physical memory is updated so something executing in a completely different context (e.g., Monitor or IOVM) can see the change?

[mriswyth]

I'll think about it some more too. My experience with barriers leads me to think that it isn't needed, but they are complex to think about and my focus on them has been narrow.

My use of barriers has been around devices where the barrier ensures that instructions I wrote before the barrier will be fully executed before instructions after the barrier. Because of this, I think of the memory barriers as strictly instruction ordering enforcement. The ensure that your code running in your context executes in the order that you have specified. I don't think they allow you to guarantee anything outside of your context.

Early in the s-Par days Alan Grubb presented a talk "Volatile and Friends" that dealt with this kind of thing. I've looked for it again over the years, but haven't found a copy of it recently. I have read the following 2 things more than I should have had to and I once again find myself at a point where I don't really remember for sure what they say. https://www.kernel.org/doc/Documentation/memory-barriers.txt https://www.kernel.org/pub/linux/kernel/people/paulmck/perfbook/perfbook.html

Finally, because I always end up coming at this from the "volatile" discussion I'm always reading the following too. https://www.kernel.org/doc/Documentation/volatile-considered-harmful.txt The title is a bit misleading since the Linux kernel has special functions that specifically remove the need for volatile and I'm always coming at it from a non-Linux kernel world (Monitor), but it allows me to orient my thinking to start.

Steve M and I discuss volatile and barriers about once a year for hours whenever problems like this come up. He's my final resource whenever I have to look into these things.

[selltc]

Those are fantastic links Bryan; thanks. (...just skimmed so far...)