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
>
--
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
