Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

From: Thomas Gleixner <tglx@linutronix.de>
To: Yu-cheng Yu <yu-cheng.yu@intel.com>, Borislav Petkov <bp@alien8.de>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org,
	"H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Tony Luck <tony.luck@intel.com>,
	Andy Lutomirski <luto@kernel.org>,
	Rik van Riel <riel@surriel.com>,
	"Ravi V. Shankar" <ravi.v.shankar@intel.com>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	Fenghua Yu <fenghua.yu@intel.com>,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()
Date: Tue, 10 Mar 2020 22:16:55 +0100	[thread overview]
Message-ID: <87a74n6hbs.fsf@nanos.tec.linutronix.de> (raw)
In-Reply-To: <e62e968c0980b091d7b263401ddd10162773678f.camel@intel.com>

Yu-cheng Yu <yu-cheng.yu@intel.com> writes:
> On Fri, 2020-03-06 at 21:50 +0100, Borislav Petkov wrote:
> Earlier you wrote:
>
>   53973 / (3*60 + 35) =~ 251 XSAVES invocations per second!
>
> I would argue that the kernel does much more than that for context
> switches.

And that argument is completly bogus. You deliberately ignored that
Boris said that this is just a boring kernel build which is definitely
not a signal heavy workload. And just if you look at what the build
does, it spends a huge amount if time in ld which is not using signals
at all.

> These are from:
>   perf record -a make -j32 bzImage

How useful: This is a systemwide record and not a recording of the
workload. Try again and try with and without your stuff.

Even if you do it proper, those numbers are uninteresting, really.

What do they tell? Nothing but a picture of a workload which is known to
be not signal heavy. You can also run while(1); for 20 seconds thats
equally useful.

Run a signal benchmark and then show the numbers before and after and
chose a benchmark which is not running on a single CPU, chose one which
is bouncing signals back and forth between two busy threads pinned on
different CPUs.

> Consider this and later maintenance, I think copy_xregs_to_kernel() is at
> least not worse than saving each state separately.

Please provide precise numbers taken with TSC for both variants just for
that particular code path.

And these numbers will only tell half of the story because they still do
not take the cache foot print into account. 1000 bytes just to throw
away 960 of them?

While I appreciate that you worry about maintenance I can assure you
that the code will be in a maintainable state at the point where it is
going to be merged.

Thanks,

        tglx