Hi Paul, > -----Original Message----- > From: kvm-ppc-owner@xxxxxxxxxxxxxxx [mailto:kvm-ppc-owner@xxxxxxxxxxxxxxx] On > Behalf Of Paul Mackerras > Sent: Monday, October 07, 2013 4:39 AM > To: Bhushan Bharat-R65777 > Cc: agraf@xxxxxxx; kvm@xxxxxxxxxxxxxxx; kvm-ppc@xxxxxxxxxxxxxxx; Wood Scott- > B07421; benh@xxxxxxxxxxxxxxxxxxx; Bhushan Bharat-R65777 > Subject: Re: [PATCH 1/3 v6] kvm: powerpc: keep only pte search logic in > lookup_linux_pte > > On Fri, Oct 04, 2013 at 08:25:31PM +0530, Bharat Bhushan wrote: > > lookup_linux_pte() was searching for a pte and also sets access flags > > is writable. This function now searches only pte while access flag > > setting is done explicitly. > > So in order to reduce some code duplication, you have added code duplication in > the existing callers of this function. I'm not convinced it's an overall win. lookup_linux_pte(): as per name it is supposed to only lookup for a pte, but it is doing more than that (Also updating the pte). So I made this function to only do lookup (which also check size). I am not an MM expert but I think we can make this function better like you suggested checking pte_present() only if _PAGE_BUSY not set. > What's left in this function is pretty trivial, just a call to > find_linux_pte_or_hugepte() and some pagesize computations. I would prefer you > found a way to do what you want without adding code duplication at the existing > call sites. What about doing this way: 1) A function which will do the lookup for Linux pte. May be call that as lookup_linux_pte() 2) lookup + page update (what the existing function lookup_linux_pte() is doing). Will rename this function to lookup_linux_pte_and_update(), which will call above defined lookup_linux_pte() Thanks -Bharat > Maybe you could have a new find_linux_pte_and_check_pagesize() and > call that from the existing lookup_linux_pte(). > > The other thing you've done, without commenting on why you have done it, is to > add a pte_present check without having looked at _PAGE_BUSY. > kvmppc_read_update_linux_pte() only checks _PAGE_PRESENT after checking that > _PAGE_BUSY is clear, so this is a semantic change, which I think is wrong for > server processors. > > So, on the whole, NACK from me for this patch. > > Paul. > -- > To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body > of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at > http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
