Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1531442 > unrolled thread
| Started by | Dave Hansen <dave@sr71.net> |
|---|---|
| First post | 2016-11-28 18:00 +0100 |
| Last post | 2016-11-29 16:20 +0100 |
| Articles | 5 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] proc: mm: export PTE sizes directly in smaps (v2) Dave Hansen <dave@sr71.net> - 2016-11-28 18:00 +0100
Re: [PATCH] proc: mm: export PTE sizes directly in smaps (v2) Vlastimil Babka <vbabka@suse.cz> - 2016-11-28 22:10 +0100
Re: [PATCH] proc: mm: export PTE sizes directly in smaps (v2) Dave Hansen <dave@sr71.net> - 2016-11-28 22:50 +0100
Re: [PATCH] proc: mm: export PTE sizes directly in smaps (v2) Vlastimil Babka <vbabka@suse.cz> - 2016-11-29 09:10 +0100
Re: [PATCH] proc: mm: export PTE sizes directly in smaps (v2) Catalin Marinas <catalin.marinas@arm.com> - 2016-11-29 16:20 +0100
| From | Dave Hansen <dave@sr71.net> |
|---|---|
| Date | 2016-11-28 18:00 +0100 |
| Subject | Re: [PATCH] proc: mm: export PTE sizes directly in smaps (v2) |
| Message-ID | <sIxrA-4y6-29@gated-at.bofh.it> |
On 11/24/2016 06:22 AM, Vlastimil Babka wrote:
> On 11/17/2016 01:28 AM, Dave Hansen wrote:
>> @@ -702,11 +707,13 @@ static int smaps_hugetlb_range(pte_t *pt
>> }
>> if (page) {
>> int mapcount = page_mapcount(page);
>> + unsigned long hpage_size = huge_page_size(hstate_vma(vma));
>>
>> + mss->rss_pud += hpage_size;
>
> This hardcoded pud doesn't look right, doesn't the pmd/pud depend on
> hpage_size?
Urg, nope. Thanks for noticing that! I think we'll need something
along the lines of:
if (hpage_size == PUD_SIZE)
mss->rss_pud += PUD_SIZE;
else if (hpage_size == PMD_SIZE)
mss->rss_pmd += PMD_SIZE;
I'll respin and resend.
[toc] | [next] | [standalone]
| From | Vlastimil Babka <vbabka@suse.cz> |
|---|---|
| Date | 2016-11-28 22:10 +0100 |
| Message-ID | <sIBlw-7lp-35@gated-at.bofh.it> |
| In reply to | #1531442 |
On 11/28/2016 05:52 PM, Dave Hansen wrote:
> On 11/24/2016 06:22 AM, Vlastimil Babka wrote:
>> On 11/17/2016 01:28 AM, Dave Hansen wrote:
>>> @@ -702,11 +707,13 @@ static int smaps_hugetlb_range(pte_t *pt
>>> }
>>> if (page) {
>>> int mapcount = page_mapcount(page);
>>> + unsigned long hpage_size = huge_page_size(hstate_vma(vma));
>>>
>>> + mss->rss_pud += hpage_size;
>>
>> This hardcoded pud doesn't look right, doesn't the pmd/pud depend on
>> hpage_size?
>
> Urg, nope. Thanks for noticing that! I think we'll need something
> along the lines of:
>
> if (hpage_size == PUD_SIZE)
> mss->rss_pud += PUD_SIZE;
> else if (hpage_size == PMD_SIZE)
> mss->rss_pmd += PMD_SIZE;
Sounds better, although I wonder whether there are some weird arches
supporting hugepage sizes that don't match page table levels. I recall
that e.g. MIPS could do arbitrary size, but dunno if the kernel supports
that...
> I'll respin and resend.
>
[toc] | [prev] | [next] | [standalone]
| From | Dave Hansen <dave@sr71.net> |
|---|---|
| Date | 2016-11-28 22:50 +0100 |
| Message-ID | <sIBYd-7z9-1@gated-at.bofh.it> |
| In reply to | #1531666 |
... cc'ing the arm64 maintainers
On 11/28/2016 01:07 PM, Vlastimil Babka wrote:
> On 11/28/2016 05:52 PM, Dave Hansen wrote:
>> On 11/24/2016 06:22 AM, Vlastimil Babka wrote:
>>> On 11/17/2016 01:28 AM, Dave Hansen wrote:
>>>> @@ -702,11 +707,13 @@ static int smaps_hugetlb_range(pte_t *pt
>>>> }
>>>> if (page) {
>>>> int mapcount = page_mapcount(page);
>>>> + unsigned long hpage_size = huge_page_size(hstate_vma(vma));
>>>>
>>>> + mss->rss_pud += hpage_size;
>>>
>>> This hardcoded pud doesn't look right, doesn't the pmd/pud depend on
>>> hpage_size?
>>
>> Urg, nope. Thanks for noticing that! I think we'll need something
>> along the lines of:
>>
>> if (hpage_size == PUD_SIZE)
>> mss->rss_pud += PUD_SIZE;
>> else if (hpage_size == PMD_SIZE)
>> mss->rss_pmd += PMD_SIZE;
>
> Sounds better, although I wonder whether there are some weird arches
> supporting hugepage sizes that don't match page table levels. I recall
> that e.g. MIPS could do arbitrary size, but dunno if the kernel supports
> that...
arm64 seems to have pretty arbitrary sizes, and seems to be able to
build them out of multiple hardware PTE sizes. I think I can fix my
code to handle those:
if (hpage_size >= PGD_SIZE)
mss->rss_pgd += PGD_SIZE;
else if (hpage_size >= PUD_SIZE)
mss->rss_pud += PUD_SIZE;
else if (hpage_size >= PMD_SIZE)
mss->rss_pmd += PMD_SIZE;
else
mss->rss_pte += PAGE_SIZE;
But, I *think* that means that smaps_hugetlb_range() is *currently*
broken for these intermediate arm64 sizes. The code does:
if (mapcount >= 2)
mss->shared_hugetlb += hpage_size;
else
mss->private_hugetlb += hpage_size;
So I *think* if we may count a hugetlbfs arm64 CONT_PTES page multiple
times, and account hpage_size for *each* of the CONT_PTES. That would
artificially inflate the smaps output for those pages.
Will / Catalin, is there something I'm missing?
[toc] | [prev] | [next] | [standalone]
| From | Vlastimil Babka <vbabka@suse.cz> |
|---|---|
| Date | 2016-11-29 09:10 +0100 |
| Message-ID | <sILEd-5HY-1@gated-at.bofh.it> |
| In reply to | #1531692 |
On 11/28/2016 10:39 PM, Dave Hansen wrote:
> ... cc'ing the arm64 maintainers
>
> On 11/28/2016 01:07 PM, Vlastimil Babka wrote:
>> On 11/28/2016 05:52 PM, Dave Hansen wrote:
>>> On 11/24/2016 06:22 AM, Vlastimil Babka wrote:
>>>> On 11/17/2016 01:28 AM, Dave Hansen wrote:
>>>>> @@ -702,11 +707,13 @@ static int smaps_hugetlb_range(pte_t *pt
>>>>> }
>>>>> if (page) {
>>>>> int mapcount = page_mapcount(page);
>>>>> + unsigned long hpage_size = huge_page_size(hstate_vma(vma));
>>>>>
>>>>> + mss->rss_pud += hpage_size;
>>>>
>>>> This hardcoded pud doesn't look right, doesn't the pmd/pud depend on
>>>> hpage_size?
>>>
>>> Urg, nope. Thanks for noticing that! I think we'll need something
>>> along the lines of:
>>>
>>> if (hpage_size == PUD_SIZE)
>>> mss->rss_pud += PUD_SIZE;
>>> else if (hpage_size == PMD_SIZE)
>>> mss->rss_pmd += PMD_SIZE;
>>
>> Sounds better, although I wonder whether there are some weird arches
>> supporting hugepage sizes that don't match page table levels. I recall
>> that e.g. MIPS could do arbitrary size, but dunno if the kernel supports
>> that...
>
> arm64 seems to have pretty arbitrary sizes, and seems to be able to
> build them out of multiple hardware PTE sizes. I think I can fix my
> code to handle those:
>
> if (hpage_size >= PGD_SIZE)
> mss->rss_pgd += PGD_SIZE;
> else if (hpage_size >= PUD_SIZE)
> mss->rss_pud += PUD_SIZE;
> else if (hpage_size >= PMD_SIZE)
> mss->rss_pmd += PMD_SIZE;
> else
> mss->rss_pte += PAGE_SIZE;
>
> But, I *think* that means that smaps_hugetlb_range() is *currently*
> broken for these intermediate arm64 sizes. The code does:
>
> if (mapcount >= 2)
> mss->shared_hugetlb += hpage_size;
> else
> mss->private_hugetlb += hpage_size;
>
> So I *think* if we may count a hugetlbfs arm64 CONT_PTES page multiple
> times, and account hpage_size for *each* of the CONT_PTES. That would
> artificially inflate the smaps output for those pages.
Hmm IIUC walk_hugetlb_range() will call the smaps_hugetlb_range()
callback once per hugepage, not once per "pte", no? See
hugetlb_entry_end(). In that case the current code should be OK and
yours would undercount?
> Will / Catalin, is there something I'm missing?
>
[toc] | [prev] | [next] | [standalone]
| From | Catalin Marinas <catalin.marinas@arm.com> |
|---|---|
| Date | 2016-11-29 16:20 +0100 |
| Message-ID | <sISmm-1He-7@gated-at.bofh.it> |
| In reply to | #1531692 |
On Mon, Nov 28, 2016 at 01:39:49PM -0800, Dave Hansen wrote:
> On 11/28/2016 01:07 PM, Vlastimil Babka wrote:
> > On 11/28/2016 05:52 PM, Dave Hansen wrote:
> >> On 11/24/2016 06:22 AM, Vlastimil Babka wrote:
> >>> On 11/17/2016 01:28 AM, Dave Hansen wrote:
> >>>> @@ -702,11 +707,13 @@ static int smaps_hugetlb_range(pte_t *pt
> >>>> }
> >>>> if (page) {
> >>>> int mapcount = page_mapcount(page);
> >>>> + unsigned long hpage_size = huge_page_size(hstate_vma(vma));
> >>>>
> >>>> + mss->rss_pud += hpage_size;
> >>>
> >>> This hardcoded pud doesn't look right, doesn't the pmd/pud depend on
> >>> hpage_size?
> >>
> >> Urg, nope. Thanks for noticing that! I think we'll need something
> >> along the lines of:
> >>
> >> if (hpage_size == PUD_SIZE)
> >> mss->rss_pud += PUD_SIZE;
> >> else if (hpage_size == PMD_SIZE)
> >> mss->rss_pmd += PMD_SIZE;
> >
> > Sounds better, although I wonder whether there are some weird arches
> > supporting hugepage sizes that don't match page table levels. I recall
> > that e.g. MIPS could do arbitrary size, but dunno if the kernel supports
> > that...
>
> arm64 seems to have pretty arbitrary sizes, and seems to be able to
> build them out of multiple hardware PTE sizes. I think I can fix my
> code to handle those:
>
> if (hpage_size >= PGD_SIZE)
> mss->rss_pgd += PGD_SIZE;
> else if (hpage_size >= PUD_SIZE)
> mss->rss_pud += PUD_SIZE;
> else if (hpage_size >= PMD_SIZE)
> mss->rss_pmd += PMD_SIZE;
> else
> mss->rss_pte += PAGE_SIZE;
>
> But, I *think* that means that smaps_hugetlb_range() is *currently*
> broken for these intermediate arm64 sizes. The code does:
>
> if (mapcount >= 2)
> mss->shared_hugetlb += hpage_size;
> else
> mss->private_hugetlb += hpage_size;
>
> So I *think* if we may count a hugetlbfs arm64 CONT_PTES page multiple
> times, and account hpage_size for *each* of the CONT_PTES. That would
> artificially inflate the smaps output for those pages.
I don't think it would count them multiple times. As Vlastimil
mentioned, huge_page_size() would return (CONT_PTES * PAGE_SIZE) in such
case, so walk_hugetlb_range() skips the intermediate ptes. In general,
we try to keep the contiguous pte/pmd support visible only to the arm64
hugetlb code and hidden to the core code.
--
Catalin
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web