We wanted to capture outliers when you apply the follow up patch to vm load
procedure.
That will make depth>0 for the whole process of vm load during migration,
and we wanted to make sure it's safe, hence this patch, right?
In my perspective, both BQL and RCU can ensure that the flatview will not be
released when the worker thread accesses the flatview, because before the rcu
thread releases the flatview, it will make sure itself holding BQL and the
worker thread not holding RCU. So, whether the depth is 0 or not, as long as
BQL or RCU is held, the worker thread will not access the obsolete flatview
(IIUC 'obsolete' means that flatview is released).
To summarize, the original check didn't consider BQL, and if to consider
BQL I think it should be something like:
/* Guarantees valid access to the flatview, either lock works */
assert(BQL_HELD() || RCU_HELD());
/*
* Guarantees any BQL holder is not reading obsolete flatview (e.g. when
* during vm load)
*/
if (BQL_HELD())
assert(depth==0);
IIUC it can be merged into:
assert((BQL_HELD() && depth==0) || RCU_HELD());
IMHO assert(BQL_HELD() || RCU_HELD()) is enough..
Yes, but IMHO this will guarantee safe use of flatview only if _before_
your follow up patch.
Before that patch, the depth==0 should always stand (when BQL_HELD()
stands) I think.
After that patch, since depth will be increased at the entry of vm load
there's risk that we can overlook code that will be referencing flatview
during vm load and that can reference an obsolete flatview. Since the
whole process of qemu_loadvm_state_main() will have BQL held we won't hit
the assertion if only to check "BQL_HELD() || RCU_HELD()" because BQL_HELD
always satisfies.
Or you think that once a mr transaction is in progress, the old flatview has
been obsolete? If we regard flatview as obsolete when a mr transaction is in
progress, How can RCU ensure that flatview is not obsolete?
AFAIU RCU cannot guarantee that. So IIUC any RCU lock user need to be able
to tolerant obsolete flatviews being referenced and it should not harm the
system. If it needs the latest update, it should take care of that
separately.
For example, the virtio code we're looking at in this series uses RCU lock
to build address space cache for the device vrings which is based on the
current flatview of mem. It's safe to reference obsolete flatview in this
case (it means the flatview can be during an update when virtio is
establishing the address space cache), IMHO that's fine because the address
space cache will be updated again in virtio_memory_listener_commit() so
it'll be consistent at last. The intermediate phase of inconsistency
should be fine in this case just like any DMA happens during a memory
hotplug.
For this specific patch, IMHO the core is to check depth>0 reference, and
we need RCU_HELD to mask out where the obsolete references are fine.
Thanks,