LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: "Chang S. Bae" <chang.seok.bae@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>, Andy Lutomirski <luto@kernel.org>,
	"H . Peter Anvin" <hpa@zytor.com>,
	Andi Kleen <ak@linux.intel.com>,
	Ravi Shankar <ravi.v.shankar@intel.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	x86@kernel.org
Subject: Re: [RESEND PATCH v6 04/12] x86/fsgsbase/64: Enable FSGSBASE instructions in the helper functions
Date: Mon, 25 Mar 2019 12:38:35 +0100 (CET)
Message-ID: <alpine.DEB.2.21.1903251107300.1798@nanos.tec.linutronix.de> (raw)
In-Reply-To: <1552680405-5265-5-git-send-email-chang.seok.bae@intel.com>

On Fri, 15 Mar 2019, Chang S. Bae wrote:

> The helper functions will switch on faster accesses to FSBASE and GSBASE
> when the FSGSBASE feature is enabled.
> 
> Accessing user GSBASE needs a couple of SWAPGS operations. It is avoidable
> if the user GSBASE is saved at kernel entry, being updated as changes, and
> restored back at kernel exit. However, it seems to spend more cycles for
> savings and restorations. Little or no benefit was measured from
> experiments.

This smells fishy and looking at the end result of this series just
confirms it. This ends up being a mixture of SWAPGS and FSGSBASE usage and
as already pointed out in the other reply, it causes inconsistencies.

Let's look at the big picture.

For both variants GS needs to be swapped on kernel entry and on kernel
exit.

1) SWAPGS

   MSR_KERNEL_GS_BASE contains the user space GS when running in the kernel
   and the kernel GS when running in user space.

   SWAPGS is used to swap the content of GS and MSR_KERNEL_GS_BASE on the
   transitions from and to user space.

   On context switch MSR_KERNEL_GS_BASE has to be updated when switching
   between processes.

   User space cannot change GS other than through the PRCTL which updates
   MSR_KERNEL_GS_BASE.

2) FSGSBASE

   User space can set GS without kernel interaction.

   So on user space to kernel space transitions swapping in kernel GS should
   simply do:

     userGS = RDGSBASE()
     WRGSBASE(kernelGS)

   and on the way out:

     WRGSBASE(userGS)

   instead of SWAPGS all over the place.

   userGS is stored in thread_struct, except for the few paranoid
   exceptions which return straight to user space, e.g. NMI. Those can just
   keep it on stack or in a register.

   Context switch does not have to do anything at all vs. GS because
   thread_struct contains the correct value already.

   The PRCTL is straight forward to support. Instead of fiddling with
   MSR_KERNEL_GS_BASE it just updates thread struct.

   I don't see how that's NOT going to be an advantage and I don't see
   either how this seems to cause more cycles for save and restore.

Making it consistently FSGSBASE avoids especially this piece of art in the
context switch path:

        local_irq_save(flags);
        native_swapgs();
        gsbase = rdgsbase();
        native_swapgs();
        local_irq_restore(flags);

along with it's write counterpart.

The whole point of FSGSBASE support is performance, right?

So can please someone explain why having the following in the context
switch path when it can be completely avoided is enhancing performance:

  - 4 x SWAPGS
  - 1 x RDMSR
  - 1 x WRMSR
  - 2 x local_irq_save()
  - 2 x local_irq_restore()

Of course the local_irq_save/restore() pairs are utterly pointless because
switch_to() runs with interrupts disabled already.

SWAPGS instead needs:

  1 x WRMSR

and nothing else.

So trading the single WRMSR against the above in the context switch path is
gaining performance, right?

The only thing which gains performance is user space switching GS. And this
user space performance gain is achieved by:

  - Inconsistent and fragile code with a guarantee for subtle and hard to
    diagnose bugs

  - Pointless overhead in the context switch code

Sorry, not going to happen ever.

Get your act together and make this consistent. Either SWAPGS or FSGSBASE,
but not a mix of it.

Thanks,

	tglx

  reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-15 20:06 [RESEND PATCH v6 00/12] x86: Enable FSGSBASE instructions Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 01/12] x86/fsgsbase/64: Add 'unsafe_fsgsbase' to enable CR4.FSGSBASE Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 02/12] kbuild: Raise the minimum required binutils version to 2.21 Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 03/12] x86/fsgsbase/64: Add intrinsics for FSGSBASE instructions Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 04/12] x86/fsgsbase/64: Enable FSGSBASE instructions in the helper functions Chang S. Bae
2019-03-25 11:38   ` Thomas Gleixner [this message]
2019-03-25 12:46     ` Thomas Gleixner
2019-03-25 13:05       ` Thomas Gleixner
2019-03-26  0:38     ` Andi Kleen
2019-03-26 15:01       ` New feature/ABI review process [was Re: [RESEND PATCH v6 04/12] x86/fsgsbase/64:..] Thomas Gleixner
2019-03-26 22:56         ` Andi Kleen
2019-03-27 21:15           ` Thomas Gleixner
2019-03-15 20:06 ` [RESEND PATCH v6 05/12] x86/fsgsbase/64: Preserve FS/GS state in __switch_to() if FSGSBASE is on Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 06/12] x86/fsgsbase/64: When copying a thread, use the FSGSBASE instructions if available Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 07/12] x86/fsgsbase/64: Introduce the FIND_PERCPU_BASE macro Chang S. Bae
2019-03-25  9:02   ` Thomas Gleixner
2019-05-01 13:52     ` Bae, Chang Seok
2019-03-15 20:06 ` [RESEND PATCH v6 08/12] x86/fsgsbase/64: Use the per-CPU base as GSBASE at the paranoid_entry Chang S. Bae
2019-03-25  9:44   ` Thomas Gleixner
2019-04-05  8:35     ` Thomas Gleixner
2019-04-05 13:50       ` Andy Lutomirski
2019-05-01 13:52         ` Bae, Chang Seok
2019-05-01 17:40           ` Andy Lutomirski
2019-05-01 18:01             ` Bae, Chang Seok
     [not found]               ` <7029A32B-958E-4C1E-8B5F-D49BA68E4755@intel.com>
2019-05-01 20:25                 ` Andy Lutomirski
2019-05-01 21:04                   ` Bae, Chang Seok
2019-05-02  0:29                     ` Andy Lutomirski
2019-05-06 22:56     ` Bae, Chang Seok
2019-03-15 20:06 ` [RESEND PATCH v6 09/12] selftests/x86/fsgsbase: Test WRGSBASE Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 10/12] x86/fsgsbase/64: Enable FSGSBASE by default and add a chicken bit Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 11/12] x86/elf: Enumerate kernel FSGSBASE capability in AT_HWCAP2 Chang S. Bae
2019-03-15 20:06 ` [RESEND PATCH v6 12/12] x86/fsgsbase/64: Add documentation for FSGSBASE Chang S. Bae
2019-03-30 16:15   ` Randy Dunlap
2019-03-26  0:43 ` [RESEND PATCH v6 00/12] x86: Enable FSGSBASE instructions Andy Lutomirski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.21.1903251107300.1798@nanos.tec.linutronix.de \
    --to=tglx@linutronix.de \
    --cc=ak@linux.intel.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=chang.seok.bae@intel.com \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mingo@kernel.org \
    --cc=ravi.v.shankar@intel.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git