Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Nadav Amit <namit@vmware.com>
To:     Peter Zijlstra <peterz@infradead.org>,
        Borislav Petkov <bp@alien8.de>
CC:     Andy Lutomirski <luto@kernel.org>, Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>, X86 ML <x86@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Dave Hansen <dave.hansen@intel.com>
Subject: Re: [PATCH v2] x86/mm/tlb: Remove flush_tlb_info from the stack
Thread-Topic: [PATCH v2] x86/mm/tlb: Remove flush_tlb_info from the stack
Thread-Index: AQHU+5HncRUmcKTEhES/sxCi8aqoJ6ZNProA
Date:   Thu, 25 Apr 2019 19:13:17 +0000
Message-ID: <F2218F7E-2011-4AAA-B3A3-F13A1A438204@vmware.com>
References: <20190425180828.24959-1-namit@vmware.com>
In-Reply-To: <20190425180828.24959-1-namit@vmware.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: vmware.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2817D4BEA80C3F4693347DEB60D2C8C6@namprd05.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: c0044e05-c183-4f19-48dc-08d6c9b212b7
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Apr 2019 19:13:17.3178
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR05MB6054
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> On Apr 25, 2019, at 11:08 AM, Nadav Amit <namit@vmware.com> wrote:
>=20
> Move flush_tlb_info variables off the stack. This allows to align
> flush_tlb_info to cache-line and avoid potentially unnecessary cache
> line movements. It also allows to have a fixed virtual-to-physical
> translation of the variables, which reduces TLB misses.
>=20
> Use per-CPU struct for flush_tlb_mm_range() and
> flush_tlb_kernel_range(). Add debug assertions to ensure there are
> no nested TLB flushes that might overwrite the per-CPU data. For
> arch_tlbbatch_flush() use a const struct.
>=20
> Results when running a microbenchmarks that performs 10^6 MADV_DONTEED
> operations and touching a page, in which 3 additional threads run a
> busy-wait loop (5 runs, PTI and retpolines are turned off):
>=20
> 			base		off-stack
> 			----		---------
> avg (usec/op)		1.629		1.570	(-3%)
> stddev			0.014		0.009
>=20
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Nadav Amit <namit@vmware.com>
>=20
> ---
>=20
> v1->v2:
> - Initialize all flush_tlb_info fields [Andy]
> ---
> arch/x86/mm/tlb.c | 100 ++++++++++++++++++++++++++++++++++------------
> 1 file changed, 74 insertions(+), 26 deletions(-)
>=20
> diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> index 487b8474c01c..aac191eb2b90 100644
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -634,7 +634,7 @@ static void flush_tlb_func_common(const struct flush_=
tlb_info *f,
> 	this_cpu_write(cpu_tlbstate.ctxs[loaded_mm_asid].tlb_gen, mm_tlb_gen);
> }
>=20
> -static void flush_tlb_func_local(void *info, enum tlb_flush_reason reaso=
n)
> +static void flush_tlb_func_local(const void *info, enum tlb_flush_reason=
 reason)
