On 20/05/19 05:08, Peter Xu wrote:
> cpu_physical_memory_sync_dirty_bitmap() has one RAMBlock* as
> parameter, which means that it must be with RCU read lock held
> already. Taking it again inside seems redundant. Removing it.
> Instead comment on the functions about the RCU read lock.
>
> Reviewed-by: Paolo Bonzini <pbonz...@redhat.com>
> Signed-off-by: Peter Xu <pet...@redhat.com>
> ---
> include/exec/ram_addr.h | 5 +----
> migration/ram.c | 1 +
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 139ad79390..993fb760f3 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -408,6 +408,7 @@ static inline void
> cpu_physical_memory_clear_dirty_range(ram_addr_t start,
> }
>
>
> +/* Must be with rcu read lock held */
The usual way to spell this is "Called within RCU critical section.",
otherwise the patch looks good.
Paolo
> static inline
> uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
> ram_addr_t start,
> @@ -431,8 +432,6 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock
> *rb,
> DIRTY_MEMORY_BLOCK_SIZE);
> unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
>
> - rcu_read_lock();
> -
> src = atomic_rcu_read(
> &ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION])->blocks;
>
> @@ -452,8 +451,6 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock
> *rb,
> idx++;
> }
> }
> -
> - rcu_read_unlock();
> } else {
> ram_addr_t offset = rb->offset;
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 4c60869226..05f9f36c7c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1678,6 +1678,7 @@ static inline bool
> migration_bitmap_clear_dirty(RAMState *rs,
> return ret;
> }
>
> +/* Must be with rcu read lock held */
> static void migration_bitmap_sync_range(RAMState *rs, RAMBlock *rb,
> ram_addr_t length)
> {
>
