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;
Message-ID: <1530216318.16379.4.camel@surriel.com>
Subject: Re: [PATCH 3/6] x86,tlb: make lazy TLB mode lazier
From:   Rik van Riel <riel@surriel.com>
To:     Andy Lutomirski <luto@kernel.org>
Cc:     LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Ingo Molnar <mingo@kernel.org>,
        kernel-team <kernel-team@fb.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Mike Galbraith <efault@gmx.de>, songliubraving@fb.com
Date:   Thu, 28 Jun 2018 16:05:18 -0400
In-Reply-To: <CALCETrVgDXe6NFCmxhyiwMEQmQqyidAh+wQBWeaCKg+z6jQOPw@mail.gmail.com>
References: <20180626173126.12296-1-riel@surriel.com>
         <20180626173126.12296-4-riel@surriel.com>
         <CALCETrVgDXe6NFCmxhyiwMEQmQqyidAh+wQBWeaCKg+z6jQOPw@mail.gmail.com>
Content-Type: multipart/signed; micalg="pgp-sha256";
        protocol="application/pgp-signature"; boundary="=-gt597znUsnfNZWhHLAGr"
Mime-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--=-gt597znUsnfNZWhHLAGr
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, 2018-06-27 at 11:10 -0700, Andy Lutomirski wrote:
>=20
> You left this comment:
>=20
>                 /*
>                  * We don't currently support having a real mm loaded
> without
>                  * our cpu set in mm_cpumask().  We have all the
> bookkeeping
>                  * in place to figure out whether we would need to
> flush
>                  * if our cpu were cleared in mm_cpumask(), but we
> don't
>                  * currently use it.
>                  */
>=20
> Presumably you should either clear the cpu from mm_cpumask when lazy
> or you shoudl update the comment.

The lazy TLB mode leaves the mm loaded, AND the
cpu set in mm_cpumask(). However, I guess while
the comment is technically accurate, it is no longer
relevant, so I will update it :)

> > +               /*
> > +                * Switching straight from one thread in a process
> > to another
> > +                * thread in the same process requires no TLB flush
> > at all.
> > +                */
> > +               if (!was_lazy)
> > +                       return;
>=20
> Comment doesn't match code.  Maybe add "... if we weren't lazy"?

Done.

> > +
> > +               /*
> > +                * The code below checks whether there was a TLB
> > flush while
> > +                * this CPU was in lazy TLB mode. The barrier
> > ensures ordering
> > +                * with the TLB invalidation code advancing
> > .tlb_gen.
> > +                */
> > +               smp_rmb();
>=20
> I think it may need to be smp_mb().  You're trying to order
> this_cpu_write() against subsequent reads.

I have updated the barrier to an smp_mb().

> In general, the changes to this function are very hard to review
> because you're mixing semantic changes and restructuring the
> function.
> Is there any way you could avoid that?  Or maybe just open-code a
> tlb_gen check in the unlazying path?
>=20
>=20
> > +       /*
> > +        * Instead of sending IPIs to CPUs in lazy TLB mode, move
> > that
> > +        * CPU's TLB state to TLBSTATE_FLUSH, causing the TLB to be
> > flushed
> > +        * at the next context switch, or at page table free time.
> > +        */
>=20
> Stale comment?

Will fix.

I am running some last tests now, and will send
v3 soon.

--=20
All Rights Reversed.
--=-gt597znUsnfNZWhHLAGr
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part
Content-Transfer-Encoding: 7bit

-----BEGIN PGP SIGNATURE-----

iQEzBAABCAAdFiEEKR73pCCtJ5Xj3yADznnekoTE3oMFAls1P34ACgkQznnekoTE
3oPEngf/dwHqRR+66WXIupND84IXM2X5ZB/5AIuhWTh58dUTCjlW0HCFSB0Pq2UG
6S0O/fxhbF1LXFX+vPguajrEzNYLUfSwX7SQo0QxtCn3GKJ5HCmyMqg9UMqgNB85
0HRLeR+bwIbrspsEqJhCIjIKn7L3Qhzh4FP60BAuNl83kp0gD9gGlDW0PVcHY2VY
03/dmUc66ZJzvKz8k7b1bwO/8SbwR/PkfAGTj7X/DLNcD6qEWZ9JgUnqHpQk08Ig
vhau2aPVbGFOCaqAHIFOXiEDYAJam73JQSxL+Fj7Fd89b4JEQungpQZzjxw7uo4e
/aygOzYxfQkdMdci7f9ttHPcelW7Qw==
=5M0S
-----END PGP SIGNATURE-----

--=-gt597znUsnfNZWhHLAGr--