From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH 3/4] KVM: Count the number of dirty pages for dirty logging Date: Tue, 27 Dec 2011 11:50:17 -0200 Message-ID: <20111227135017.GB23270@amt.cnet> References: <20111114182041.43570cdf.yoshikawa.takuya@oss.ntt.co.jp> <20111114182334.f57fbeae.yoshikawa.takuya@oss.ntt.co.jp> <4EC0E85F.80004@redhat.com> <4EF00F2F.6080305@oss.ntt.co.jp> <20111223111451.GD24308@amt.cnet> <20111224115251.2aaa9549cb0a98de79f7d918@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Takuya Yoshikawa , Avi Kivity , kvm@vger.kernel.org To: Takuya Yoshikawa Return-path: Received: from mx1.redhat.com ([209.132.183.28]:50507 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753894Ab1L0Nvh (ORCPT ); Tue, 27 Dec 2011 08:51:37 -0500 Content-Disposition: inline In-Reply-To: <20111224115251.2aaa9549cb0a98de79f7d918@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: On Sat, Dec 24, 2011 at 11:52:51AM +0900, Takuya Yoshikawa wrote: > On Fri, 23 Dec 2011 09:14:51 -0200 > Marcelo Tosatti wrote: > > > > >btw mark_page_dirty() itself seems to assume mmu_lock protection that > > > >doesn't exist. Marcelo? > > > > > > > > Not mmu_lock protection, kvm->srcu protection. > > But it just protects slot readers against updates and two, or more, threads > can call mark_page_dirty() concurrently? Yes, two or more threads can call mark_page_dirty() concurrently. > What I am worring about here is the atomicity of bitmap updates. > > commit c8240bd6f0b4b1b21ffd36dd44114d05c7afe0c0 > Author: Alexander Graf > Date: Fri Oct 30 05:47:26 2009 +0000 > > Use Little Endian for Dirty Bitmap > > has changed set_bit() to the non-atomic version and nothing protects > dirty bits if mmu_lock is not held. > > The changelog has no explanation why using non-atomic version is safe. > Some comment in the code may be worthwhile if it is really safe. It should not be necessary, atomicity of updates to memslot->dirty_bitmap, because of RCU updates to the memslot pointer (note memslot = gfn_to_memslot(kvm, gfn); in mark_page_dirty). The order is: - update page data. - grab memslot pointer. - set page bit in memslot. > I want to see some clear reasoning now if possible. The reasoning is stated above, but reviewing the code to make sure its correct is not a bad idea. > Takuya > > > > > > I want to hear the answer for this question. > > > > > > Though I myself is reading the code, I cannot understand it thoroughly yet. > > > I wish if there were mmu_lock entry in locking.txt ... > > > > Agreed. > > > > -- > Takuya Yoshikawa