linux-kernel - Re: [v4 2/3] mm: Defer TLB flush by keeping both src and dst folios at migration

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: <ZUzuUf7JfhybYBgg@casper.infradead.org>
Date:   Thu, 9 Nov 2023 14:36:01 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     Byungchul Park <byungchul@...com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        kernel_team@...ynix.com, akpm@...ux-foundation.org,
        ying.huang@...el.com, namit@...are.com, xhao@...ux.alibaba.com,
        mgorman@...hsingularity.net, hughd@...gle.com, david@...hat.com,
        peterz@...radead.org, luto@...nel.org, tglx@...utronix.de,
        mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com
Subject: Re: [v4 2/3] mm: Defer TLB flush by keeping both src and dst folios
 at migration

On Thu, Nov 09, 2023 at 01:59:07PM +0900, Byungchul Park wrote:
> +++ b/include/linux/page-flags.h
> @@ -136,6 +136,7 @@ enum pageflags {
>  	PG_arch_2,
>  	PG_arch_3,
>  #endif
> +	PG_migrc,		/* Page is under migrc's control */
>  	__NR_PAGEFLAGS,

Yeah; no.  We're out of page flags.  And CXL is insufficiently
compelling to add more.  If you use CXL, you don't care about
performance, by definition.

> @@ -589,6 +590,9 @@ TESTCLEARFLAG(Young, young, PF_ANY)
>  PAGEFLAG(Idle, idle, PF_ANY)
>  #endif
>  
> +TESTCLEARFLAG(Migrc, migrc, PF_ANY)
> +__PAGEFLAG(Migrc, migrc, PF_ANY)

Why PF_ANY?

> +/*
> + * Initialize the page when allocated from buddy allocator.
> + */
> +static inline void migrc_init_page(struct page *p)
> +{
> +	__ClearPageMigrc(p);
> +}

This flag should already be clear ... ?

> +/*
> + * Check if the folio is pending for TLB flush and then clear the flag.
> + */
> +static inline bool migrc_unpend_if_pending(struct folio *f)
> +{
> +	return folio_test_clear_migrc(f);
> +}

If you named the flag better, you wouldn't need this wrapper.

> +static void migrc_mark_pending(struct folio *fsrc, struct folio *fdst)
> +{
> +	folio_get(fsrc);
> +	__folio_set_migrc(fsrc);
> +	__folio_set_migrc(fdst);
> +}

This is almost certainly unsafe.  By using the non-atomic bit ops, you
stand the risk of losing a simultaneous update to any other bit in this
word.  Like, say, someone trying to lock the folio?

> +++ b/mm/page_alloc.c
> @@ -1535,6 +1535,9 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
>  
>  	set_page_owner(page, order, gfp_flags);
>  	page_table_check_alloc(page, order);
> +
> +	for (i = 0; i != 1 << order; ++i)
> +		migrc_init_page(page + i);

No.