* Re: [PATCH] /proc/PID/smaps: Add PMD migration entry parsing
2020-03-31 8:56 [PATCH] /proc/PID/smaps: Add PMD migration entry parsing Huang, Ying
@ 2020-03-31 9:51 ` Konstantin Khlebnikov
2020-04-01 2:31 ` Huang, Ying
2020-03-31 12:24 ` Zi Yan
2020-04-01 23:04 ` Andrew Morton
2 siblings, 1 reply; 12+ messages in thread
From: Konstantin Khlebnikov @ 2020-03-31 9:51 UTC (permalink / raw)
To: Huang, Ying, Andrew Morton
Cc: linux-mm, linux-kernel, Andrea Arcangeli, Kirill A . Shutemov,
Zi Yan, Vlastimil Babka, Alexey Dobriyan, Michal Hocko,
Jérôme Glisse, Yang Shi
On 31/03/2020 11.56, Huang, Ying wrote:
> From: Huang Ying <ying.huang@intel.com>
>
> Now, when read /proc/PID/smaps, the PMD migration entry in page table is simply
> ignored. To improve the accuracy of /proc/PID/smaps, its parsing and processing
> is added.
>
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Cc: "Jérôme Glisse" <jglisse@redhat.com>
> Cc: Yang Shi <yang.shi@linux.alibaba.com>
> ---
> fs/proc/task_mmu.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 8d382d4ec067..b5b3aef8cb3b 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -548,8 +548,17 @@ static void smaps_pmd_entry(pmd_t *pmd, unsigned long addr,
> bool locked = !!(vma->vm_flags & VM_LOCKED);
> struct page *page;
struct page *page = NULL;
>
> - /* FOLL_DUMP will return -EFAULT on huge zero page */
> - page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
> + if (pmd_present(*pmd)) {
> + /* FOLL_DUMP will return -EFAULT on huge zero page */
> + page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
> + } else if (unlikely(is_swap_pmd(*pmd))) {
> + swp_entry_t entry = pmd_to_swp_entry(*pmd);
> +
> + VM_BUG_ON(!is_migration_entry(entry));
> + page = migration_entry_to_page(entry);
if (is_migration_entry(entry))
page = migration_entry_to_page(entry);
Seems safer and doesn't add much code.
> + } else {
> + return;
> + }
> if (IS_ERR_OR_NULL(page))
> return;
> if (PageAnon(page))
> @@ -578,8 +587,7 @@ static int smaps_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
>
> ptl = pmd_trans_huge_lock(pmd, vma);
> if (ptl) {
> - if (pmd_present(*pmd))
> - smaps_pmd_entry(pmd, addr, walk);
> + smaps_pmd_entry(pmd, addr, walk);
> spin_unlock(ptl);
> goto out;
> }
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] /proc/PID/smaps: Add PMD migration entry parsing
2020-03-31 9:51 ` Konstantin Khlebnikov
@ 2020-04-01 2:31 ` Huang, Ying
2020-04-01 6:03 ` Konstantin Khlebnikov
0 siblings, 1 reply; 12+ messages in thread
From: Huang, Ying @ 2020-04-01 2:31 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Andrew Morton, linux-mm, linux-kernel, Andrea Arcangeli,
Kirill A . Shutemov, Zi Yan, Vlastimil Babka, Alexey Dobriyan,
Michal Hocko, Jérôme Glisse, Yang Shi
Konstantin Khlebnikov <khlebnikov@yandex-team.ru> writes:
> On 31/03/2020 11.56, Huang, Ying wrote:
>> From: Huang Ying <ying.huang@intel.com>
>>
>> Now, when read /proc/PID/smaps, the PMD migration entry in page table is simply
>> ignored. To improve the accuracy of /proc/PID/smaps, its parsing and processing
>> is added.
>>
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> Cc: Andrea Arcangeli <aarcange@redhat.com>
>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Cc: Zi Yan <ziy@nvidia.com>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Cc: Alexey Dobriyan <adobriyan@gmail.com>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>> Cc: "Jérôme Glisse" <jglisse@redhat.com>
>> Cc: Yang Shi <yang.shi@linux.alibaba.com>
>> ---
>> fs/proc/task_mmu.c | 16 ++++++++++++----
>> 1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index 8d382d4ec067..b5b3aef8cb3b 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -548,8 +548,17 @@ static void smaps_pmd_entry(pmd_t *pmd, unsigned long addr,
>> bool locked = !!(vma->vm_flags & VM_LOCKED);
>> struct page *page;
>
> struct page *page = NULL;
Looks good. Will do this in the next version.
>> - /* FOLL_DUMP will return -EFAULT on huge zero page */
>> - page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
>> + if (pmd_present(*pmd)) {
>> + /* FOLL_DUMP will return -EFAULT on huge zero page */
>> + page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
>> + } else if (unlikely(is_swap_pmd(*pmd))) {
>> + swp_entry_t entry = pmd_to_swp_entry(*pmd);
>> +
>> + VM_BUG_ON(!is_migration_entry(entry));
>> + page = migration_entry_to_page(entry);
>
> if (is_migration_entry(entry))
> page = migration_entry_to_page(entry);
>
> Seems safer and doesn't add much code.
With this, we lose an opportunity to capture some bugs during debugging.
Right?
Best Regards,
Huang, Ying
>> + } else {
>> + return;
>> + }
>> if (IS_ERR_OR_NULL(page))
>> return;
>> if (PageAnon(page))
>> @@ -578,8 +587,7 @@ static int smaps_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
>> ptl = pmd_trans_huge_lock(pmd, vma);
>> if (ptl) {
>> - if (pmd_present(*pmd))
>> - smaps_pmd_entry(pmd, addr, walk);
>> + smaps_pmd_entry(pmd, addr, walk);
>> spin_unlock(ptl);
>> goto out;
>> }
>>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] /proc/PID/smaps: Add PMD migration entry parsing
2020-04-01 2:31 ` Huang, Ying
@ 2020-04-01 6:03 ` Konstantin Khlebnikov
2020-04-01 6:20 ` Huang, Ying
0 siblings, 1 reply; 12+ messages in thread
From: Konstantin Khlebnikov @ 2020-04-01 6:03 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Andrea Arcangeli,
Kirill A . Shutemov, Zi Yan, Vlastimil Babka, Alexey Dobriyan,
Michal Hocko, Jérôme Glisse, Yang Shi
On 01/04/2020 05.31, Huang, Ying wrote:
> Konstantin Khlebnikov <khlebnikov@yandex-team.ru> writes:
>
>> On 31/03/2020 11.56, Huang, Ying wrote:
>>> From: Huang Ying <ying.huang@intel.com>
>>>
>>> Now, when read /proc/PID/smaps, the PMD migration entry in page table is simply
>>> ignored. To improve the accuracy of /proc/PID/smaps, its parsing and processing
>>> is added.
>>>
>>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>>> Cc: Andrea Arcangeli <aarcange@redhat.com>
>>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>> Cc: Zi Yan <ziy@nvidia.com>
>>> Cc: Vlastimil Babka <vbabka@suse.cz>
>>> Cc: Alexey Dobriyan <adobriyan@gmail.com>
>>> Cc: Michal Hocko <mhocko@suse.com>
>>> Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>>> Cc: "Jérôme Glisse" <jglisse@redhat.com>
>>> Cc: Yang Shi <yang.shi@linux.alibaba.com>
>>> ---
>>> fs/proc/task_mmu.c | 16 ++++++++++++----
>>> 1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>>> index 8d382d4ec067..b5b3aef8cb3b 100644
>>> --- a/fs/proc/task_mmu.c
>>> +++ b/fs/proc/task_mmu.c
>>> @@ -548,8 +548,17 @@ static void smaps_pmd_entry(pmd_t *pmd, unsigned long addr,
>>> bool locked = !!(vma->vm_flags & VM_LOCKED);
>>> struct page *page;
>>
>> struct page *page = NULL;
>
> Looks good. Will do this in the next version.
>
>>> - /* FOLL_DUMP will return -EFAULT on huge zero page */
>>> - page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
>>> + if (pmd_present(*pmd)) {
>>> + /* FOLL_DUMP will return -EFAULT on huge zero page */
>>> + page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
>>> + } else if (unlikely(is_swap_pmd(*pmd))) {
>>> + swp_entry_t entry = pmd_to_swp_entry(*pmd);
>>> +
>>> + VM_BUG_ON(!is_migration_entry(entry));
>>> + page = migration_entry_to_page(entry);
>>
>> if (is_migration_entry(entry))
>> page = migration_entry_to_page(entry);
>>
>> Seems safer and doesn't add much code.
>
> With this, we lose an opportunity to capture some bugs during debugging.
> Right?
You can keep VM_BUG_ON or VM_WARN_ON_ONCE
Off-by-page in statistics isn't a big deal and not a good reason to crash (even debug) kernel.
But for normal build should use safe behaviour if this isn't hard.
>
> Best Regards,
> Huang, Ying
>
>>> + } else {
>>> + return;
>>> + }
>>> if (IS_ERR_OR_NULL(page))
>>> return;
>>> if (PageAnon(page))
>>> @@ -578,8 +587,7 @@ static int smaps_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
>>> ptl = pmd_trans_huge_lock(pmd, vma);
>>> if (ptl) {
>>> - if (pmd_present(*pmd))
>>> - smaps_pmd_entry(pmd, addr, walk);
>>> + smaps_pmd_entry(pmd, addr, walk);
>>> spin_unlock(ptl);
>>> goto out;
>>> }
>>>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] /proc/PID/smaps: Add PMD migration entry parsing
2020-04-01 6:03 ` Konstantin Khlebnikov
@ 2020-04-01 6:20 ` Huang, Ying
0 siblings, 0 replies; 12+ messages in thread
From: Huang, Ying @ 2020-04-01 6:20 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Andrew Morton, linux-mm, linux-kernel, Andrea Arcangeli,
Kirill A . Shutemov, Zi Yan, Vlastimil Babka, Alexey Dobriyan,
Michal Hocko, Jérôme Glisse, Yang Shi
Konstantin Khlebnikov <khlebnikov@yandex-team.ru> writes:
> On 01/04/2020 05.31, Huang, Ying wrote:
>> Konstantin Khlebnikov <khlebnikov@yandex-team.ru> writes:
>>
>>> On 31/03/2020 11.56, Huang, Ying wrote:
>>>> From: Huang Ying <ying.huang@intel.com>
>>>>
>>>> Now, when read /proc/PID/smaps, the PMD migration entry in page table is simply
>>>> ignored. To improve the accuracy of /proc/PID/smaps, its parsing and processing
>>>> is added.
>>>>
>>>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>>>> Cc: Andrea Arcangeli <aarcange@redhat.com>
>>>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>> Cc: Zi Yan <ziy@nvidia.com>
>>>> Cc: Vlastimil Babka <vbabka@suse.cz>
>>>> Cc: Alexey Dobriyan <adobriyan@gmail.com>
>>>> Cc: Michal Hocko <mhocko@suse.com>
>>>> Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>>>> Cc: "Jérôme Glisse" <jglisse@redhat.com>
>>>> Cc: Yang Shi <yang.shi@linux.alibaba.com>
>>>> ---
>>>> fs/proc/task_mmu.c | 16 ++++++++++++----
>>>> 1 file changed, 12 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>>>> index 8d382d4ec067..b5b3aef8cb3b 100644
>>>> --- a/fs/proc/task_mmu.c
>>>> +++ b/fs/proc/task_mmu.c
>>>> @@ -548,8 +548,17 @@ static void smaps_pmd_entry(pmd_t *pmd, unsigned long addr,
>>>> bool locked = !!(vma->vm_flags & VM_LOCKED);
>>>> struct page *page;
>>>
>>> struct page *page = NULL;
>>
>> Looks good. Will do this in the next version.
>>
>>>> - /* FOLL_DUMP will return -EFAULT on huge zero page */
>>>> - page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
>>>> + if (pmd_present(*pmd)) {
>>>> + /* FOLL_DUMP will return -EFAULT on huge zero page */
>>>> + page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
>>>> + } else if (unlikely(is_swap_pmd(*pmd))) {
>>>> + swp_entry_t entry = pmd_to_swp_entry(*pmd);
>>>> +
>>>> + VM_BUG_ON(!is_migration_entry(entry));
>>>> + page = migration_entry_to_page(entry);
>>>
>>> if (is_migration_entry(entry))
>>> page = migration_entry_to_page(entry);
>>>
>>> Seems safer and doesn't add much code.
>>
>> With this, we lose an opportunity to capture some bugs during debugging.
>> Right?
>
> You can keep VM_BUG_ON or VM_WARN_ON_ONCE
>
> Off-by-page in statistics isn't a big deal and not a good reason to crash (even debug) kernel.
> But for normal build should use safe behaviour if this isn't hard.
Sounds reasonable! Will revise the code. Thanks!
Best Regards,
Huang, Ying
>>
>> Best Regards,
>> Huang, Ying
>>
>>>> + } else {
>>>> + return;
>>>> + }
>>>> if (IS_ERR_OR_NULL(page))
>>>> return;
>>>> if (PageAnon(page))
>>>> @@ -578,8 +587,7 @@ static int smaps_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
>>>> ptl = pmd_trans_huge_lock(pmd, vma);
>>>> if (ptl) {
>>>> - if (pmd_present(*pmd))
>>>> - smaps_pmd_entry(pmd, addr, walk);
>>>> + smaps_pmd_entry(pmd, addr, walk);
>>>> spin_unlock(ptl);
>>>> goto out;
>>>> }
>>>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] /proc/PID/smaps: Add PMD migration entry parsing
2020-03-31 8:56 [PATCH] /proc/PID/smaps: Add PMD migration entry parsing Huang, Ying
2020-03-31 9:51 ` Konstantin Khlebnikov
@ 2020-03-31 12:24 ` Zi Yan
2020-04-01 2:24 ` Huang, Ying
2020-04-02 1:49 ` Huang, Ying
2020-04-01 23:04 ` Andrew Morton
2 siblings, 2 replies; 12+ messages in thread
From: Zi Yan @ 2020-03-31 12:24 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Andrea Arcangeli,
Kirill A . Shutemov, Vlastimil Babka, Alexey Dobriyan,
Michal Hocko, Konstantin Khlebnikov, Jérôme Glisse,
Yang Shi
[-- Attachment #1: Type: text/plain, Size: 2834 bytes --]
On 31 Mar 2020, at 4:56, Huang, Ying wrote:
>
> From: Huang Ying <ying.huang@intel.com>
>
> Now, when read /proc/PID/smaps, the PMD migration entry in page table is simply
> ignored. To improve the accuracy of /proc/PID/smaps, its parsing and processing
> is added.
>
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Cc: "Jérôme Glisse" <jglisse@redhat.com>
> Cc: Yang Shi <yang.shi@linux.alibaba.com>
> ---
> fs/proc/task_mmu.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 8d382d4ec067..b5b3aef8cb3b 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -548,8 +548,17 @@ static void smaps_pmd_entry(pmd_t *pmd, unsigned long addr,
> bool locked = !!(vma->vm_flags & VM_LOCKED);
> struct page *page;
Like Konstantin pointed out in another email, you could initialize page to NULL here.
Plus you do not need the “else-return” below, if you do that.
>
> - /* FOLL_DUMP will return -EFAULT on huge zero page */
> - page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
> + if (pmd_present(*pmd)) {
> + /* FOLL_DUMP will return -EFAULT on huge zero page */
> + page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
> + } else if (unlikely(is_swap_pmd(*pmd))) {
Should be:
} else if (unlikely(thp_migration_support() && is_swap_pmd(*pmd))) {
Otherwise, when THP migration is disabled and the PMD is under splitting, VM_BUG_ON
will be triggered.
> + swp_entry_t entry = pmd_to_swp_entry(*pmd);
> +
> + VM_BUG_ON(!is_migration_entry(entry));
> + page = migration_entry_to_page(entry);
> + } else {
> + return;
> + }
> if (IS_ERR_OR_NULL(page))
> return;
> if (PageAnon(page))
> @@ -578,8 +587,7 @@ static int smaps_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
>
> ptl = pmd_trans_huge_lock(pmd, vma);
> if (ptl) {
> - if (pmd_present(*pmd))
> - smaps_pmd_entry(pmd, addr, walk);
> + smaps_pmd_entry(pmd, addr, walk);
> spin_unlock(ptl);
> goto out;
> }
> --
> 2.25.0
Everything else looks good to me. Thanks.
With the fixes mentioned above, you can add
Reviewed-by: Zi Yan <ziy@nvidia.com>
—
Best Regards,
Yan Zi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] /proc/PID/smaps: Add PMD migration entry parsing
2020-03-31 12:24 ` Zi Yan
@ 2020-04-01 2:24 ` Huang, Ying
2020-04-01 2:42 ` Zi Yan
2020-04-02 1:49 ` Huang, Ying
1 sibling, 1 reply; 12+ messages in thread
From: Huang, Ying @ 2020-04-01 2:24 UTC (permalink / raw)
To: Zi Yan
Cc: Andrew Morton, linux-mm, linux-kernel, Andrea Arcangeli,
Kirill A . Shutemov, Vlastimil Babka, Alexey Dobriyan,
Michal Hocko, Konstantin Khlebnikov, Jérôme Glisse,
Yang Shi
Zi Yan <ziy@nvidia.com> writes:
> On 31 Mar 2020, at 4:56, Huang, Ying wrote:
>>
>> From: Huang Ying <ying.huang@intel.com>
>>
>> Now, when read /proc/PID/smaps, the PMD migration entry in page table is simply
>> ignored. To improve the accuracy of /proc/PID/smaps, its parsing and processing
>> is added.
>>
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> Cc: Andrea Arcangeli <aarcange@redhat.com>
>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Cc: Zi Yan <ziy@nvidia.com>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Cc: Alexey Dobriyan <adobriyan@gmail.com>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>> Cc: "Jérôme Glisse" <jglisse@redhat.com>
>> Cc: Yang Shi <yang.shi@linux.alibaba.com>
>> ---
>> fs/proc/task_mmu.c | 16 ++++++++++++----
>> 1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index 8d382d4ec067..b5b3aef8cb3b 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -548,8 +548,17 @@ static void smaps_pmd_entry(pmd_t *pmd, unsigned long addr,
>> bool locked = !!(vma->vm_flags & VM_LOCKED);
>> struct page *page;
>
> Like Konstantin pointed out in another email, you could initialize page to NULL here.
> Plus you do not need the “else-return” below, if you do that.
Yes. That looks better. Will change this in the next version.
>>
>> - /* FOLL_DUMP will return -EFAULT on huge zero page */
>> - page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
>> + if (pmd_present(*pmd)) {
>> + /* FOLL_DUMP will return -EFAULT on huge zero page */
>> + page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
>> + } else if (unlikely(is_swap_pmd(*pmd))) {
>
> Should be:
> } else if (unlikely(thp_migration_support() && is_swap_pmd(*pmd))) {
>
> Otherwise, when THP migration is disabled and the PMD is under splitting, VM_BUG_ON
> will be triggered.
We hold the PMD page table lock when call smaps_pmd_entry(). How does
PMD splitting trigger VM_BUG_ON()?
>> + swp_entry_t entry = pmd_to_swp_entry(*pmd);
>> +
>> + VM_BUG_ON(!is_migration_entry(entry));
>> + page = migration_entry_to_page(entry);
>> + } else {
>> + return;
>> + }
>> if (IS_ERR_OR_NULL(page))
>> return;
>> if (PageAnon(page))
>> @@ -578,8 +587,7 @@ static int smaps_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
>>
>> ptl = pmd_trans_huge_lock(pmd, vma);
>> if (ptl) {
>> - if (pmd_present(*pmd))
>> - smaps_pmd_entry(pmd, addr, walk);
>> + smaps_pmd_entry(pmd, addr, walk);
>> spin_unlock(ptl);
>> goto out;
>> }
>> --
>> 2.25.0
>
> Everything else looks good to me. Thanks.
>
> With the fixes mentioned above, you can add
> Reviewed-by: Zi Yan <ziy@nvidia.com>
Thanks!
Best Regards,
Huang, Ying
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] /proc/PID/smaps: Add PMD migration entry parsing
2020-04-01 2:24 ` Huang, Ying
@ 2020-04-01 2:42 ` Zi Yan
0 siblings, 0 replies; 12+ messages in thread
From: Zi Yan @ 2020-04-01 2:42 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Andrea Arcangeli,
Kirill A . Shutemov, Vlastimil Babka, Alexey Dobriyan,
Michal Hocko, Konstantin Khlebnikov, Jérôme Glisse,
Yang Shi
[-- Attachment #1: Type: text/plain, Size: 2470 bytes --]
On 31 Mar 2020, at 22:24, Huang, Ying wrote:
> External email: Use caution opening links or attachments
>
>
> Zi Yan <ziy@nvidia.com> writes:
>
>> On 31 Mar 2020, at 4:56, Huang, Ying wrote:
>>>
>>> From: Huang Ying <ying.huang@intel.com>
>>>
>>> Now, when read /proc/PID/smaps, the PMD migration entry in page table is simply
>>> ignored. To improve the accuracy of /proc/PID/smaps, its parsing and processing
>>> is added.
>>>
>>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>>> Cc: Andrea Arcangeli <aarcange@redhat.com>
>>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>> Cc: Zi Yan <ziy@nvidia.com>
>>> Cc: Vlastimil Babka <vbabka@suse.cz>
>>> Cc: Alexey Dobriyan <adobriyan@gmail.com>
>>> Cc: Michal Hocko <mhocko@suse.com>
>>> Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>>> Cc: "Jérôme Glisse" <jglisse@redhat.com>
>>> Cc: Yang Shi <yang.shi@linux.alibaba.com>
>>> ---
>>> fs/proc/task_mmu.c | 16 ++++++++++++----
>>> 1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>>> index 8d382d4ec067..b5b3aef8cb3b 100644
>>> --- a/fs/proc/task_mmu.c
>>> +++ b/fs/proc/task_mmu.c
>>> @@ -548,8 +548,17 @@ static void smaps_pmd_entry(pmd_t *pmd, unsigned long addr,
>>> bool locked = !!(vma->vm_flags & VM_LOCKED);
>>> struct page *page;
>>
>> Like Konstantin pointed out in another email, you could initialize page to NULL here.
>> Plus you do not need the “else-return” below, if you do that.
>
> Yes. That looks better. Will change this in the next version.
Thanks.
>
>>>
>>> - /* FOLL_DUMP will return -EFAULT on huge zero page */
>>> - page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
>>> + if (pmd_present(*pmd)) {
>>> + /* FOLL_DUMP will return -EFAULT on huge zero page */
>>> + page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
>>> + } else if (unlikely(is_swap_pmd(*pmd))) {
>>
>> Should be:
>> } else if (unlikely(thp_migration_support() && is_swap_pmd(*pmd))) {
>>
>> Otherwise, when THP migration is disabled and the PMD is under splitting, VM_BUG_ON
>> will be triggered.
>
> We hold the PMD page table lock when call smaps_pmd_entry(). How does
> PMD splitting trigger VM_BUG_ON()?
Oh, I missed that. Your original code is right. Thank you for the explanation.
—
Best Regards,
Yan Zi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] /proc/PID/smaps: Add PMD migration entry parsing
2020-03-31 12:24 ` Zi Yan
2020-04-01 2:24 ` Huang, Ying
@ 2020-04-02 1:49 ` Huang, Ying
1 sibling, 0 replies; 12+ messages in thread
From: Huang, Ying @ 2020-04-02 1:49 UTC (permalink / raw)
To: Zi Yan
Cc: Andrew Morton, linux-mm, linux-kernel, Andrea Arcangeli,
Kirill A . Shutemov, Vlastimil Babka, Alexey Dobriyan,
Michal Hocko, Konstantin Khlebnikov, Jérôme Glisse,
Yang Shi
Zi Yan <ziy@nvidia.com> writes:
> On 31 Mar 2020, at 4:56, Huang, Ying wrote:
>>
>> From: Huang Ying <ying.huang@intel.com>
>> - /* FOLL_DUMP will return -EFAULT on huge zero page */
>> - page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
>> + if (pmd_present(*pmd)) {
>> + /* FOLL_DUMP will return -EFAULT on huge zero page */
>> + page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
>> + } else if (unlikely(is_swap_pmd(*pmd))) {
>
> Should be:
> } else if (unlikely(thp_migration_support() && is_swap_pmd(*pmd))) {
Thought this again. This can reduce the code size if
thp_migration_support() isn't enabled. I will do this in the next
version.
Best Regards,
Huang, Ying
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] /proc/PID/smaps: Add PMD migration entry parsing
2020-03-31 8:56 [PATCH] /proc/PID/smaps: Add PMD migration entry parsing Huang, Ying
2020-03-31 9:51 ` Konstantin Khlebnikov
2020-03-31 12:24 ` Zi Yan
@ 2020-04-01 23:04 ` Andrew Morton
2020-04-02 1:42 ` Huang, Ying
2020-04-02 6:27 ` Michal Hocko
2 siblings, 2 replies; 12+ messages in thread
From: Andrew Morton @ 2020-04-01 23:04 UTC (permalink / raw)
To: Huang, Ying
Cc: linux-mm, linux-kernel, Andrea Arcangeli, Kirill A . Shutemov,
Zi Yan, Vlastimil Babka, Alexey Dobriyan, Michal Hocko,
Konstantin Khlebnikov, Jérôme Glisse, Yang Shi
On Tue, 31 Mar 2020 16:56:04 +0800 "Huang, Ying" <ying.huang@intel.com> wrote:
> From: Huang Ying <ying.huang@intel.com>
>
> Now, when read /proc/PID/smaps, the PMD migration entry in page table is simply
> ignored. To improve the accuracy of /proc/PID/smaps, its parsing and processing
> is added.
It would be helpful to show the before-and-after output in the changelog.
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -548,8 +548,17 @@ static void smaps_pmd_entry(pmd_t *pmd, unsigned long addr,
> bool locked = !!(vma->vm_flags & VM_LOCKED);
> struct page *page;
>
> - /* FOLL_DUMP will return -EFAULT on huge zero page */
> - page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
> + if (pmd_present(*pmd)) {
> + /* FOLL_DUMP will return -EFAULT on huge zero page */
> + page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
> + } else if (unlikely(is_swap_pmd(*pmd))) {
> + swp_entry_t entry = pmd_to_swp_entry(*pmd);
> +
> + VM_BUG_ON(!is_migration_entry(entry));
I don't think this justifies nuking the kernel. A
WARN()-and-recover would be better.
> + page = migration_entry_to_page(entry);
> + } else {
> + return;
> + }
> if (IS_ERR_OR_NULL(page))
> return;
> if (PageAnon(page))
> @@ -578,8 +587,7 @@ static int smaps_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
>
> ptl = pmd_trans_huge_lock(pmd, vma);
> if (ptl) {
> - if (pmd_present(*pmd))
> - smaps_pmd_entry(pmd, addr, walk);
> + smaps_pmd_entry(pmd, addr, walk);
> spin_unlock(ptl);
> goto out;
> }
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] /proc/PID/smaps: Add PMD migration entry parsing
2020-04-01 23:04 ` Andrew Morton
@ 2020-04-02 1:42 ` Huang, Ying
2020-04-02 6:27 ` Michal Hocko
1 sibling, 0 replies; 12+ messages in thread
From: Huang, Ying @ 2020-04-02 1:42 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, linux-kernel, Andrea Arcangeli, Kirill A . Shutemov,
Zi Yan, Vlastimil Babka, Alexey Dobriyan, Michal Hocko,
Konstantin Khlebnikov, jglisse, Yang Shi
Andrew Morton <akpm@linux-foundation.org> writes:
> On Tue, 31 Mar 2020 16:56:04 +0800 "Huang, Ying" <ying.huang@intel.com> wrote:
>
>> From: Huang Ying <ying.huang@intel.com>
>>
>> Now, when read /proc/PID/smaps, the PMD migration entry in page table is simply
>> ignored. To improve the accuracy of /proc/PID/smaps, its parsing and processing
>> is added.
>
> It would be helpful to show the before-and-after output in the changelog.
OK.
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -548,8 +548,17 @@ static void smaps_pmd_entry(pmd_t *pmd, unsigned long addr,
>> bool locked = !!(vma->vm_flags & VM_LOCKED);
>> struct page *page;
>>
>> - /* FOLL_DUMP will return -EFAULT on huge zero page */
>> - page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
>> + if (pmd_present(*pmd)) {
>> + /* FOLL_DUMP will return -EFAULT on huge zero page */
>> + page = follow_trans_huge_pmd(vma, addr, pmd, FOLL_DUMP);
>> + } else if (unlikely(is_swap_pmd(*pmd))) {
>> + swp_entry_t entry = pmd_to_swp_entry(*pmd);
>> +
>> + VM_BUG_ON(!is_migration_entry(entry));
>
> I don't think this justifies nuking the kernel. A
> WARN()-and-recover would be better.
Yes. Will change this in the next version.
Best Regards,
Huang, Ying
>> + page = migration_entry_to_page(entry);
>> + } else {
>> + return;
>> + }
>> if (IS_ERR_OR_NULL(page))
>> return;
>> if (PageAnon(page))
>> @@ -578,8 +587,7 @@ static int smaps_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
>>
>> ptl = pmd_trans_huge_lock(pmd, vma);
>> if (ptl) {
>> - if (pmd_present(*pmd))
>> - smaps_pmd_entry(pmd, addr, walk);
>> + smaps_pmd_entry(pmd, addr, walk);
>> spin_unlock(ptl);
>> goto out;
>> }
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] /proc/PID/smaps: Add PMD migration entry parsing
2020-04-01 23:04 ` Andrew Morton
2020-04-02 1:42 ` Huang, Ying
@ 2020-04-02 6:27 ` Michal Hocko
1 sibling, 0 replies; 12+ messages in thread
From: Michal Hocko @ 2020-04-02 6:27 UTC (permalink / raw)
To: Andrew Morton
Cc: Huang, Ying, linux-mm, linux-kernel, Andrea Arcangeli,
Kirill A . Shutemov, Zi Yan, Vlastimil Babka, Alexey Dobriyan,
Konstantin Khlebnikov, Jérôme Glisse, Yang Shi
On Wed 01-04-20 16:04:32, Andrew Morton wrote:
> On Tue, 31 Mar 2020 16:56:04 +0800 "Huang, Ying" <ying.huang@intel.com> wrote:
>
> > From: Huang Ying <ying.huang@intel.com>
> >
> > Now, when read /proc/PID/smaps, the PMD migration entry in page table is simply
> > ignored. To improve the accuracy of /proc/PID/smaps, its parsing and processing
> > is added.
>
> It would be helpful to show the before-and-after output in the changelog.
Migration entries are ephemeral. Is this observable in practice? I
suspect this is just primarily motivated by reading the code more than
hitting the actual problem.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 12+ messages in thread