Re: [PATCH v9 2/4] arm: ARMv7 dirty page logging inital mem region write protect (w/no huge PUD support)

[Mario Smarduch <m.smarduch@samsung.com>]

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
>