linux-kernel - Re: [PATCH v2] mm: mlock: remove lru_add_drain

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171020061902.sqz5vklhtqrawelf@dhcp22.suse.cz>
Date:   Fri, 20 Oct 2017 08:19:02 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Shakeel Butt <shakeelb@...gle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Minchan Kim <minchan@...nel.org>,
        Yisheng Xie <xieyisheng1@...wei.com>,
        Ingo Molnar <mingo@...nel.org>,
        Greg Thelen <gthelen@...gle.com>,
        Hugh Dickins <hughd@...gle.com>,
        Balbir Singh <bsingharora@...il.com>,
        Anshuman Khandual <khandual@...ux.vnet.ibm.com>,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] mm: mlock: remove lru_add_drain_all()

On Thu 19-10-17 15:25:07, Shakeel Butt wrote:
> lru_add_drain_all() is not required by mlock() and it will drain
> everything that has been cached at the time mlock is called. And
> that is not really related to the memory which will be faulted in
> (and cached) and mlocked by the syscall itself.
> 
> Without lru_add_drain_all() the mlocked pages can remain on pagevecs
> and be moved to evictable LRUs. However they will eventually be moved
> back to unevictable LRU by reclaim. So, we can safely remove
> lru_add_drain_all() from mlock syscall. Also there is no need for
> local lru_add_drain() as it will be called deep inside __mm_populate()
> (in follow_page_pte()).

This paragraph can be still a bit confusing. I suspect you meant to say
something like: "If anything lru_add_drain_all" should be called _after_
pages have been mlocked and faulted in but even that is not strictly
needed because those pages would get to the appropriate LRUs lazily
during the reclaim path. Moreover follow_page_pte (gup) will drain the
local pcp LRU cache."

> On larger machines the overhead of lru_add_drain_all() in mlock() can
> be significant when mlocking data already in memory. We have observed
> high latency in mlock() due to lru_add_drain_all() when the users
> were mlocking in memory tmpfs files.
> 
> Signed-off-by: Shakeel Butt <shakeelb@...gle.com>

Anyway, this patch makes a lot of sense to me. Feel free to add
Acked-by: Michal Hocko <mhocko@...e.com>

> ---
> Changelog since v1:
> - updated commit message
> 
>  mm/mlock.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/mm/mlock.c b/mm/mlock.c
> index dfc6f1912176..3ceb2935d1e0 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -669,8 +669,6 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
>  	if (!can_do_mlock())
>  		return -EPERM;
>  
> -	lru_add_drain_all();	/* flush pagevec */
> -
>  	len = PAGE_ALIGN(len + (offset_in_page(start)));
>  	start &= PAGE_MASK;
>  
> @@ -797,9 +795,6 @@ SYSCALL_DEFINE1(mlockall, int, flags)
>  	if (!can_do_mlock())
>  		return -EPERM;
>  
> -	if (flags & MCL_CURRENT)
> -		lru_add_drain_all();	/* flush pagevec */
> -
>  	lock_limit = rlimit(RLIMIT_MEMLOCK);
>  	lock_limit >>= PAGE_SHIFT;
>  
> -- 
> 2.15.0.rc0.271.g36b669edcc-goog
> 

-- 
Michal Hocko
SUSE Labs