From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49476) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZPyVp-0001fn-Sq for qemu-devel@nongnu.org; Thu, 13 Aug 2015 15:50:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZPyVk-0001if-Ki for qemu-devel@nongnu.org; Thu, 13 Aug 2015 15:50:05 -0400 Received: from out3-smtp.messagingengine.com ([66.111.4.27]:59151) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZPyVk-0001iZ-C3 for qemu-devel@nongnu.org; Thu, 13 Aug 2015 15:50:00 -0400 Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailout.nyi.internal (Postfix) with ESMTP id 7D1BE23438 for ; Thu, 13 Aug 2015 15:49:59 -0400 (EDT) Date: Thu, 13 Aug 2015 15:50:11 -0400 From: "Emilio G. Cota" Message-ID: <20150813195011.GA1164@flamenco> References: <1439397664-70734-1-git-send-email-pbonzini@redhat.com> <1439397664-70734-9-git-send-email-pbonzini@redhat.com> <20150812203734.GA17706@flamenco> <55CC51AC.7030503@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55CC51AC.7030503@redhat.com> Subject: Re: [Qemu-devel] [PATCH 08/10] tcg: add memory barriers in page_find_alloc accesses List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: mttcg@greensocs.com, qemu-devel@nongnu.org, fred.konrad@greensocs.com On Thu, Aug 13, 2015 at 10:13:32 +0200, Paolo Bonzini wrote: > On 12/08/2015 22:37, Emilio G. Cota wrote: > > > page_find is reading the radix tree outside all locks, so it has to > > > use the RCU primitives. It does not need RCU critical sections > > > because the PageDescs are never removed, so there is never a need > > > to wait for the end of code sections that use a PageDesc. > > > > Note that rcu_find_alloc might end up writing to the tree, see below. > > Yes, but in that case it's always called with the mmap_lock held, see > patch 7. Oh I see. Didn't have much time to take a deep look at the patchset; the commit message got me confused. > page_find_alloc is only called by tb_alloc_page (called by tb_link_page > which takes mmap_lock), or by page_set_flags (called with mmap_lock held > by linux-user/mmap.c). > > > BTW the fact that there are no removals makes the use of RCU unnecessary. > > It only makes it not use the RCU synchronization primitives. You still > need the memory barriers. Yes. Personally I find it confusing to use the RCU macros just for the convenience that they bring in barriers we need; I'd prefer to explicitly have the barriers in the code. > > I argue however that it is better to call page_find/_alloc with a mutex held, > > since otherwise we'd have to add per-PageDesc locks (it's very common to > > call page_find and then update the PageDesc). > > The fields are protected by either the mmap_lock (e.g. the flags, see > page_unprotect and tb_alloc_page) or the tb_lock (e.g. the tb lists). > > The code is complicated and could definitely use more documentation, > especially for struct PageDesc, but it seems correct to me apart from > the lock inversion fixed in patch 10. OK. I have a bit of time today and tomorrow so I'll rebase my work on top of this patchset and then submit it. Thanks, Emilio