From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932169AbcH2QI6 (ORCPT ); Mon, 29 Aug 2016 12:08:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40732 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751314AbcH2QI5 (ORCPT ); Mon, 29 Aug 2016 12:08:57 -0400 Message-ID: <1472486932.32433.103.camel@redhat.com> Subject: Re: [PATCH RFC UGLY] x86,mm,sched: make lazy TLB mode even lazier From: Rik van Riel To: "H. Peter Anvin" , serebrin@google.com Cc: mingo@kernel.org, peterz@infradead.org, torvalds@kernel.org, linux-kernel@vger.kernel.org, luto@kernel.org, bp@suse.de, mgorman@suse.de, tglx@linutronix.de Date: Mon, 29 Aug 2016 12:08:52 -0400 In-Reply-To: <7E7CF02F-F0B1-493A-98B3-B078174811DA@zytor.com> References: <20160825150515.02c2d8ea@riellap.home.surriel.com> <7E7CF02F-F0B1-493A-98B3-B078174811DA@zytor.com> Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="=-DEPLWnculbanbOs6HgJ+" Mime-Version: 1.0 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Mon, 29 Aug 2016 16:08:56 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-DEPLWnculbanbOs6HgJ+ Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 2016-08-25 at 12:42 -0700, H. Peter Anvin wrote: > > +static void set_lazy_tlbstate_flush(int cpu) { > > + if (per_cpu(cpu_tlbstate.state, cpu) =3D=3D TLBSTATE_LAZY) { > > + raw_spin_lock(&cpu_rq(cpu)->lock); > > + if (per_cpu(cpu_tlbstate.state, cpu) =3D=3D > > TLBSTATE_LAZY) > > + per_cpu(cpu_tlbstate.state, cpu) =3D > > TLBSTATE_FLUSH; > > + raw_spin_unlock(&cpu_rq(cpu)->lock); > > + } > > +} > > + > >=C2=A0 > Why grabbing a lock instead of cmpxchg? The second and third version of the patch had cmpxchg, instead of grabbing the remote CPU's runqueue lock, but I am no longer convinced it is safe. At TLB invalidation time, we have this: int *tlbstate =3D &per_cpu(cpu_tlbstate.state, cpu); int old; switch (*tlbstate) { case TLBSTATE_LAZY: /* * The CPU is in TLBSTATE_LAZY, which could context switch = back * to TLBSTATE_OK, re-using the old TLB state without a flu= sh. * If that happened, send a TLB flush IPI. * * Otherwise, the state is now TLBSTATE_FLUSH, and TLB will * be flushed at the next context switch. Skip the IPI. */=20 old =3D cmpxchg(tlbstate, TLBSTATE_LAZY, TLBSTATE_FLUSH); return old !=3D TLBSTATE_OK; At context switch time, we have this: int oldstate =3D this_cpu_read(cpu_tlbstate.state); this_cpu_write(cpu_tlbstate.state, TLBSTATE_OK); BUG_ON(this_cpu_read(cpu_tlbstate.active_mm) !=3D next); if (oldstate =3D=3D TLBSTATE_FLUSH || !cpumask_test_cpu(cpu, mm_cpumask(next))) { In each case, the read will happen before the write, because they are to the same address. If the invalidate and context switch happen concurrently, the writes can be ordered in two directions: 1) The cmpxchg in the TLB flush code happens after the this_cpu_write in the context switch code. This is safe. 2) The cmpxchg in the TLB flush code happens before the this_cpu_write in the context switch code. This is broken. I can see two ways to fix that: 1) Change the write in the context switch code to a cmpxchg. I do not know how expensive this is on modern CPUs, or whether the overhead of doing this is unacceptable (or even noticeable, considering the cache line needs to be acquired for write anyway). 2) Acquire the runqueue lock of the remote CPU from the (much rarer?) TLB flush code, in order to ensure it does not run concurrently with the context switch code. Any preferences? --=20 All Rights Reversed. --=-DEPLWnculbanbOs6HgJ+ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJXxF4UAAoJEM553pKExN6DlrMH/iRUvLOhWG59tCXeZSbhGHE7 w425excGvx3uYwZeaWW9CfZqGsCtqO20RHHbcRtmHeK0O73ZB6yQ5yXlAWpkEDSG yuSTSjXwszPESXQjGSCGsDl2ojbZneOMROPLltH14qUjHL4OBB4KGR292D1sOk+Q HQ/o5Fbf/ujb8KVeTNkWnYvibkbS2iOOdYq5JgNS4E15E3CRCSLgC2GA+h3TM1m5 geDEA8Xg/+qDksY2al2tSMC22OfuACMtMvWe5k+xYeYcvkyxpCyVcIEMlOxBT8n2 qGXhfOUB6mUT8QtrPcadF8bs2RCxPieMNE+aeSqMqevj+m8hJKUXaYiXG69hylY= =D4+c -----END PGP SIGNATURE----- --=-DEPLWnculbanbOs6HgJ+--