[PATCH] mm/swap: Use page flags to determine LRU list in __activate_page()

[PATCH] mm/swap: Use page flags to determine LRU list in __activate_page()

[Anshuman Khandual]

Its already assumed that the PageActive flag is clear on the input
page, hence page_lru(page) will pick the base LRU for the page. In
the same way page_lru(page) will pick active base LRU, once the
flag PageActive is set on the page. This change of LRU list should
happen implicitly through the page flags instead of being hard
coded.

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 mm/swap.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/mm/swap.c b/mm/swap.c
index fcd82bc..494276b 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -275,12 +275,10 @@ static void __activate_page(struct page *page, struct lruvec *lruvec,
 {
 	if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
 		int file = page_is_file_cache(page);
-		int lru = page_lru_base_type(page);
 
-		del_page_from_lru_list(page, lruvec, lru);
+		del_page_from_lru_list(page, lruvec, page_lru(page));
 		SetPageActive(page);
-		lru += LRU_ACTIVE;
-		add_page_to_lru_list(page, lruvec, lru);
+		add_page_to_lru_list(page, lruvec, page_lru(page));
 		trace_mm_lru_activate(page);
 
 		__count_vm_event(PGACTIVATE);
-- 
1.8.5.2

Re: [PATCH] mm/swap: Use page flags to determine LRU list in __activate_page()

[Michal Hocko]

On Thu 19-10-17 20:26:57, Anshuman Khandual wrote:
> Its already assumed that the PageActive flag is clear on the input
> page, hence page_lru(page) will pick the base LRU for the page. In
> the same way page_lru(page) will pick active base LRU, once the
> flag PageActive is set on the page. This change of LRU list should
> happen implicitly through the page flags instead of being hard
> coded.

The patch description tells what but it doesn't explain _why_? Does the
resulting code is better, more optimized or is this a pure readability
thing?

All I can see is that page_lru is more complex and a large part of it
can be optimized away which has been done manually here. I suspect the
compiler can deduce the same thing.

> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> ---
>  mm/swap.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/swap.c b/mm/swap.c
> index fcd82bc..494276b 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -275,12 +275,10 @@ static void __activate_page(struct page *page, struct lruvec *lruvec,
>  {
>  	if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
>  		int file = page_is_file_cache(page);
> -		int lru = page_lru_base_type(page);
>  
> -		del_page_from_lru_list(page, lruvec, lru);
> +		del_page_from_lru_list(page, lruvec, page_lru(page));
>  		SetPageActive(page);
> -		lru += LRU_ACTIVE;
> -		add_page_to_lru_list(page, lruvec, lru);
> +		add_page_to_lru_list(page, lruvec, page_lru(page));
>  		trace_mm_lru_activate(page);
>  
>  		__count_vm_event(PGACTIVATE);
> -- 
> 1.8.5.2

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/swap: Use page flags to determine LRU list in __activate_page()

[Anshuman Khandual]

On 10/19/2017 09:03 PM, Michal Hocko wrote:
> On Thu 19-10-17 20:26:57, Anshuman Khandual wrote:
>> Its already assumed that the PageActive flag is clear on the input
>> page, hence page_lru(page) will pick the base LRU for the page. In
>> the same way page_lru(page) will pick active base LRU, once the
>> flag PageActive is set on the page. This change of LRU list should
>> happen implicitly through the page flags instead of being hard
>> coded.
> 
> The patch description tells what but it doesn't explain _why_? Does the
> resulting code is better, more optimized or is this a pure readability
> thing?

Not really. Not only it removes couple of lines of code but it also
makes it look more logical from function flow point of view as well.

> 
> All I can see is that page_lru is more complex and a large part of it
> can be optimized away which has been done manually here. I suspect the
> compiler can deduce the same thing.

Why not ? I mean, that is the essence of the function page_lru() which
should get us the exact LRU list the page should be on and hence we
should not hand craft these manually.

Re: [PATCH] mm/swap: Use page flags to determine LRU list in __activate_page()

[Anshuman Khandual]

On 10/23/2017 08:52 AM, Anshuman Khandual wrote:
> On 10/19/2017 09:03 PM, Michal Hocko wrote:
>> On Thu 19-10-17 20:26:57, Anshuman Khandual wrote:
>>> Its already assumed that the PageActive flag is clear on the input
>>> page, hence page_lru(page) will pick the base LRU for the page. In
>>> the same way page_lru(page) will pick active base LRU, once the
>>> flag PageActive is set on the page. This change of LRU list should
>>> happen implicitly through the page flags instead of being hard
>>> coded.
>>
>> The patch description tells what but it doesn't explain _why_? Does the
>> resulting code is better, more optimized or is this a pure readability
>> thing?
> 
> Not really. Not only it removes couple of lines of code but it also
> makes it look more logical from function flow point of view as well.
> 
>>
>> All I can see is that page_lru is more complex and a large part of it
>> can be optimized away which has been done manually here. I suspect the
>> compiler can deduce the same thing.
> 
> Why not ? I mean, that is the essence of the function page_lru() which
> should get us the exact LRU list the page should be on and hence we
> should not hand craft these manually.

Hi Michal,

Did not hear from you on this. So wondering what is the verdict
about this patch ?

- Anshuman

Re: [PATCH] mm/swap: Use page flags to determine LRU list in __activate_page()

[Michal Hocko]

On Fri 27-10-17 09:36:37, Anshuman Khandual wrote:
> On 10/23/2017 08:52 AM, Anshuman Khandual wrote:
> > On 10/19/2017 09:03 PM, Michal Hocko wrote:
> >> On Thu 19-10-17 20:26:57, Anshuman Khandual wrote:
> >>> Its already assumed that the PageActive flag is clear on the input
> >>> page, hence page_lru(page) will pick the base LRU for the page. In
> >>> the same way page_lru(page) will pick active base LRU, once the
> >>> flag PageActive is set on the page. This change of LRU list should
> >>> happen implicitly through the page flags instead of being hard
> >>> coded.
> >>
> >> The patch description tells what but it doesn't explain _why_? Does the
> >> resulting code is better, more optimized or is this a pure readability
> >> thing?
> > 
> > Not really. Not only it removes couple of lines of code but it also
> > makes it look more logical from function flow point of view as well.
> > 
> >>
> >> All I can see is that page_lru is more complex and a large part of it
> >> can be optimized away which has been done manually here. I suspect the
> >> compiler can deduce the same thing.
> > 
> > Why not ? I mean, that is the essence of the function page_lru() which
> > should get us the exact LRU list the page should be on and hence we
> > should not hand craft these manually.
> 
> Hi Michal,
> 
> Did not hear from you on this. So wondering what is the verdict
> about this patch ?

IMHO, there is no reason to change the code as it doesn't solve any real
problem or it doesn't make the code more effective AFAICS.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/swap: Use page flags to determine LRU list in __activate_page()

[Vlastimil Babka]

On 10/19/2017 05:33 PM, Michal Hocko wrote:
> On Thu 19-10-17 20:26:57, Anshuman Khandual wrote:
>> Its already assumed that the PageActive flag is clear on the input
>> page, hence page_lru(page) will pick the base LRU for the page. In
>> the same way page_lru(page) will pick active base LRU, once the
>> flag PageActive is set on the page. This change of LRU list should
>> happen implicitly through the page flags instead of being hard
>> coded.
> 
> The patch description tells what but it doesn't explain _why_? Does the
> resulting code is better, more optimized or is this a pure readability
> thing?
> 
> All I can see is that page_lru is more complex and a large part of it
> can be optimized away which has been done manually here. I suspect the
> compiler can deduce the same thing.

We shouldn't overestimate the compiler (or the objective conditions it
has) for optimizing stuff away:

After applying the patch:

./scripts/bloat-o-meter swap_before.o mm/swap.o
add/remove: 0/0 grow/shrink: 1/0 up/down: 160/0 (160)
function                                     old     new   delta
__activate_page                              708     868    +160
Total: Before=13538, After=13698, chg +1.18%

I don't think we want that, it's not exactly a cold code...

Re: [PATCH] mm/swap: Use page flags to determine LRU list in __activate_page()

[Anshuman Khandual]

On 10/31/2017 06:15 PM, Vlastimil Babka wrote:
> On 10/19/2017 05:33 PM, Michal Hocko wrote:
>> On Thu 19-10-17 20:26:57, Anshuman Khandual wrote:
>>> Its already assumed that the PageActive flag is clear on the input
>>> page, hence page_lru(page) will pick the base LRU for the page. In
>>> the same way page_lru(page) will pick active base LRU, once the
>>> flag PageActive is set on the page. This change of LRU list should
>>> happen implicitly through the page flags instead of being hard
>>> coded.
>>
>> The patch description tells what but it doesn't explain _why_? Does the
>> resulting code is better, more optimized or is this a pure readability
>> thing?
>>
>> All I can see is that page_lru is more complex and a large part of it
>> can be optimized away which has been done manually here. I suspect the
>> compiler can deduce the same thing.
> 
> We shouldn't overestimate the compiler (or the objective conditions it
> has) for optimizing stuff away:
> 
> After applying the patch:
> 
> ./scripts/bloat-o-meter swap_before.o mm/swap.o
> add/remove: 0/0 grow/shrink: 1/0 up/down: 160/0 (160)
> function                                     old     new   delta
> __activate_page                              708     868    +160
> Total: Before=13538, After=13698, chg +1.18%
> 
> I don't think we want that, it's not exactly a cold code...

Yeah, makes sense.