From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mario Smarduch Subject: Re: [PATCH v9 2/4] arm: ARMv7 dirty page logging inital mem region write protect (w/no huge PUD support) Date: Tue, 12 Aug 2014 16:17:10 -0700 Message-ID: <53EAA076.8060802@samsung.com> References: <1406249768-25315-1-git-send-email-m.smarduch@samsung.com> <1406249768-25315-3-git-send-email-m.smarduch@samsung.com> <20140811191257.GG10550@cbox> <53E95CD5.9070809@samsung.com> <20140812093222.GL10550@cbox> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: kvmarm@lists.cs.columbia.edu, marc.zyngier@arm.com, pbonzini@redhat.com, gleb@kernel.org, agraf@suse.de, xiantao.zhang@intel.com, borntraeger@de.ibm.com, cornelia.huck@de.ibm.com, xiaoguangrong@linux.vnet.ibm.com, steve.capper@arm.com, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, jays.lee@samsung.com, sungjinn.chung@samsung.com To: Christoffer Dall Return-path: Received: from mailout4.w2.samsung.com ([211.189.100.14]:24732 "EHLO usmailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751157AbaHLXRV (ORCPT ); Tue, 12 Aug 2014 19:17:21 -0400 Received: from uscpsbgex3.samsung.com (u124.gpu85.samsung.co.kr [203.254.195.124]) by usmailout4.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0NA7006NJVCVLJ00@usmailout4.samsung.com> for kvm@vger.kernel.org; Tue, 12 Aug 2014 19:17:20 -0400 (EDT) In-reply-to: <20140812093222.GL10550@cbox> Sender: kvm-owner@vger.kernel.org List-ID: On 08/12/2014 02:32 AM, Christoffer Dall wrote: > On Mon, Aug 11, 2014 at 05:16:21PM -0700, Mario Smarduch wrote: >> On 08/11/2014 12:12 PM, Christoffer Dall wrote: >>> Remove the parenthesis from the subject line. >> > > I just prefer not having the "(w/no huge PUD support)" part in the patch > title. Ah ok. > >> Hmmm have to check this don't see it my patch file. >>> >>> On Thu, Jul 24, 2014 at 05:56:06PM -0700, Mario Smarduch wrote: >>>> Patch adds support for initial write protection VM memlsot. This patch series >>> ^^ ^ >>> stray whitespace of >>> >> Need to watch out for these adds delays to review cycle. > > yes, I care quite a lot about proper English, syntax, grammar and > spelling. Reading critically through your own patch files before > mailing them out is a good exercise. You can even consider putting them > through a spell-checker and/or configure your editor to mark double > white space, trailing white space etc. > > [...] > >>>> + do { >>>> + next = kvm_pmd_addr_end(addr, end); >>>> + if (!pmd_none(*pmd)) { >>>> + if (kvm_pmd_huge(*pmd)) { >>>> + if (!kvm_s2pmd_readonly(pmd)) >>>> + kvm_set_s2pmd_readonly(pmd); >>>> + } else >>>> + stage2_wp_pte_range(pmd, addr, next); >>> please use a closing brace when the first part of the if-statement is a >>> multi-line block with braces, as per the CodingStyle. >> Will fix. >>>> + >>> >>> stray blank line >> >> Not sure how it got by checkpatch, will fix. > > Not sure checkpatch will complain, but I do ;) No big deal anyway. > >>> >>>> + } >>>> + } while (pmd++, addr = next, addr != end); >>>> +} >>>> + >>>> +/** >>>> + * stage2_wp_pud_range - write protect PUD range >>>> + * @kvm: pointer to kvm structure >>>> + * @pud: pointer to pgd entry >>> pgd >>>> + * @addr: range start address >>>> + * @end: range end address >>>> + * >>>> + * While walking the PUD range huge PUD pages are ignored, in the future this >>> range, huge PUDs are ignored. In the future... >>>> + * may need to be revisited. Determine how to handle huge PUDs when logging >>>> + * of dirty pages is enabled. >>> >>> I don't understand the last sentence? >> >> Probably last two sentences should be combined. >> ".... to determine how to handle huge PUT...". Would that be >> clear enough? >> >> The overall theme is what to do about PUDs - mark all pages dirty >> in the region, attempt to breakup such huge regions? >> > > I think you should just state that this is not supported and worry > about how to deal with it when it's properly supported. The TODO below > is sufficient, so just drop all mentionings about the future in the > function description above - it's likely to be forgotten when PUDs are > in fact support anyhow. Ok the simpler the better. > > Thanks, > -Christoffer > From mboxrd@z Thu Jan 1 00:00:00 1970 From: m.smarduch@samsung.com (Mario Smarduch) Date: Tue, 12 Aug 2014 16:17:10 -0700 Subject: [PATCH v9 2/4] arm: ARMv7 dirty page logging inital mem region write protect (w/no huge PUD support) In-Reply-To: <20140812093222.GL10550@cbox> References: <1406249768-25315-1-git-send-email-m.smarduch@samsung.com> <1406249768-25315-3-git-send-email-m.smarduch@samsung.com> <20140811191257.GG10550@cbox> <53E95CD5.9070809@samsung.com> <20140812093222.GL10550@cbox> Message-ID: <53EAA076.8060802@samsung.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 08/12/2014 02:32 AM, Christoffer Dall wrote: > On Mon, Aug 11, 2014 at 05:16:21PM -0700, Mario Smarduch wrote: >> On 08/11/2014 12:12 PM, Christoffer Dall wrote: >>> Remove the parenthesis from the subject line. >> > > I just prefer not having the "(w/no huge PUD support)" part in the patch > title. Ah ok. > >> Hmmm have to check this don't see it my patch file. >>> >>> On Thu, Jul 24, 2014 at 05:56:06PM -0700, Mario Smarduch wrote: >>>> Patch adds support for initial write protection VM memlsot. This patch series >>> ^^ ^ >>> stray whitespace of >>> >> Need to watch out for these adds delays to review cycle. > > yes, I care quite a lot about proper English, syntax, grammar and > spelling. Reading critically through your own patch files before > mailing them out is a good exercise. You can even consider putting them > through a spell-checker and/or configure your editor to mark double > white space, trailing white space etc. > > [...] > >>>> + do { >>>> + next = kvm_pmd_addr_end(addr, end); >>>> + if (!pmd_none(*pmd)) { >>>> + if (kvm_pmd_huge(*pmd)) { >>>> + if (!kvm_s2pmd_readonly(pmd)) >>>> + kvm_set_s2pmd_readonly(pmd); >>>> + } else >>>> + stage2_wp_pte_range(pmd, addr, next); >>> please use a closing brace when the first part of the if-statement is a >>> multi-line block with braces, as per the CodingStyle. >> Will fix. >>>> + >>> >>> stray blank line >> >> Not sure how it got by checkpatch, will fix. > > Not sure checkpatch will complain, but I do ;) No big deal anyway. > >>> >>>> + } >>>> + } while (pmd++, addr = next, addr != end); >>>> +} >>>> + >>>> +/** >>>> + * stage2_wp_pud_range - write protect PUD range >>>> + * @kvm: pointer to kvm structure >>>> + * @pud: pointer to pgd entry >>> pgd >>>> + * @addr: range start address >>>> + * @end: range end address >>>> + * >>>> + * While walking the PUD range huge PUD pages are ignored, in the future this >>> range, huge PUDs are ignored. In the future... >>>> + * may need to be revisited. Determine how to handle huge PUDs when logging >>>> + * of dirty pages is enabled. >>> >>> I don't understand the last sentence? >> >> Probably last two sentences should be combined. >> ".... to determine how to handle huge PUT...". Would that be >> clear enough? >> >> The overall theme is what to do about PUDs - mark all pages dirty >> in the region, attempt to breakup such huge regions? >> > > I think you should just state that this is not supported and worry > about how to deal with it when it's properly supported. The TODO below > is sufficient, so just drop all mentionings about the future in the > function description above - it's likely to be forgotten when PUDs are > in fact support anyhow. Ok the simpler the better. > > Thanks, > -Christoffer >