Re: MemoryFence()


Andrew Fish <afish@...>
 

On Feb 10, 2021, at 7:55 AM, Laszlo Ersek <lersek@...> wrote:

On 02/10/21 08:21, Ankur Arora wrote:

In general I would say that fences are -- except for one case -- strictly
greater in power than read_once/write_once. That one case is alignment
checks (which we do for instance in MmioRead32() and friends.)

Given that difference in power, I agree with the approach Paolo
recommended elsewhere that for CPU memory access syncrhonization we
start with Fences and then switch to lesser powered variants
as and when needed:

"However, these changes do not have to be done in a single step.
Starting with the rationalization of memory fences makes sense
because, in addition to enabling the annotation of code rather
than data, it is essentially a bugfix. "
Given the actual amount of MemoryFence() calls and "volatile" uses in
edk2, it's actually a pretty large undertaking to "fix" these issues. I
don't foresee a quick resolution here, especially if I'm supposed to
work on it alone (considering actual patches).

I've not seen any feedback from Mike or Liming in this thread yet, for
example, so I'm unsure about the MdePkg (BaseLib) maintainer buy-in.

This is just to state the context in which I interpret "first step" and
"essentially a bugfix". The issue is systemic in edk2, as every such
occasion as we've run into now has only added to the proliferation of
"volatile".
Laszlo,

I think it makes sense to “break this up”. Seems like we need correct primitives and documentation on how to use them. It is easy enough after that to use code review to make new code “correct”, but as you say fixing the general volatile usage (especially given some of the VC++ behavior) is a big undertaking. Let us not have perfection block being able to do it correctly going forward.

Changing the CPU drivers (MP libraries) is a complex undertaking as we really need to test against the various compilers. I’ve tracked down quite a few MP bugs in proprietary CPU drivers that did not show up under VC++, but did under clang.

Thanks,

Andrew Fish

The next stable tag is coming up too. I recommend we focus on merging
the VCPU hotplug series. I don't intend to let this thread silently
peter out (like many similar threads must have, in the past); I hope to
do something about actual patches in the next development cycle. I do
admit it looks like a daunting task, considering the multiple
architectures and toolchains.

(NB: we've not spent a single word on RISC-V yet -- AFAICS, there RISC-V
doesn't even have *any* implementation of MemoryFence(), yet. Refer to
commit 7601b251fd5c ("MdePkg/BaseLib: BaseLib for RISCV64 architecture",
2020-05-07), for example.)

"You touch it, you own it" and "learn as you go" should not be a
surprise to me in an open source project, but I admit I don't know if I
should even attempt patching UefiCpuPkg content (MdePkg and OvmfPkg look
more approachable). The risk of regressions runs high, reproducibility
is always a mess in multiprocessing code, and the code is also
privileged. I want it fixed everywhere, but I feel uncertain about the
apparent lack of commitment.

I have some hands-on multiprocessing background, but I've always made a
very conscious decision to stay away from lockless programming, and use
safe and *simple* POSIX Threads-based schemes (mutexes, condition
variables with *broadcasts* etc). The "network of IP blocks" that Ard
used for (correctly) describing modern computer architecture is way
below the programming abstraction level where I feel comfortable. Thus
I'm very much out of my comfort zone here. (My initial decision against
lockless programming goes back to when <http://www.1024cores.net/> was
published originally.)

Anyway: I have several messages in this thread starred for revising.
Ankur, is the picture clear enough for you to return to work on the v7
unplug series? The soft feature freeze is coming: 2021-02-22.

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning

Thanks
Laszlo

Join rfc@edk2.groups.io to automatically receive all group messages.