linux-kernel - Re: [PATCH] mm: don't be stuck to rmap lock on reclaim path

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]

Date:   Thu, 5 May 2022 01:42:17 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Minchan Kim <minchan@...nel.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-mm <linux-mm@...ck.org>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Michal Hocko <mhocko@...e.com>,
        John Dias <joaodias@...gle.com>,
        Tim Murray <timmurray@...gle.com>,
        Vladimir Davydov <vdavydov.dev@...il.com>
Subject: Re: [PATCH] mm: don't be stuck to rmap lock on reclaim path

On Wed, May 04, 2022 at 04:47:51PM -0700, Minchan Kim wrote:
> Since this patch introduces a new "contended" field as out param
> in rmap_walk_control, it's not immutable any longer so remove
> const keywords on rmap related functions. Since rmap walking
> is already expensive operation, I doubt the const would help sizable
> benefit(And we didn't have it until 5.17).

Um?  If it's now mutable, it surely can't be static as that means it
would be shared by all callers, and you might write to the new fields
in one caller and have them interpreted by the other caller!

Or if it is safe, then the comment needs to not be deleted, but modified
to explain why it's safe to do so in this instance, and that other
instances should not copy the approach unless they are similarly safe.

> diff --git a/mm/page_idle.c b/mm/page_idle.c
> index fc0435abf909..fdff8c6dcd2d 100644
> --- a/mm/page_idle.c
> +++ b/mm/page_idle.c
> @@ -86,11 +86,8 @@ static bool page_idle_clear_pte_refs_one(struct folio *folio,
>  static void page_idle_clear_pte_refs(struct page *page)
>  {
>  	struct folio *folio = page_folio(page);
> -	/*
> -	 * Since rwc.arg is unused, rwc is effectively immutable, so we
> -	 * can make it static const to save some cycles and stack.
> -	 */
> -	static const struct rmap_walk_control rwc = {
> +
> +	static struct rmap_walk_control rwc = {
>  		.rmap_one = page_idle_clear_pte_refs_one,
>  		.anon_lock = folio_lock_anon_vma_read,
>  	};