From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bharata B Rao Date: Fri, 30 Aug 2019 11:25:32 +0000 Subject: Re: [PATCH v7 1/7] kvmppc: Driver to manage pages of secure guest Message-Id: <20190830111332.GE31913@in.ibm.com> List-Id: References: <20190822102620.21897-1-bharata@linux.ibm.com> <20190822102620.21897-2-bharata@linux.ibm.com> <20190829030219.GA17497@us.ibm.com> <20190829065642.GA31913@in.ibm.com> <20190829193911.GA26729@us.ibm.com> In-Reply-To: <20190829193911.GA26729@us.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Sukadev Bhattiprolu Cc: linuxppc-dev@lists.ozlabs.org, kvm-ppc@vger.kernel.org, linux-mm@kvack.org, paulus@au1.ibm.com, aneesh.kumar@linux.vnet.ibm.com, jglisse@redhat.com, linuxram@us.ibm.com, cclaudio@linux.ibm.com, hch@lst.de On Thu, Aug 29, 2019 at 12:39:11PM -0700, Sukadev Bhattiprolu wrote: > Bharata B Rao [bharata@linux.ibm.com] wrote: > > On Wed, Aug 28, 2019 at 08:02:19PM -0700, Sukadev Bhattiprolu wrote: > Where do we serialize two threads attempting to H_SVM_PAGE_IN the same gfn > at the same time? Or one thread issuing a H_SVM_PAGE_IN and another a > H_SVM_PAGE_OUT for the same page? I am not not serializing page-in/out calls on same gfn, I thought you take care of that in UV, guess UV doesn't yet. I can probably use rmap_lock() and serialize such calls in HV if UV can't prevent such calls easily. > > > > + > > > > + if (!trylock_page(dpage)) > > > > + goto out_clear; > > > > + > > > > + *rmap = devm_pfn | KVMPPC_RMAP_DEVM_PFN; > > > > + pvt = kzalloc(sizeof(*pvt), GFP_ATOMIC); > > > > + if (!pvt) > > > > + goto out_unlock; > > If we fail to alloc, we don't clear the KVMPPC_RMAP_DEVM_PFN? Right, I will move the assignment to *rmap to after kzalloc. > > Also, when/where do we clear this flag on an uv-page-out? > kvmppc_devm_drop_pages() drops the flag on a local variable but not > in the rmap? If we don't clear the flag on page-out, would the > subsequent H_SVM_PAGE_IN of this page fail? It gets cleared in kvmppc_devm_page_free(). > > Ok. Nit. thought we can drop the "_fault" in the function name but would > collide the other "alloc_and_copy" function used during H_SVM_PAGE_IN. > If the two alloc_and_copy functions are symmetric, maybe they could > have "page_in" and "page_out" in the (already long) names. Christoph also suggested to reorganize these two calls. Will take care. Regards, Bharata.