Page MenuHomeFreeBSD

vmm: remove a wmb() call
ClosedPublic

Authored by avg on Oct 11 2019, 8:28 AM.

Details

Summary

After removing wmb(), vm_set_rendezvous_func() became super trivial, so
there was no point in keeping it.

The wmb (sfence on amd64, lock nop on i386) was not needed. This can be
explained from several points of view.

First, wmb() is used for store-store ordering (although, the primitive is
undocumented). There was no obvious subsequent store that needed the
barrier.

Second, x86 has a memory model with strong ordering including total store
order. An explicit store barrier may be needed only when working with
special memory (device, special caching mode) or using special instructions
(non-temporal stores). That was not the case for this code.

Third, I believe that there is a misconception that sfence "flushes" the
store buffer in a sense that it speeds up the propagation of stores from the
store buffer to the global visibility. I think that such propagation always
happens as fast as possible. sfence only makes subsequent stores wait for
that propagation to complete. So, sfence is only useful for ordering of
stores and only in the situations described above.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

avg created this revision.Oct 11 2019, 8:28 AM
kib added a comment.Oct 11 2019, 5:19 PM

It is relative, of course. SFENCE might not flush store buffers faster than possbile, but it does guarantee that code after it sees store buffers flushed.

Anyway, I think that for formal correctness stores to vm->rendezvous_func should be _rel, and loads _acq, to properly express what the removed comment tried to state.

avg added a comment.Oct 11 2019, 5:56 PM

I think that the formal correctness is achieved by rendezvous_mtx.
That wmb() was for the benefit of code like:

if (rendezvous_func != NULL) {
  ...
  mtx_lock(rendezvous_mtx);
  // do real things with rendezvous_func
  mtx_unlock(rendezvous_mtx);
}

That is, rendezvous_func is assigned and used always under rendezvous_mtx.
Access outside of the lock are only to check it for being set.

avg added a comment.Oct 17 2019, 6:24 AM

I plan on committing this tomorrow.

jhb added a comment.Oct 17 2019, 5:51 PM

The sfence does nothing for checks outside the lock, so this does indeed seem pointless. The removed code also used KASSERT(mtx_owned...) instead of mtx_assert() which seems sub-par.

Checking the function pointer outside of the lock is racy regardless. I think though it doesn't matter as bhyve just needs the vCPU to see the function pointer "eventually" and invoke it. I believe a rendezvous will block until all vCPUs have invoked the handler, so there isn't an issue with the function pointer being overwritten and a rendezvous being "missed" AFAIK.

jhb accepted this revision.Oct 17 2019, 5:52 PM
This revision is now accepted and ready to land.Oct 17 2019, 5:52 PM
This revision was automatically updated to reflect the committed changes.