[RFC PATCH] mm: fix mlock incorrent event account

[RFC PATCH] mm: fix mlock incorrent event account

[zhongjiang]

From: zhong jiang <zhongjiang@huawei.com>

when clear_page_mlock call, we had finish the page isolate successfully,
but it fails to increase the UNEVICTABLE_PGMUNLOCKED account.

The patch add the event account when successful page isolation.

Signed-off-by: zhong jiang <zhongjiang@huawei.com>
---
 mm/mlock.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/mlock.c b/mm/mlock.c
index c483c5c..941930b 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -64,6 +64,7 @@ void clear_page_mlock(struct page *page)
 			    -hpage_nr_pages(page));
 	count_vm_event(UNEVICTABLE_PGCLEARED);
 	if (!isolate_lru_page(page)) {
+		count_vm_event(UNEVICTABLE_PGMUNLOCKED);
 		putback_lru_page(page);
 	} else {
 		/*
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH] mm: fix mlock incorrent event account

[Michal Hocko]

On Thu 25-05-17 15:59:39, zhongjiang wrote:
> From: zhong jiang <zhongjiang@huawei.com>
> 
> when clear_page_mlock call, we had finish the page isolate successfully,
> but it fails to increase the UNEVICTABLE_PGMUNLOCKED account.
> 
> The patch add the event account when successful page isolation.

Could you describe _what_ is the problem, how it can be _triggered_
and _how_ serious it is. Is it something that can be triggered from
userspace? The mlock code is really tricky and it is far from trivial
to see whether this is obviously right or a wrong assumption on your
side. Before people go and spend time reviewing it is fair to introduce
them to the problem.

I believe this is not the first time I am giving you this feedback
so I would _really_ appreciated if you tried harder with the changelog.
It is much simpler to write a patch than review it in many cases.

> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> ---
>  mm/mlock.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/mlock.c b/mm/mlock.c
> index c483c5c..941930b 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -64,6 +64,7 @@ void clear_page_mlock(struct page *page)
>  			    -hpage_nr_pages(page));
>  	count_vm_event(UNEVICTABLE_PGCLEARED);
>  	if (!isolate_lru_page(page)) {
> +		count_vm_event(UNEVICTABLE_PGMUNLOCKED);
>  		putback_lru_page(page);
>  	} else {
>  		/*
> -- 
> 1.8.3.1

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH] mm: fix mlock incorrent event account

[zhong jiang]

On 2017/5/25 16:13, Michal Hocko wrote:
> On Thu 25-05-17 15:59:39, zhongjiang wrote:
>> From: zhong jiang <zhongjiang@huawei.com>
>>
>> when clear_page_mlock call, we had finish the page isolate successfully,
>> but it fails to increase the UNEVICTABLE_PGMUNLOCKED account.
>>
>> The patch add the event account when successful page isolation.
> Could you describe _what_ is the problem, how it can be _triggered_
> and _how_ serious it is. Is it something that can be triggered from
> userspace? The mlock code is really tricky and it is far from trivial
> to see whether this is obviously right or a wrong assumption on your
> side. Before people go and spend time reviewing it is fair to introduce
> them to the problem.
>
> I believe this is not the first time I am giving you this feedback
> so I would _really_ appreciated if you tried harder with the changelog.
> It is much simpler to write a patch than review it in many cases.
 HI, MIchal
 
  I am sorry for that. I will keep in mind. 

  As for the above issue. I get the conclusin after reviewing the code.
  I will further prove wheher the issue is exist or not.

  Thanks
  zhongjiang
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>> ---
>>  mm/mlock.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/mm/mlock.c b/mm/mlock.c
>> index c483c5c..941930b 100644
>> --- a/mm/mlock.c
>> +++ b/mm/mlock.c
>> @@ -64,6 +64,7 @@ void clear_page_mlock(struct page *page)
>>  			    -hpage_nr_pages(page));
>>  	count_vm_event(UNEVICTABLE_PGCLEARED);
>>  	if (!isolate_lru_page(page)) {
>> +		count_vm_event(UNEVICTABLE_PGMUNLOCKED);
>>  		putback_lru_page(page);
>>  	} else {
>>  		/*
>> -- 
>> 1.8.3.1


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH] mm: fix mlock incorrent event account

[zhong jiang]

Hi Michal

by a testcase, The patch is work as I think. The testcase is as follows.

int main(void)
{
    char *map;
    int fd;

    fd = open("test", O_CREAT|O_RDWR);
    unlink("test");
    ftruncate(fd, 4096);
    map = mmap(NULL, 4096, PROT_WRITE, MAP_PRIVATE, fd, 0);
    map[0] = 11;
    mlock(map, sizeof(fd));
    ftruncate(fd, 0);
    close(fd);
    munlock(map, sizeof(fd));
    munmap(map, 4096);

    return 0;
}

before:
unevictable_pgs_mlocked 10589
unevictable_pgs_munlocked 10588
unevictable_pgs_cleared 1

apply the patch;
after:
unevictable_pgs_mlocked 9497
unevictable_pgs_munlocked 9497
unevictable_pgs_cleared 1

unmap_mapping_range unmap them,  page_remove_rmap will deal with
clear_page_mlock situation.  we clear page Mlock flag and successful
isolate the page,  the page will putback the evictable list. but it is not
record the munlock event.

Thanks
zhongjiang

On 2017/5/25 16:13, Michal Hocko wrote:
> On Thu 25-05-17 15:59:39, zhongjiang wrote:
>> From: zhong jiang <zhongjiang@huawei.com>
>>
>> when clear_page_mlock call, we had finish the page isolate successfully,
>> but it fails to increase the UNEVICTABLE_PGMUNLOCKED account.
>>
>> The patch add the event account when successful page isolation.
> Could you describe _what_ is the problem, how it can be _triggered_
> and _how_ serious it is. Is it something that can be triggered from
> userspace? The mlock code is really tricky and it is far from trivial
> to see whether this is obviously right or a wrong assumption on your
> side. Before people go and spend time reviewing it is fair to introduce
> them to the problem.
>
> I believe this is not the first time I am giving you this feedback
> so I would _really_ appreciated if you tried harder with the changelog.
> It is much simpler to write a patch than review it in many cases.
>
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>> ---
>>  mm/mlock.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/mm/mlock.c b/mm/mlock.c
>> index c483c5c..941930b 100644
>> --- a/mm/mlock.c
>> +++ b/mm/mlock.c
>> @@ -64,6 +64,7 @@ void clear_page_mlock(struct page *page)
>>  			    -hpage_nr_pages(page));
>>  	count_vm_event(UNEVICTABLE_PGCLEARED);
>>  	if (!isolate_lru_page(page)) {
>> +		count_vm_event(UNEVICTABLE_PGMUNLOCKED);
>>  		putback_lru_page(page);
>>  	} else {
>>  		/*
>> -- 
>> 1.8.3.1


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH] mm: fix mlock incorrent event account

[Michal Hocko]

On Thu 25-05-17 21:48:56, zhong jiang wrote:
> Hi Michal
> 
> by a testcase, The patch is work as I think. The testcase is as follows.
> 
> int main(void)
> {
>     char *map;
>     int fd;
> 
>     fd = open("test", O_CREAT|O_RDWR);
>     unlink("test");
>     ftruncate(fd, 4096);
>     map = mmap(NULL, 4096, PROT_WRITE, MAP_PRIVATE, fd, 0);
>     map[0] = 11;
>     mlock(map, sizeof(fd));

just a nit
you probably wanted mlock(map, 4096)

>     ftruncate(fd, 0);
>     close(fd);
>     munlock(map, sizeof(fd));

similarly here

>     munmap(map, 4096);
> 
>     return 0;
> }
> 
> before:
> unevictable_pgs_mlocked 10589
> unevictable_pgs_munlocked 10588
> unevictable_pgs_cleared 1
> 
> apply the patch;
> after:
> unevictable_pgs_mlocked 9497
> unevictable_pgs_munlocked 9497
> unevictable_pgs_cleared 1

OK, this is definitely useful for the changelog.

> unmap_mapping_range unmap them,  page_remove_rmap will deal with
> clear_page_mlock situation.  we clear page Mlock flag and successful
> isolate the page,  the page will putback the evictable list. but it is not
> record the munlock event.

and this as well. I haven't checked that but it gives reviewers chance
to understand your thinking much better so it is definitely useful.
Mlock code is everything but straightforward.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH] mm: fix mlock incorrent event account

[zhong jiang]

On 2017/5/25 22:19, Michal Hocko wrote:
> On Thu 25-05-17 21:48:56, zhong jiang wrote:
>> Hi Michal
>>
>> by a testcase, The patch is work as I think. The testcase is as follows.
>>
>> int main(void)
>> {
>>     char *map;
>>     int fd;
>>
>>     fd = open("test", O_CREAT|O_RDWR);
>>     unlink("test");
>>     ftruncate(fd, 4096);
>>     map = mmap(NULL, 4096, PROT_WRITE, MAP_PRIVATE, fd, 0);
>>     map[0] = 11;
>>     mlock(map, sizeof(fd));
> just a nit
> you probably wanted mlock(map, 4096)
>
>>     ftruncate(fd, 0);
>>     close(fd);
>>     munlock(map, sizeof(fd));
> similarly here
>
>>     munmap(map, 4096);
>>
>>     return 0;
>> }
>>
>> before:
>> unevictable_pgs_mlocked 10589
>> unevictable_pgs_munlocked 10588
>> unevictable_pgs_cleared 1
>>
>> apply the patch;
>> after:
>> unevictable_pgs_mlocked 9497
>> unevictable_pgs_munlocked 9497
>> unevictable_pgs_cleared 1
> OK, this is definitely useful for the changelog.
>
>> unmap_mapping_range unmap them,  page_remove_rmap will deal with
>> clear_page_mlock situation.  we clear page Mlock flag and successful
>> isolate the page,  the page will putback the evictable list. but it is not
>> record the munlock event.
> and this as well. I haven't checked that but it gives reviewers chance
> to understand your thinking much better so it is definitely useful.
> Mlock code is everything but straightforward.
>
 HI, Michal

 I will add the above info the chanelog. and resent it in v2.

 Thanks
zhongjiang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>