From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56495) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UseVa-0003MB-P1 for qemu-devel@nongnu.org; Fri, 28 Jun 2013 15:39:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UseVZ-0001BP-E1 for qemu-devel@nongnu.org; Fri, 28 Jun 2013 15:39:02 -0400 Received: from mout.web.de ([212.227.15.3]:55320) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UseVZ-0001B9-4R for qemu-devel@nongnu.org; Fri, 28 Jun 2013 15:39:01 -0400 Message-ID: <51CDE648.2050000@web.de> Date: Fri, 28 Jun 2013 21:38:48 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1372444009-11544-1-git-send-email-pbonzini@redhat.com> <1372444009-11544-31-git-send-email-pbonzini@redhat.com> In-Reply-To: <1372444009-11544-31-git-send-email-pbonzini@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="----enig2RJTUVMHWOUXKDFBFKFKQ" Subject: Re: [Qemu-devel] [PATCH 30/30] exec: put address space dispatch under RCU critical section List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2RJTUVMHWOUXKDFBFKFKQ Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable 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. >=20 > 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 >=20 > mr =3D address_space_translate(...) > if (!mr->unlocked) { > locked =3D true; > qemu_mutex_lock_iothread(); > } > ...dispatch... > if (locked) { > qemu_mutex_unlock_iothread(); > } >=20 > (Note that subpages are already ready to be changed to unlocked > access). >=20 > Signed-off-by: Paolo Bonzini > --- > cputlb.c | 4 +++- > exec.c | 30 ++++++++++++++++++++++++++---- > hw/ppc/spapr_iommu.c | 10 +++++++++- > 3 files changed, 38 insertions(+), 6 deletions(-) >=20 > 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 v= addr, > tlb_add_large_page(env, vaddr, size); > } > =20 > + rcu_read_lock(); > sz =3D size; > - d =3D address_space_memory.dispatch; > + d =3D rcu_dereference(&address_space_memory.dispatch); > section =3D address_space_translate_for_iotlb(d, paddr, &xlat, &sz= ); > assert(sz >=3D TARGET_PAGE_SIZE); > =20 > @@ -321,6 +322,7 @@ void tlb_set_page(CPUArchState *env, target_ulong v= addr, > } else { > te->addr_write =3D -1; > } > + rcu_read_unlock(); > } > =20 > /* 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]; > =20 > 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 > =20 > 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 !=3D &io_mem_watch; > } > =20 > +/* Called from RCU critical section */ > static MemoryRegionSection *address_space_lookup_region(AddressSpaceDi= spatch *d, > hwaddr addr, > bool resolve_s= ubpage) > @@ -252,6 +257,7 @@ static MemoryRegionSection *address_space_lookup_re= gion(AddressSpaceDispatch *d, > return section; > } > =20 > +/* 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 =3D *plen; > =20 > + rcu_read_lock(); > for (;;) { > - section =3D address_space_translate_internal(as->dispatch, add= r, &addr, plen, true); > + AddressSpaceDispatch *d =3D rcu_dereference(&as->dispatch); > + section =3D address_space_translate_internal(d, addr, &addr, p= len, true); > mr =3D section->mr; > =20 > if (!mr->iommu_ops) { > @@ -303,9 +311,11 @@ MemoryRegion *address_space_translate(AddressSpace= *as, hwaddr addr, > *plen =3D len; > *xlat =3D 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 ------enig2RJTUVMHWOUXKDFBFKFKQ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlHN5k8ACgkQitSsb3rl5xSPvQCgrOCofLF9Rkff/MFPBOHMntlu FdoAn0OAigAnn9EXJeA0GKKWHrcd3OXU =QBaq -----END PGP SIGNATURE----- ------enig2RJTUVMHWOUXKDFBFKFKQ--