Re: [Qemu-devel] [PATCH 30/30] exec: put address space dispatch under RCU critical section

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 4226 bytes --]

On 2013-06-28 20:26, Paolo Bonzini wrote:
> With this change, address space dispatch can be moved outside the
> BQL.  The actual I/O would still have to happen within the lock.
> 
> The next step would be to introduce a function that can only
> be called from outside the BQL, address_space_rw_unlocked.  The
> function would do something like
> 
>     mr = address_space_translate(...)
>     if (!mr->unlocked) {
>         locked = true;
>         qemu_mutex_lock_iothread();
>     }
>     ...dispatch...
>     if (locked) {
>         qemu_mutex_unlock_iothread();
>     }
> 
> (Note that subpages are already ready to be changed to unlocked
> access).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cputlb.c             |  4 +++-
>  exec.c               | 30 ++++++++++++++++++++++++++----
>  hw/ppc/spapr_iommu.c | 10 +++++++++-
>  3 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/cputlb.c b/cputlb.c
> index 82875b1..97bfe70 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -267,8 +267,9 @@ void tlb_set_page(CPUArchState *env, target_ulong vaddr,
>          tlb_add_large_page(env, vaddr, size);
>      }
>  
> +    rcu_read_lock();
>      sz = size;
> -    d = address_space_memory.dispatch;
> +    d = rcu_dereference(&address_space_memory.dispatch);
>      section = address_space_translate_for_iotlb(d, paddr, &xlat, &sz);
>      assert(sz >= TARGET_PAGE_SIZE);
>  
> @@ -321,6 +322,7 @@ void tlb_set_page(CPUArchState *env, target_ulong vaddr,
>      } else {
>          te->addr_write = -1;
>      }
> +    rcu_read_unlock();
>  }
>  
>  /* NOTE: this function can trigger an exception */
> diff --git a/exec.c b/exec.c
> index e564014..8c6f925 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -97,6 +97,8 @@ struct PhysPageEntry {
>  typedef PhysPageEntry Node[L2_SIZE];
>  
>  struct AddressSpaceDispatch {
> +    struct rcu_head rcu;
> +
>      /* This is a multi-level map on the physical address space.
>       * The bottom level has pointers to MemoryRegionSections.
>       */
> @@ -120,6 +122,8 @@ typedef struct subpage_t {
>  #define PHYS_SECTION_WATCH 3
>  
>  typedef struct PhysPageMap {
> +    struct rcu_head rcu;
> +
>      unsigned sections_nb;
>      unsigned sections_nb_alloc;
>      unsigned nodes_nb;
> @@ -236,6 +240,7 @@ bool memory_region_is_unassigned(MemoryRegion *mr)
>          && mr != &io_mem_watch;
>  }
>  
> +/* Called from RCU critical section */
>  static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
>                                                          hwaddr addr,
>                                                          bool resolve_subpage)
> @@ -252,6 +257,7 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
>      return section;
>  }
>  
> +/* Called from RCU critical section */
>  static MemoryRegionSection *
>  address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *xlat,
>                                   hwaddr *plen, bool resolve_subpage)
> @@ -280,8 +286,10 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
>      MemoryRegion *mr;
>      hwaddr len = *plen;
>  
> +    rcu_read_lock();
>      for (;;) {
> -        section = address_space_translate_internal(as->dispatch, addr, &addr, plen, true);
> +        AddressSpaceDispatch *d = rcu_dereference(&as->dispatch);
> +        section = address_space_translate_internal(d, addr, &addr, plen, true);
>          mr = section->mr;
>  
>          if (!mr->iommu_ops) {
> @@ -303,9 +311,11 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
>      *plen = len;
>      *xlat = addr;
>      memory_region_ref(mr);
> +    rcu_read_unlock();
>      return mr;
>  }

At this point, do we still have unowned memory regions? If so, we must
not return them here if the caller is not holding the BQL (which is
supposed to protect their registration/modification/deregistration).

I played with a version today that returns NULL in this case. Then the
caller of address_space_translate can take the BQL and retry the
translation.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]