2019-05-10 00:18:12

by Yang Shi

[permalink] [raw]
Subject: [PATCH] mm: vmscan: correct nr_reclaimed for THP

Since commit bd4c82c22c36 ("mm, THP, swap: delay splitting THP after
swapped out"), THP can be swapped out in a whole. But, nr_reclaimed
still gets inc'ed by one even though a whole THP (512 pages) gets
swapped out.

This doesn't make too much sense to memory reclaim. For example, direct
reclaim may just need reclaim SWAP_CLUSTER_MAX pages, reclaiming one THP
could fulfill it. But, if nr_reclaimed is not increased correctly,
direct reclaim may just waste time to reclaim more pages,
SWAP_CLUSTER_MAX * 512 pages in worst case.

This change may result in more reclaimed pages than scanned pages showed
by /proc/vmstat since scanning one head page would reclaim 512 base pages.

Cc: "Huang, Ying" <[email protected]>
Cc: Johannes Weiner <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Mel Gorman <[email protected]>
Cc: "Kirill A . Shutemov" <[email protected]>
Cc: Hugh Dickins <[email protected]>
Signed-off-by: Yang Shi <[email protected]>
---
I'm not quite sure if it was the intended behavior or just omission. I tried
to dig into the review history, but didn't find any clue. I may miss some
discussion.

mm/vmscan.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index fd9de50..7e026ec 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1446,7 +1446,11 @@ static unsigned long shrink_page_list(struct list_head *page_list,

unlock_page(page);
free_it:
- nr_reclaimed++;
+ /*
+ * THP may get swapped out in a whole, need account
+ * all base pages.
+ */
+ nr_reclaimed += (1 << compound_order(page));

/*
* Is there need to periodically free_page_list? It would
--
1.8.3.1


2019-05-10 00:42:08

by Shakeel Butt

[permalink] [raw]
Subject: Re: [PATCH] mm: vmscan: correct nr_reclaimed for THP

From: Yang Shi <[email protected]>
Date: Thu, May 9, 2019 at 5:16 PM
To: <[email protected]>, <[email protected]>, <[email protected]>,
<[email protected]>, <[email protected]>,
<[email protected]>, <[email protected]>
Cc: <[email protected]>, <[email protected]>,
<[email protected]>

> Since commit bd4c82c22c36 ("mm, THP, swap: delay splitting THP after
> swapped out"), THP can be swapped out in a whole. But, nr_reclaimed
> still gets inc'ed by one even though a whole THP (512 pages) gets
> swapped out.
>
> This doesn't make too much sense to memory reclaim. For example, direct
> reclaim may just need reclaim SWAP_CLUSTER_MAX pages, reclaiming one THP
> could fulfill it. But, if nr_reclaimed is not increased correctly,
> direct reclaim may just waste time to reclaim more pages,
> SWAP_CLUSTER_MAX * 512 pages in worst case.
>
> This change may result in more reclaimed pages than scanned pages showed
> by /proc/vmstat since scanning one head page would reclaim 512 base pages.
>
> Cc: "Huang, Ying" <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Mel Gorman <[email protected]>
> Cc: "Kirill A . Shutemov" <[email protected]>
> Cc: Hugh Dickins <[email protected]>
> Signed-off-by: Yang Shi <[email protected]>

Nice find.

Reviewed-by: Shakeel Butt <[email protected]>

> ---
> I'm not quite sure if it was the intended behavior or just omission. I tried
> to dig into the review history, but didn't find any clue. I may miss some
> discussion.
>
> mm/vmscan.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index fd9de50..7e026ec 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1446,7 +1446,11 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>
> unlock_page(page);
> free_it:
> - nr_reclaimed++;
> + /*
> + * THP may get swapped out in a whole, need account
> + * all base pages.
> + */
> + nr_reclaimed += (1 << compound_order(page));
>
> /*
> * Is there need to periodically free_page_list? It would
> --
> 1.8.3.1
>

2019-05-10 02:16:07

by Huang, Ying

[permalink] [raw]
Subject: Re: [PATCH] mm: vmscan: correct nr_reclaimed for THP

Yang Shi <[email protected]> writes:

> Since commit bd4c82c22c36 ("mm, THP, swap: delay splitting THP after
> swapped out"), THP can be swapped out in a whole. But, nr_reclaimed
> still gets inc'ed by one even though a whole THP (512 pages) gets
> swapped out.
>
> This doesn't make too much sense to memory reclaim. For example, direct
> reclaim may just need reclaim SWAP_CLUSTER_MAX pages, reclaiming one THP
> could fulfill it. But, if nr_reclaimed is not increased correctly,
> direct reclaim may just waste time to reclaim more pages,
> SWAP_CLUSTER_MAX * 512 pages in worst case.
>
> This change may result in more reclaimed pages than scanned pages showed
> by /proc/vmstat since scanning one head page would reclaim 512 base pages.
>
> Cc: "Huang, Ying" <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Mel Gorman <[email protected]>
> Cc: "Kirill A . Shutemov" <[email protected]>
> Cc: Hugh Dickins <[email protected]>
> Signed-off-by: Yang Shi <[email protected]>
> ---
> I'm not quite sure if it was the intended behavior or just omission. I tried
> to dig into the review history, but didn't find any clue. I may miss some
> discussion.
>
> mm/vmscan.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index fd9de50..7e026ec 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1446,7 +1446,11 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>
> unlock_page(page);
> free_it:
> - nr_reclaimed++;
> + /*
> + * THP may get swapped out in a whole, need account
> + * all base pages.
> + */
> + nr_reclaimed += (1 << compound_order(page));
>
> /*
> * Is there need to periodically free_page_list? It would

Good catch! Thanks!

How about to change this to


nr_reclaimed += hpage_nr_pages(page);

Best Regards,
Huang, Ying

2019-05-10 02:50:01

by Yang Shi

[permalink] [raw]
Subject: Re: [PATCH] mm: vmscan: correct nr_reclaimed for THP



On 5/9/19 7:12 PM, Huang, Ying wrote:
> Yang Shi <[email protected]> writes:
>
>> Since commit bd4c82c22c36 ("mm, THP, swap: delay splitting THP after
>> swapped out"), THP can be swapped out in a whole. But, nr_reclaimed
>> still gets inc'ed by one even though a whole THP (512 pages) gets
>> swapped out.
>>
>> This doesn't make too much sense to memory reclaim. For example, direct
>> reclaim may just need reclaim SWAP_CLUSTER_MAX pages, reclaiming one THP
>> could fulfill it. But, if nr_reclaimed is not increased correctly,
>> direct reclaim may just waste time to reclaim more pages,
>> SWAP_CLUSTER_MAX * 512 pages in worst case.
>>
>> This change may result in more reclaimed pages than scanned pages showed
>> by /proc/vmstat since scanning one head page would reclaim 512 base pages.
>>
>> Cc: "Huang, Ying" <[email protected]>
>> Cc: Johannes Weiner <[email protected]>
>> Cc: Michal Hocko <[email protected]>
>> Cc: Mel Gorman <[email protected]>
>> Cc: "Kirill A . Shutemov" <[email protected]>
>> Cc: Hugh Dickins <[email protected]>
>> Signed-off-by: Yang Shi <[email protected]>
>> ---
>> I'm not quite sure if it was the intended behavior or just omission. I tried
>> to dig into the review history, but didn't find any clue. I may miss some
>> discussion.
>>
>> mm/vmscan.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index fd9de50..7e026ec 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -1446,7 +1446,11 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>>
>> unlock_page(page);
>> free_it:
>> - nr_reclaimed++;
>> + /*
>> + * THP may get swapped out in a whole, need account
>> + * all base pages.
>> + */
>> + nr_reclaimed += (1 << compound_order(page));
>>
>> /*
>> * Is there need to periodically free_page_list? It would
> Good catch! Thanks!
>
> How about to change this to
>
>
> nr_reclaimed += hpage_nr_pages(page);

Either is fine to me. Is this faster than "1 << compound_order(page)"?

>
> Best Regards,
> Huang, Ying

2019-05-10 03:05:44

by Huang, Ying

[permalink] [raw]
Subject: Re: [PATCH] mm: vmscan: correct nr_reclaimed for THP

Yang Shi <[email protected]> writes:

> On 5/9/19 7:12 PM, Huang, Ying wrote:
>> Yang Shi <[email protected]> writes:
>>
>>> Since commit bd4c82c22c36 ("mm, THP, swap: delay splitting THP after
>>> swapped out"), THP can be swapped out in a whole. But, nr_reclaimed
>>> still gets inc'ed by one even though a whole THP (512 pages) gets
>>> swapped out.
>>>
>>> This doesn't make too much sense to memory reclaim. For example, direct
>>> reclaim may just need reclaim SWAP_CLUSTER_MAX pages, reclaiming one THP
>>> could fulfill it. But, if nr_reclaimed is not increased correctly,
>>> direct reclaim may just waste time to reclaim more pages,
>>> SWAP_CLUSTER_MAX * 512 pages in worst case.
>>>
>>> This change may result in more reclaimed pages than scanned pages showed
>>> by /proc/vmstat since scanning one head page would reclaim 512 base pages.
>>>
>>> Cc: "Huang, Ying" <[email protected]>
>>> Cc: Johannes Weiner <[email protected]>
>>> Cc: Michal Hocko <[email protected]>
>>> Cc: Mel Gorman <[email protected]>
>>> Cc: "Kirill A . Shutemov" <[email protected]>
>>> Cc: Hugh Dickins <[email protected]>
>>> Signed-off-by: Yang Shi <[email protected]>
>>> ---
>>> I'm not quite sure if it was the intended behavior or just omission. I tried
>>> to dig into the review history, but didn't find any clue. I may miss some
>>> discussion.
>>>
>>> mm/vmscan.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>> index fd9de50..7e026ec 100644
>>> --- a/mm/vmscan.c
>>> +++ b/mm/vmscan.c
>>> @@ -1446,7 +1446,11 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>>> unlock_page(page);
>>> free_it:
>>> - nr_reclaimed++;
>>> + /*
>>> + * THP may get swapped out in a whole, need account
>>> + * all base pages.
>>> + */
>>> + nr_reclaimed += (1 << compound_order(page));
>>> /*
>>> * Is there need to periodically free_page_list? It would
>> Good catch! Thanks!
>>
>> How about to change this to
>>
>>
>> nr_reclaimed += hpage_nr_pages(page);
>
> Either is fine to me. Is this faster than "1 << compound_order(page)"?

I think the readability is a little better. And this will become

nr_reclaimed += 1

if CONFIG_TRANSPARENT_HUAGEPAGE is disabled.

Best Regards,
Huang, Ying

>>
>> Best Regards,
>> Huang, Ying

2019-05-10 05:05:48

by William Kucharski

[permalink] [raw]
Subject: Re: [PATCH] mm: vmscan: correct nr_reclaimed for THP



> On May 9, 2019, at 9:03 PM, Huang, Ying <[email protected]> wrote:
>
> Yang Shi <[email protected]> writes:
>
>> On 5/9/19 7:12 PM, Huang, Ying wrote:
>>>
>>> How about to change this to
>>>
>>>
>>> nr_reclaimed += hpage_nr_pages(page);
>>
>> Either is fine to me. Is this faster than "1 << compound_order(page)"?
>
> I think the readability is a little better. And this will become
>
> nr_reclaimed += 1
>
> if CONFIG_TRANSPARENT_HUAGEPAGE is disabled.

I find this more legible and self documenting, and it avoids the bit shift
operation completely on the majority of systems where THP is not configured.


2019-05-10 15:51:04

by Yang Shi

[permalink] [raw]
Subject: Re: [PATCH] mm: vmscan: correct nr_reclaimed for THP



On 5/9/19 8:03 PM, Huang, Ying wrote:
> Yang Shi <[email protected]> writes:
>
>> On 5/9/19 7:12 PM, Huang, Ying wrote:
>>> Yang Shi <[email protected]> writes:
>>>
>>>> Since commit bd4c82c22c36 ("mm, THP, swap: delay splitting THP after
>>>> swapped out"), THP can be swapped out in a whole. But, nr_reclaimed
>>>> still gets inc'ed by one even though a whole THP (512 pages) gets
>>>> swapped out.
>>>>
>>>> This doesn't make too much sense to memory reclaim. For example, direct
>>>> reclaim may just need reclaim SWAP_CLUSTER_MAX pages, reclaiming one THP
>>>> could fulfill it. But, if nr_reclaimed is not increased correctly,
>>>> direct reclaim may just waste time to reclaim more pages,
>>>> SWAP_CLUSTER_MAX * 512 pages in worst case.
>>>>
>>>> This change may result in more reclaimed pages than scanned pages showed
>>>> by /proc/vmstat since scanning one head page would reclaim 512 base pages.
>>>>
>>>> Cc: "Huang, Ying" <[email protected]>
>>>> Cc: Johannes Weiner <[email protected]>
>>>> Cc: Michal Hocko <[email protected]>
>>>> Cc: Mel Gorman <[email protected]>
>>>> Cc: "Kirill A . Shutemov" <[email protected]>
>>>> Cc: Hugh Dickins <[email protected]>
>>>> Signed-off-by: Yang Shi <[email protected]>
>>>> ---
>>>> I'm not quite sure if it was the intended behavior or just omission. I tried
>>>> to dig into the review history, but didn't find any clue. I may miss some
>>>> discussion.
>>>>
>>>> mm/vmscan.c | 6 +++++-
>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>>> index fd9de50..7e026ec 100644
>>>> --- a/mm/vmscan.c
>>>> +++ b/mm/vmscan.c
>>>> @@ -1446,7 +1446,11 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>>>> unlock_page(page);
>>>> free_it:
>>>> - nr_reclaimed++;
>>>> + /*
>>>> + * THP may get swapped out in a whole, need account
>>>> + * all base pages.
>>>> + */
>>>> + nr_reclaimed += (1 << compound_order(page));
>>>> /*
>>>> * Is there need to periodically free_page_list? It would
>>> Good catch! Thanks!
>>>
>>> How about to change this to
>>>
>>>
>>> nr_reclaimed += hpage_nr_pages(page);
>> Either is fine to me. Is this faster than "1 << compound_order(page)"?
> I think the readability is a little better. And this will become
>
> nr_reclaimed += 1
>
> if CONFIG_TRANSPARENT_HUAGEPAGE is disabled.

Good point. Will update in v2 soon.

>
> Best Regards,
> Huang, Ying
>
>>> Best Regards,
>>> Huang, Ying

2019-05-10 15:51:48

by Yang Shi

[permalink] [raw]
Subject: Re: [PATCH] mm: vmscan: correct nr_reclaimed for THP



On 5/9/19 9:33 PM, William Kucharski wrote:
>
>> On May 9, 2019, at 9:03 PM, Huang, Ying <[email protected]> wrote:
>>
>> Yang Shi <[email protected]> writes:
>>
>>> On 5/9/19 7:12 PM, Huang, Ying wrote:
>>>> How about to change this to
>>>>
>>>>
>>>> nr_reclaimed += hpage_nr_pages(page);
>>> Either is fine to me. Is this faster than "1 << compound_order(page)"?
>> I think the readability is a little better. And this will become
>>
>> nr_reclaimed += 1
>>
>> if CONFIG_TRANSPARENT_HUAGEPAGE is disabled.
> I find this more legible and self documenting, and it avoids the bit shift
> operation completely on the majority of systems where THP is not configured.

Yes, I do agree. Thanks for the suggestion.
>

2019-05-10 16:38:46

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH] mm: vmscan: correct nr_reclaimed for THP

On Fri, May 10, 2019 at 10:12:40AM +0800, Huang, Ying wrote:
> > + nr_reclaimed += (1 << compound_order(page));
>
> How about to change this to
>
>
> nr_reclaimed += hpage_nr_pages(page);

Please don't. That embeds the knowledge that we can only swap out either
normal pages or THP sized pages. I'm trying to make the VM capable of
supporting arbitrary-order pages, and this would be just one more place
to fix.

I'm sympathetic to the "self documenting" argument. My current tree has
a patch in it:

mm: Introduce compound_nr

Replace 1 << compound_order(page) with compound_nr(page). Minor
improvements in readability.

It goes along with this patch:

mm: Introduce page_size()

It's unnecessarily hard to find out the size of a potentially huge page.
Replace 'PAGE_SIZE << compound_order(page)' with page_size(page).

Better suggestions on naming gratefully received. I'm more happy with
page_size() than I am with compound_nr(). page_nr() gives the wrong
impression; page_count() isn't great either.

2019-05-10 16:52:20

by Yang Shi

[permalink] [raw]
Subject: Re: [PATCH] mm: vmscan: correct nr_reclaimed for THP



On 5/10/19 9:36 AM, Matthew Wilcox wrote:
> On Fri, May 10, 2019 at 10:12:40AM +0800, Huang, Ying wrote:
>>> + nr_reclaimed += (1 << compound_order(page));
>> How about to change this to
>>
>>
>> nr_reclaimed += hpage_nr_pages(page);
> Please don't. That embeds the knowledge that we can only swap out either
> normal pages or THP sized pages. I'm trying to make the VM capable of
> supporting arbitrary-order pages, and this would be just one more place
> to fix.
>
> I'm sympathetic to the "self documenting" argument. My current tree has
> a patch in it:
>
> mm: Introduce compound_nr
>
> Replace 1 << compound_order(page) with compound_nr(page). Minor
> improvements in readability.
>
> It goes along with this patch:
>
> mm: Introduce page_size()
>
> It's unnecessarily hard to find out the size of a potentially huge page.
> Replace 'PAGE_SIZE << compound_order(page)' with page_size(page).

So you prefer keeping usingĀ  "1 << compound_order" as v1 did? Then you
will convert all "1 << compound_order" to compound_nr?

>
> Better suggestions on naming gratefully received. I'm more happy with
> page_size() than I am with compound_nr(). page_nr() gives the wrong
> impression; page_count() isn't great either.

2019-05-10 16:53:30

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH] mm: vmscan: correct nr_reclaimed for THP

On Fri, May 10, 2019 at 09:50:04AM -0700, Yang Shi wrote:
> On 5/10/19 9:36 AM, Matthew Wilcox wrote:
> > On Fri, May 10, 2019 at 10:12:40AM +0800, Huang, Ying wrote:
> > > > + nr_reclaimed += (1 << compound_order(page));
> > > How about to change this to
> > >
> > > nr_reclaimed += hpage_nr_pages(page);
> > Please don't. That embeds the knowledge that we can only swap out either
> > normal pages or THP sized pages. I'm trying to make the VM capable of
> > supporting arbitrary-order pages, and this would be just one more place
> > to fix.
> >
> > I'm sympathetic to the "self documenting" argument. My current tree has
> > a patch in it:
> >
> > mm: Introduce compound_nr
> > Replace 1 << compound_order(page) with compound_nr(page). Minor
> > improvements in readability.
> >
> > It goes along with this patch:
> >
> > mm: Introduce page_size()
> >
> > It's unnecessarily hard to find out the size of a potentially huge page.
> > Replace 'PAGE_SIZE << compound_order(page)' with page_size(page).
>
> So you prefer keeping using? "1 << compound_order" as v1 did? Then you will
> convert all "1 << compound_order" to compound_nr?

Yes. Please, let's merge v1 and ignore v2.

2019-05-10 16:57:15

by Yang Shi

[permalink] [raw]
Subject: Re: [PATCH] mm: vmscan: correct nr_reclaimed for THP



On 5/10/19 9:52 AM, Matthew Wilcox wrote:
> On Fri, May 10, 2019 at 09:50:04AM -0700, Yang Shi wrote:
>> On 5/10/19 9:36 AM, Matthew Wilcox wrote:
>>> On Fri, May 10, 2019 at 10:12:40AM +0800, Huang, Ying wrote:
>>>>> + nr_reclaimed += (1 << compound_order(page));
>>>> How about to change this to
>>>>
>>>> nr_reclaimed += hpage_nr_pages(page);
>>> Please don't. That embeds the knowledge that we can only swap out either
>>> normal pages or THP sized pages. I'm trying to make the VM capable of
>>> supporting arbitrary-order pages, and this would be just one more place
>>> to fix.
>>>
>>> I'm sympathetic to the "self documenting" argument. My current tree has
>>> a patch in it:
>>>
>>> mm: Introduce compound_nr
>>> Replace 1 << compound_order(page) with compound_nr(page). Minor
>>> improvements in readability.
>>>
>>> It goes along with this patch:
>>>
>>> mm: Introduce page_size()
>>>
>>> It's unnecessarily hard to find out the size of a potentially huge page.
>>> Replace 'PAGE_SIZE << compound_order(page)' with page_size(page).
>> So you prefer keeping usingĀ  "1 << compound_order" as v1 did? Then you will
>> convert all "1 << compound_order" to compound_nr?
> Yes. Please, let's merge v1 and ignore v2.

Fine to me. I think Andrew will take care of it, Andrew?


2019-05-10 22:56:41

by Ira Weiny

[permalink] [raw]
Subject: Re: [PATCH] mm: vmscan: correct nr_reclaimed for THP

On Fri, May 10, 2019 at 09:36:12AM -0700, Matthew Wilcox wrote:
> On Fri, May 10, 2019 at 10:12:40AM +0800, Huang, Ying wrote:
> > > + nr_reclaimed += (1 << compound_order(page));
> >
> > How about to change this to
> >
> >
> > nr_reclaimed += hpage_nr_pages(page);
>
> Please don't. That embeds the knowledge that we can only swap out either
> normal pages or THP sized pages. I'm trying to make the VM capable of
> supporting arbitrary-order pages, and this would be just one more place
> to fix.
>
> I'm sympathetic to the "self documenting" argument. My current tree has
> a patch in it:
>
> mm: Introduce compound_nr
>
> Replace 1 << compound_order(page) with compound_nr(page). Minor
> improvements in readability.
>
> It goes along with this patch:
>
> mm: Introduce page_size()
>
> It's unnecessarily hard to find out the size of a potentially huge page.
> Replace 'PAGE_SIZE << compound_order(page)' with page_size(page).
>
> Better suggestions on naming gratefully received. I'm more happy with
> page_size() than I am with compound_nr(). page_nr() gives the wrong
> impression; page_count() isn't great either.

Stupid question : what does 'nr' stand for?

Ira

2019-05-10 23:07:44

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH] mm: vmscan: correct nr_reclaimed for THP

On Fri, May 10, 2019 at 03:54:56PM -0700, Ira Weiny wrote:
> On Fri, May 10, 2019 at 09:36:12AM -0700, Matthew Wilcox wrote:
> > On Fri, May 10, 2019 at 10:12:40AM +0800, Huang, Ying wrote:
> > > > + nr_reclaimed += (1 << compound_order(page));
> > >
> > > How about to change this to
> > >
> > >
> > > nr_reclaimed += hpage_nr_pages(page);
> >
> > Please don't. That embeds the knowledge that we can only swap out either
> > normal pages or THP sized pages. I'm trying to make the VM capable of
> > supporting arbitrary-order pages, and this would be just one more place
> > to fix.
> >
> > I'm sympathetic to the "self documenting" argument. My current tree has
> > a patch in it:
> >
> > mm: Introduce compound_nr
> >
> > Replace 1 << compound_order(page) with compound_nr(page). Minor
> > improvements in readability.
> >
> > It goes along with this patch:
> >
> > mm: Introduce page_size()
> >
> > It's unnecessarily hard to find out the size of a potentially huge page.
> > Replace 'PAGE_SIZE << compound_order(page)' with page_size(page).
> >
> > Better suggestions on naming gratefully received. I'm more happy with
> > page_size() than I am with compound_nr(). page_nr() gives the wrong
> > impression; page_count() isn't great either.
>
> Stupid question : what does 'nr' stand for?

NumbeR. It's relatively common argot in the Linux kernel (as you can
see from the earlier example ...

> > > nr_reclaimed += hpage_nr_pages(page);

willy@bobo:~/kernel/xarray-2$ git grep -w nr mm |wc -l
388
willy@bobo:~/kernel/xarray-2$ git grep -w nr fs |wc -l
1067

2019-05-11 04:11:10

by Yafang Shao

[permalink] [raw]
Subject: Re: [PATCH] mm: vmscan: correct nr_reclaimed for THP

On Sat, May 11, 2019 at 12:36 AM Matthew Wilcox <[email protected]> wrote:
>
> On Fri, May 10, 2019 at 10:12:40AM +0800, Huang, Ying wrote:
> > > + nr_reclaimed += (1 << compound_order(page));
> >
> > How about to change this to
> >
> >
> > nr_reclaimed += hpage_nr_pages(page);
>
> Please don't. That embeds the knowledge that we can only swap out either
> normal pages or THP sized pages.

Agreed.
compound_order() is more general than hpage_nr_pages().
It seems to me that hpage_nr_pages() is a little abuse in lots of places.

Thanks
Yafang

2019-05-11 22:36:37

by William Kucharski

[permalink] [raw]
Subject: Re: [PATCH] mm: vmscan: correct nr_reclaimed for THP



> On May 10, 2019, at 10:36 AM, Matthew Wilcox <[email protected]> wrote:
>
> Please don't. That embeds the knowledge that we can only swap out either
> normal pages or THP sized pages. I'm trying to make the VM capable of
> supporting arbitrary-order pages, and this would be just one more place
> to fix.
>
> I'm sympathetic to the "self documenting" argument. My current tree has
> a patch in it:
>
> mm: Introduce compound_nr
>
> Replace 1 << compound_order(page) with compound_nr(page). Minor
> improvements in readability.
>
> It goes along with this patch:
>
> mm: Introduce page_size()
>
> It's unnecessarily hard to find out the size of a potentially huge page.
> Replace 'PAGE_SIZE << compound_order(page)' with page_size(page).
>
> Better suggestions on naming gratefully received. I'm more happy with
> page_size() than I am with compound_nr(). page_nr() gives the wrong
> impression; page_count() isn't great either.

I like page_size() as well. At least to me, page_nr() or page_count() would
imply a basis of PAGESIZE, or that you would need to do something like:

page_size = page_nr() << PAGE_SHIFT;

to get the size in bytes; page_size() is more straightforward in that respect.