On 1/3/2012 3:07 PM, Naoya Horiguchi wrote:
> Hi,
>
> Thank you for your reviewing.
>
> On Thu, Dec 29, 2011 at 10:39:18PM -0500, KOSAKI Motohiro wrote:
> ...
>>> --- 3.2-rc5.orig/fs/proc/task_mmu.c
>>> +++ 3.2-rc5/fs/proc/task_mmu.c
>>> @@ -600,6 +600,9 @@ struct pagemapread {
>>> u64 *buffer;
>>> };
>>>
>>> +#define PAGEMAP_WALK_SIZE (PMD_SIZE)
>>> +#define PAGEMAP_WALK_MASK (PMD_MASK)
>>> +
>>> #define PM_ENTRY_BYTES sizeof(u64)
>>> #define PM_STATUS_BITS 3
>>> #define PM_STATUS_OFFSET (64 - PM_STATUS_BITS)
>>> @@ -658,6 +661,22 @@ static u64 pte_to_pagemap_entry(pte_t pte)
>>> return pme;
>>> }
>>>
>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>> +static u64 thp_pte_to_pagemap_entry(pte_t pte, int offset)
>>> +{
>>> + u64 pme = 0;
>>> + if (pte_present(pte))
>>
>> When does pte_present() return 0?
>
> It does when the page pointed to by pte is swapped-out, under page migration,
> or HWPOISONed. But currenly it can't happen on thp because thp will be
> splitted before these operations are processed.
> So this if-sentense is not necessary for now, but I think it's not a bad idea
> to put it now to prepare for future implementation.
You certainly need to add a comment. otherwise you add *unnecessary* complexity
and people is going to be puzzled.
>>> + pfn = thp_pte_to_pagemap_entry(*(pte_t *)pmd,
>>> + offset);
>>
>> This (pte_t*) cast looks introduce new implicit assumption. Please don't
>> put x86 assumption here directly.
>
> OK, I think it's better to write a separate patch for this job because
> similar assumption is used in smaps_pte_range() and gather_pte_stats().
Sound sane.
>>> + } else {
>>> + spin_unlock(&walk->mm->page_table_lock);
>>> + }
>>
>> coding standard violation. plz run check_patch.pl.
>
> checkpatch.pl says nothing for here. According to Documentation/CodingStyle,
> "no braces for single statement" rule is not applicable for else-blocks with
> one statement if corresponding if-blocks have multiple statements.
ok
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>