Subject: Re: [RFC 05/11] mm: make the try_to_munlock void function
To: Minchan Kim <minchan@kernel.org>,
        Anshuman Khandual <khandual@linux.vnet.ibm.com>
References: <1488436765-32350-1-git-send-email-minchan@kernel.org>
 <1488436765-32350-6-git-send-email-minchan@kernel.org>
 <98488e1a-0202-b88b-ca9c-1dc0d6c27ae5@linux.vnet.ibm.com>
 <20170306020901.GC8779@bbox>
 <12618e8d-9eaf-322a-4334-e1416dc95b8b@linux.vnet.ibm.com>
 <20170307065048.GD29458@bbox>
Cc: Andrew Morton <akpm@linux-foundation.org>, kernel-team@lge.com,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Johannes Weiner <hannes@cmpxchg.org>, Michal Hocko <mhocko@suse.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
From: Anshuman Khandual <khandual@linux.vnet.ibm.com>
Date: Tue, 7 Mar 2017 14:25:48 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.5.1
MIME-Version: 1.0
In-Reply-To: <20170307065048.GD29458@bbox>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Message-Id: <6e412428-86c7-9032-a3b6-41980ad667f9@linux.vnet.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4975
Lines: 122

On 03/07/2017 12:20 PM, Minchan Kim wrote:
> Hi Anshuman,
> 
> On Mon, Mar 06, 2017 at 03:10:17PM +0530, Anshuman Khandual wrote:
>> On 03/06/2017 07:39 AM, Minchan Kim wrote:
>>> On Fri, Mar 03, 2017 at 05:13:54PM +0530, Anshuman Khandual wrote:
>>>> On 03/02/2017 12:09 PM, Minchan Kim wrote:
>>>>> try_to_munlock returns SWAP_MLOCK if the one of VMAs mapped
>>>>> the page has VM_LOCKED flag. In that time, VM set PG_mlocked to
>>>>> the page if the page is not pte-mapped THP which cannot be
>>>>> mlocked, either.
>>>> Right.
>>>>
>>>>> With that, __munlock_isolated_page can use PageMlocked to check
>>>>> whether try_to_munlock is successful or not without relying on
>>>>> try_to_munlock's retval. It helps to make ttu/ttuo simple with
>>>>> upcoming patches.
>>>> Right.
>>>>
>>>>> Cc: Vlastimil Babka <vbabka@suse.cz>
>>>>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>>> Signed-off-by: Minchan Kim <minchan@kernel.org>
>>>>> ---
>>>>>  include/linux/rmap.h |  2 +-
>>>>>  mm/mlock.c           |  6 ++----
>>>>>  mm/rmap.c            | 16 ++++------------
>>>>>  3 files changed, 7 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
>>>>> index b556eef..1b0cd4c 100644
>>>>> --- a/include/linux/rmap.h
>>>>> +++ b/include/linux/rmap.h
>>>>> @@ -235,7 +235,7 @@ int page_mkclean(struct page *);
>>>>>   * called in munlock()/munmap() path to check for other vmas holding
>>>>>   * the page mlocked.
>>>>>   */
>>>>> -int try_to_munlock(struct page *);
>>>>> +void try_to_munlock(struct page *);
>>>>>  
>>>>>  void remove_migration_ptes(struct page *old, struct page *new, bool locked);
>>>>>  
>>>>> diff --git a/mm/mlock.c b/mm/mlock.c
>>>>> index cdbed8a..d34a540 100644
>>>>> --- a/mm/mlock.c
>>>>> +++ b/mm/mlock.c
>>>>> @@ -122,17 +122,15 @@ static bool __munlock_isolate_lru_page(struct page *page, bool getpage)
>>>>>   */
>>>>>  static void __munlock_isolated_page(struct page *page)
>>>>>  {
>>>>> -	int ret = SWAP_AGAIN;
>>>>> -
>>>>>  	/*
>>>>>  	 * Optimization: if the page was mapped just once, that's our mapping
>>>>>  	 * and we don't need to check all the other vmas.
>>>>>  	 */
>>>>>  	if (page_mapcount(page) > 1)
>>>>> -		ret = try_to_munlock(page);
>>>>> +		try_to_munlock(page);
>>>>>  
>>>>>  	/* Did try_to_unlock() succeed or punt? */
>>>>> -	if (ret != SWAP_MLOCK)
>>>>> +	if (!PageMlocked(page))
>>>> Checks if the page is still mlocked or not.
>>>>
>>>>>  		count_vm_event(UNEVICTABLE_PGMUNLOCKED);
>>>>>  
>>>>>  	putback_lru_page(page);
>>>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>>>> index 0a48958..61ae694 100644
>>>>> --- a/mm/rmap.c
>>>>> +++ b/mm/rmap.c
>>>>> @@ -1540,18 +1540,10 @@ static int page_not_mapped(struct page *page)
>>>>>   * Called from munlock code.  Checks all of the VMAs mapping the page
>>>>>   * to make sure nobody else has this page mlocked. The page will be
>>>>>   * returned with PG_mlocked cleared if no other vmas have it mlocked.
>>>>> - *
>>>>> - * Return values are:
>>>>> - *
>>>>> - * SWAP_AGAIN	- no vma is holding page mlocked, or,
>>>>> - * SWAP_AGAIN	- page mapped in mlocked vma -- couldn't acquire mmap sem
>>>>> - * SWAP_FAIL	- page cannot be located at present
>>>>> - * SWAP_MLOCK	- page is now mlocked.
>>>>>   */
>>>>> -int try_to_munlock(struct page *page)
>>>>> -{
>>>>> -	int ret;
>>>>>  
>>>>> +void try_to_munlock(struct page *page)
>>>>> +{
>>>>>  	struct rmap_walk_control rwc = {
>>>>>  		.rmap_one = try_to_unmap_one,
>>>>>  		.arg = (void *)TTU_MUNLOCK,
>>>>> @@ -1561,9 +1553,9 @@ int try_to_munlock(struct page *page)
>>>>>  	};
>>>>>  
>>>>>  	VM_BUG_ON_PAGE(!PageLocked(page) || PageLRU(page), page);
>>>>> +	VM_BUG_ON_PAGE(PageMlocked(page), page);
>>>> We are calling on the page to see if its mlocked from any of it's
>>>> mapping VMAs. Then it is a possibility that the page is mlocked
>>>> and the above condition is true and we print VM BUG report there.
>>>> The point is if its a valid possibility why we have added the
>>>> above check ?
>>> If I read code properly,  __munlock_isolated_page calls try_to_munlock
>>> always pass the TestClearPageMlocked page to try_to_munlock.
>> Right.
>>
>>> (e.g., munlock_vma_page and __munlock_pagevec) so I thought
>>> try_to_munlock should be called non-PG_mlocked page and try_to_unmap_one
>>> returns PG_mlocked page once it found a VM_LOCKED VMA for a page.
>>> IOW, non-PG_mlocked page is precondition for try_to_munlock.
>> Okay, I have missed that part. Nonetheless this is a separate issue,
>> should be part of a different patch ? Not inside these cleanups.
> If that precondition is not true, this patch changes the behavior
> slightly.
> 
>         UNEVICTABLE_PGMUNLOCKED count mistmatch compared to old.
> 
> I wanted to catch it up. If you still think it's separate issue,
> I will do. Please tell me. However, I still think it's no problem
> to merge it in this clean up patch.

Got it, its okay. Let this change be part of this patch itself.