Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 4 Mar 2022 19:26:00 +0000
In-Reply-To: <20220304171912.305060-1-hannes@cmpxchg.org>
Message-Id: <20220304192600.rvmgbg72aq6idooc@google.com>
Mime-Version: 1.0
References: <20220304171912.305060-1-hannes@cmpxchg.org>
Subject: Re: [PATCH] mm: madvise: MADV_DONTNEED_LOCKED
From:   Shakeel Butt <shakeelb@google.com>
To:     Johannes Weiner <hannes@cmpxchg.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Michal Hocko <mhocko@suse.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Nadav Amit <nadav.amit@gmail.com>,
        David Hildenbrand <david@redhat.com>, dgilbert@redhat.com,
        Mike Kravetz <mike.kravetz@oracle.com>, linux-mm@kvack.org,
        linux-api@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"; format=flowed; delsp=yes
Precedence: bulk

On Fri, Mar 04, 2022 at 12:19:12PM -0500, Johannes Weiner wrote:
> MADV_DONTNEED historically rejects mlocked ranges, but with
> MLOCK_ONFAULT and MCL_ONFAULT allowing to mlock without populating,
> there are valid use cases for depopulating locked ranges as well.

> Users mlock memory to protect secrets. There are allocators for secure
> buffers that want in-use memory generally mlocked, but cleared and
> invalidated memory to give up the physical pages. This could be done
> with explicit munlock -> mlock calls on free -> alloc of course, but
> that adds two unnecessary syscalls, heavy mmap_sem write locks, vma
> splits and re-merges - only to get rid of the backing pages.

> Users also mlockall(MCL_ONFAULT) to suppress sustained paging, but are
> okay with on-demand initial population. It seems valid to selectively
> free some memory during the lifetime of such a process, without having
> to mess with its overall policy.

> Why add a separate flag? Isn't this a pretty niche usecase?

> - MADV_DONTNEED has been bailing on locked vmas forever. It's at least
>    conceivable that someone, somewhere is relying on mlock to protect
>    data from perhaps broader invalidation calls. Changing this behavior
>    now could lead to quiet data corruption.

> - It also clarifies expectations around MADV_FREE and maybe
>    MADV_REMOVE. It avoids the situation where one quietly behaves
>    different than the others. MADV_FREE_LOCKED can be added later.

> - The combination of mlock() and madvise() in the first place is
>    probably niche. But where it happens, I'd say that dropping pages
>    from a locked region once they don't contain secrets or won't page
>    anymore is much saner than relying on mlock to protect memory from
>    speculative or errant invalidation calls. It's just that we can't
>    change the default behavior because of the two previous points.

> Given that, an explicit new flag seems to make the most sense.

> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> Acked-by: Michal Hocko <mhocko@suse.com>

Reviewed-by: Shakeel Butt <shakeelb@google.com>