| Message ID | 20230310022425.2992472-7-xuchuangxclwt@bytedance.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: reduce time of loading non-iterable vmstate | expand |
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)
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!
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. */
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(-)