On 16/03/2017 21:02, Xu, Anthony wrote:
>>> memory_region_finalize.
>>> Let me know if you think otherwise.
>>
>> Yes, you can replace memory_region_del_subregion in
>> memory_region_finalize
>> with special code that does
>>
>> assert(!mr->enabled);
>> assert(subregion->container == mr);
>> subregion->container = NULL;
>> QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
>> memory_region_unref(subregion);
>>
>> The last four lines are shared with memory_region_del_subregion, so please
>> factor them in a new function, for example
>> memory_region_del_subregion_internal.
>
> After adding synchronize_rcu, I saw an infinite recursive call,
> mem_commit-> memory_region_finalize-> mem_commit->
> memory_region_finalize-> ......
> it caused a segment fault, because 8M stack space is used up, and found when
> memory_region_finalize is called, memory_region_transaction_depth is 0 and
> memory_region_update_pending is true. That's not normal!
Ok, this is a bug. This would fix it:
> Please review below patch
>
> diff --git a/memory.c b/memory.c
> index 64b0a60..4c95aaf 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -906,12 +906,6 @@ void memory_region_transaction_begin(void)
> ++memory_region_transaction_depth;
> }
>
> -static void memory_region_clear_pending(void)
> -{
> - memory_region_update_pending = false;
> - ioeventfd_update_pending = false;
> -}
> -
> void memory_region_transaction_commit(void)
> {
> AddressSpace *as;
> @@ -927,14 +921,14 @@ void memory_region_transaction_commit(void)
> QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> address_space_update_topology(as);
> }
> -
> + memory_region_update_pending = false;
> MEMORY_LISTENER_CALL_GLOBAL(commit, Forward);
> } else if (ioeventfd_update_pending) {
> QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> address_space_update_ioeventfds(as);
> }
> + ioeventfd_update_pending = false;
> }
> - memory_region_clear_pending();
> }
> }
So please send it to the list with Signed-off-by line.
> The thing is, seems both address_space_translate and
> address_space_dispatch_free
> are called under the global lock. When synchronize_rcu is called, no other
> threads
> are in RCU critical section.
No, not necessarily. address_space_write for example is called outside
the global lock by KVM and it calls address_space_translate.
> Seems RCU is not that useful for address space.
I suggest that you study the code more closely... there is this in
kvm-all.c:
DPRINTF("handle_mmio\n");
/* Called outside BQL */
address_space_rw(&address_space_memory,
run->mmio.phys_addr, attrs,
run->mmio.data,
run->mmio.len,
run->mmio.is_write);
and adding a simple assert() would have quickly disproved your theory.
> After adding synchronize_rcu, we noticed a RCU dead loop. synchronize_rcu is
> called
> inside RCU critical section. It happened when guest OS programmed the PCI bar.
> The call trace is like,
> address_space_write-> pci_host_config_write_common ->
> memory_region_transaction_commit ->mem_commit-> synchronize_rcu
> pci_host_config_write_common is called inside RCU critical section.
>
> The address_space_write change fixed this issue.
It's not a fix if the code is not thread-safe anymore! But I think you
have the answer now as to why you cannot use synchronize_rcu.
Paolo
