Re: [PATCH v3 05/21] x86/fpu/xstate: Add a new variable to indicate dynamic user states

From: "Bae, Chang Seok" <chang.seok.bae@intel.com>
To: Borislav Petkov <bp@suse.de>
Cc: Andy Lutomirski <luto@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@kernel.org>, x86-ml <x86@kernel.org>,
	"Brown, Len" <len.brown@intel.com>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	"Liu, Jing2" <jing2.liu@intel.com>,
	"Shankar, Ravi V" <ravi.v.shankar@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 05/21] x86/fpu/xstate: Add a new variable to indicate dynamic user states
Date: Tue, 19 Jan 2021 18:57:26 +0000	[thread overview]
Message-ID: <EF7FB3DA-868D-4597-B841-F786E094BFCF@intel.com> (raw)
In-Reply-To: <20210119155758.GF27634@zn.tnic>

On Jan 19, 2021, at 07:57, Borislav Petkov <bp@suse.de> wrote:
> On Fri, Jan 15, 2021 at 07:47:38PM +0000, Bae, Chang Seok wrote:
>> On Jan 15, 2021, at 05:39, Borislav Petkov <bp@suse.de> wrote:
>>> On Wed, Dec 23, 2020 at 07:57:01AM -0800, Chang S. Bae wrote:
>>>> The perf has a buffer that is allocated on demand. The states saved in the
>>> 
>>> What's "the perf"? I hope to find out when I countinue reading…
>> 
>> Maybe it was better to write ‘Linux perf (tools)’ [1] here. Sorry for the
>> confusion.
> 
> Well, I'm also confused as to what does the perf buffer have to do with
> AMX?

This series attempts to save the AMX state in the context switch buffer only
when needed -- so it is called out ‘dynamic’ user states.

The LBR state is saved in the perf buffer [1], and this state is named
'dynamic' supervisor states [2]. But some naming in the change has ‘dynamic’
state only.

So, these two kinds of dynamic states are different and need to be named
clearly.

>>>> -#define XFEATURE_MASK_DYNAMIC (XFEATURE_MASK_LBR)
>>>> +#define XFEATURE_MASK_SUPERVISOR_DYNAMIC (XFEATURE_MASK_LBR)
>>> 
>>> Is XFEATURE_MASK_USER_DYNAMIC coming too?
>> 
>> Instead of the new define, I thought the new variable --
>> xfeatures_mask_user_dynamic, as its value needs to be determined at boot
>> time.
> 
> Why isn't that in your commit message?

I will add it on my next revision.

> All I see a patch doing a bunch of renames, some unrelated blurb in the
> commit message and I have no clue what's going on here and why you're
> doing this. Your commit messages should contain simple english sentences
> and explain *why* the change is needed - not *what* you're doing. The
> *what* I can see from the diff itself, for the *why* I need a crystal
> ball which I can't buy in any store.
> 
> So how about explaining the *why*?

How about the changelog message like this:

"
The context switch buffer is in preparation to be dynamic for user states.
Introduce a new mask variable to indicate the 'dynamic' user states. The value
is determined at boot time.

The perf subsystem has a separate buffer to save some states only when needed,
not in every context switch. The states are named as 'dynamic' supervisor
states. Some define and helper are not named with dynamic supervisor states,
so rename them.

No functional change.
“

Thanks,
Chang

[1] https://lore.kernel.org/lkml/1593780569-62993-21-git-send-email-kan.liang@linux.intel.com/
[2] https://lore.kernel.org/lkml/1593780569-62993-22-git-send-email-kan.liang@linux.intel.com/