In last patch, we wrap vm_load with begin/commit, here we introduce
address_space_to_flatview_rcu() to avoid unnecessary enforce commit
during vm_load.

Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
---
 include/exec/memory-internal.h |  2 +-
 include/exec/memory.h          | 20 ++++++++++++++++++++
 softmmu/memory.c               |  2 +-
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
index 100c1237ac..1432240449 100644
--- a/include/exec/memory-internal.h
+++ b/include/exec/memory-internal.h
@@ -30,7 +30,7 @@ static inline AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv)
 
 static inline AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as)
 {
-    return flatview_to_dispatch(address_space_to_flatview(as));
+    return flatview_to_dispatch(address_space_to_flatview_rcu(as));
 }
 
 FlatView *address_space_get_flatview(AddressSpace *as);
diff --git a/include/exec/memory.h b/include/exec/memory.h
index d6fd89db64..235e3017bc 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1100,6 +1100,9 @@ bool memory_region_transaction_in_progress(void);
 
 void memory_region_transaction_do_commit(void);
 
+/*
+ * We recommend using this by default.
+ */
 static inline FlatView *address_space_to_flatview(AddressSpace *as)
 {
     if (qemu_mutex_iothread_locked()) {
@@ -1123,6 +1126,23 @@ static inline FlatView *address_space_to_flatview(AddressSpace *as)
     return qatomic_rcu_read(&as->current_map);
 }
 
+/*
+ * We recommend using address_space_to_flatview() rather than this one.
+ * Note that if we use this during a memory region transaction, we may
+ * see obsolete flatviews. We must bear with an obsolete map until commit.
+ * And outside a memory region transaction, this is basically the same as
+ * address_space_to_flatview().
+ */
+static inline FlatView *address_space_to_flatview_rcu(AddressSpace *as)
+{
+    /*
+     * Before using any flatview, sanity check BQL or RCU is held.
+     */
+    assert(qemu_mutex_iothread_locked() || rcu_read_is_locked());
+
+    return qatomic_rcu_read(&as->current_map);
+}
+
 /**
  * typedef flatview_cb: callback for flatview_for_each_range()
  *
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 6a8e8b4e71..33d14e967d 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -815,7 +815,7 @@ FlatView *address_space_get_flatview(AddressSpace *as)
 
     RCU_READ_LOCK_GUARD();
     do {
-        view = address_space_to_flatview(as);
+        view = address_space_to_flatview_rcu(as);
         /* If somebody has replaced as->current_map concurrently,
          * flatview_ref returns false.
          */
-- 
2.20.1

On Fri, Mar 10, 2023 at 10:24:25AM +0800, Chuang Xu wrote:
> In last patch, we wrap vm_load with begin/commit, here we introduce
> address_space_to_flatview_rcu() to avoid unnecessary enforce commit
> during vm_load.
> 
> Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
> ---
>  include/exec/memory-internal.h |  2 +-
>  include/exec/memory.h          | 20 ++++++++++++++++++++
>  softmmu/memory.c               |  2 +-
>  3 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
> index 100c1237ac..1432240449 100644
> --- a/include/exec/memory-internal.h
> +++ b/include/exec/memory-internal.h
> @@ -30,7 +30,7 @@ static inline AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv)
>  
>  static inline AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as)
>  {
> -    return flatview_to_dispatch(address_space_to_flatview(as));
> +    return flatview_to_dispatch(address_space_to_flatview_rcu(as));
>  }

I'm not sure whether this one is always safe.

tcg_commit() seems to be safe, but maybe address_space_translate_iommu() is
not?  Maybe easier to leave this untouched?

>  
>  FlatView *address_space_get_flatview(AddressSpace *as);
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index d6fd89db64..235e3017bc 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1100,6 +1100,9 @@ bool memory_region_transaction_in_progress(void);
>  
>  void memory_region_transaction_do_commit(void);
>  
> +/*
> + * We recommend using this by default.
> + */

I think this comment doesn't really help.. drop it?

>  static inline FlatView *address_space_to_flatview(AddressSpace *as)

-- 
Peter Xu

Hi, Peter,

On 2023/3/10 下午11:08, Peter Xu wrote:
> On Fri, Mar 10, 2023 at 10:24:25AM +0800, Chuang Xu wrote:
>> In last patch, we wrap vm_load with begin/commit, here we introduce
>> address_space_to_flatview_rcu() to avoid unnecessary enforce commit
>> during vm_load.
>>
>> Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
>> ---
>>   include/exec/memory-internal.h |  2 +-
>>   include/exec/memory.h          | 20 ++++++++++++++++++++
>>   softmmu/memory.c               |  2 +-
>>   3 files changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
>> index 100c1237ac..1432240449 100644
>> --- a/include/exec/memory-internal.h
>> +++ b/include/exec/memory-internal.h
>> @@ -30,7 +30,7 @@ static inline AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv)
>>   
>>   static inline AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as)
>>   {
>> -    return flatview_to_dispatch(address_space_to_flatview(as));
>> +    return flatview_to_dispatch(address_space_to_flatview_rcu(as));
>>   }
> I'm not sure whether this one is always safe.

Previously I considered that there was no address_space_translate_iommu()
traced during vm_load, so I took it as safe. But my trace may not be
able to obtain all cases of triggering do_commit() during vm_load..

>
> tcg_commit() seems to be safe, but maybe address_space_translate_iommu() is
> not?  Maybe easier to leave this untouched?

Yes, I'll fix it in v8 later.

>>   
>>   FlatView *address_space_get_flatview(AddressSpace *as);
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index d6fd89db64..235e3017bc 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -1100,6 +1100,9 @@ bool memory_region_transaction_in_progress(void);
>>   
>>   void memory_region_transaction_do_commit(void);
>>   
>> +/*
>> + * We recommend using this by default.
>> + */
> I think this comment doesn't really help.. drop it?
>
>>   static inline FlatView *address_space_to_flatview(AddressSpace *as)

Thanks!