> {
> 	const struct flush_tlb_info *f =3D info;
>=20
> @@ -722,43 +722,81 @@ void native_flush_tlb_others(const struct cpumask *=
cpumask,
>  */
> unsigned long tlb_single_page_flush_ceiling __read_mostly =3D 33;
>=20
> +static DEFINE_PER_CPU_SHARED_ALIGNED(struct flush_tlb_info, flush_tlb_in=
fo);
> +
> +#ifdef CONFIG_DEBUG_VM
> +static DEFINE_PER_CPU(unsigned int, flush_tlb_info_idx);
> +#endif
> +
> +static inline struct flush_tlb_info *get_flush_tlb_info(struct mm_struct=
 *mm,
> +			unsigned long start, unsigned long end,
> +			unsigned int stride_shift, bool freed_tables,
> +			u64 new_tlb_gen)
> +{
> +	struct flush_tlb_info *info =3D this_cpu_ptr(&flush_tlb_info);
> +
> +#ifdef CONFIG_DEBUG_VM
> +	/*
> +	 * Ensure that the following code is non-reentrant and flush_tlb_info
> +	 * is not overwritten. This means no TLB flushing is initiated by
> +	 * interrupt handlers and machine-check exception handlers.
> +	 */
> +	BUG_ON(this_cpu_inc_return(flush_tlb_info_idx) !=3D 1);
> +#endif
> +
> +	info->start =3D start;
> +	info->end =3D end;
> +	info->mm =3D mm;
> +	info->stride_shift =3D stride_shift;
> +	info->freed_tables =3D freed_tables;
> +	info->new_tlb_gen =3D new_tlb_gen;
> +
> +	return info;
> +}
> +
> +static inline void put_flush_tlb_info(void)
> +{
> +#ifdef CONFIG_DEBUG_VM
> +	/* Complete reentrency prevention checks */
> +	barrier();
> +	this_cpu_dec(flush_tlb_info_idx);
> +#endif
> +}
> +
> void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
> 				unsigned long end, unsigned int stride_shift,
> 				bool freed_tables)
> {
> +	struct flush_tlb_info *info;
> +	u64 new_tlb_gen;
> 	int cpu;
>=20
> -	struct flush_tlb_info info =3D {
> -		.mm =3D mm,
> -		.stride_shift =3D stride_shift,
> -		.freed_tables =3D freed_tables,
> -	};
> -
> 	cpu =3D get_cpu();
>=20
> -	/* This is also a barrier that synchronizes with switch_mm(). */
> -	info.new_tlb_gen =3D inc_mm_tlb_gen(mm);
> -
> 	/* Should we flush just the requested range? */
> -	if ((end !=3D TLB_FLUSH_ALL) &&
> -	    ((end - start) >> stride_shift) <=3D tlb_single_page_flush_ceiling)=
 {
> -		info.start =3D start;
> -		info.end =3D end;
> -	} else {
> -		info.start =3D 0UL;
> -		info.end =3D TLB_FLUSH_ALL;
> +	if ((end =3D=3D TLB_FLUSH_ALL) ||
> +	    ((end - start) >> stride_shift) > tlb_single_page_flush_ceiling) {
> +		start =3D 0UL;
> +		end =3D TLB_FLUSH_ALL;
> 	}
>=20
> +	/* This is also a barrier that synchronizes with switch_mm(). */
> +	new_tlb_gen =3D inc_mm_tlb_gen(mm);
> +
> +	info =3D get_flush_tlb_info(mm, start, end, stride_shift, freed_tables,
> +				  new_tlb_gen);
> +
> 	if (mm =3D=3D this_cpu_read(cpu_tlbstate.loaded_mm)) {
> -		VM_WARN_ON(irqs_disabled());
> +		lockdep_assert_irqs_enabled();
> 		local_irq_disable();
> -		flush_tlb_func_local(&info, TLB_LOCAL_MM_SHOOTDOWN);
> +		flush_tlb_func_local(info, TLB_LOCAL_MM_SHOOTDOWN);
> 		local_irq_enable();
> 	}
>=20
> 	if (cpumask_any_but(mm_cpumask(mm), cpu) < nr_cpu_ids)
> -		flush_tlb_others(mm_cpumask(mm), &info);
> +		flush_tlb_others(mm_cpumask(mm), info);
>=20
> +	put_flush_tlb_info();
> 	put_cpu();
> }
>=20
> @@ -787,22 +825,32 @@ static void do_kernel_range_flush(void *info)
>=20
> void flush_tlb_kernel_range(unsigned long start, unsigned long end)
> {
> -
> 	/* Balance as user space task's flush, a bit conservative */
> 	if (end =3D=3D TLB_FLUSH_ALL ||
> 	    (end - start) > tlb_single_page_flush_ceiling << PAGE_SHIFT) {
> 		on_each_cpu(do_flush_tlb_all, NULL, 1);
> 	} else {
> -		struct flush_tlb_info info;
> -		info.start =3D start;
> -		info.end =3D end;
> -		on_each_cpu(do_kernel_range_flush, &info, 1);
> +		struct flush_tlb_info *info;
> +
> +		preempt_disable();
> +
> +		info =3D get_flush_tlb_info(NULL, start, end, 0, false, 0);
> +
> +		info =3D this_cpu_ptr(&flush_tlb_info);
> +		info->start =3D start;
> +		info->end =3D end;

Err.. This is wrong. I will send v3 shortly.