This commit creates a new mask, XFEATURE_MASK_ZMM, to test against
xfeatures for conditionally updating the axx512_timestamp.
Based on the comments, the avx512 state is meant to track when the
state would cause frequencey throttling. The opmasks (k0-k7) do not
cause frequency throttling, so they don't make sense to include.
The current implementation, as well as the old, still does have a
false positive on ymm16-ymm31 and xmm16-31 because
XFEATURE_MASK_Hi16_ZMM includes them.
Signed-off-by: Noah Goldstein <[email protected]>
---
Issue is reproducible with the following code on x86_64:
```
.global _start
.text
_start:
korq %k0, %k0, %k0
loop:
jmp loop
movl $60, %eax
xorl %edi, %edi
syscall
```
Pretending run as executable named "foo":
$> cat /proc/$(pidof foo)/arch_status
This should yield -1 as no frequency changing AVX512 instructions
where used but instead tracks the process.
Note there still is a false positive with ymm16-ymm31 and xmm16-xmm31
but since there is no state to distinguish between there use and
zmm16-31 that seems inevitable.
arch/x86/include/asm/fpu/types.h | 2 ++
arch/x86/kernel/fpu/core.c | 4 +++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index f5a38a5f3ae1..a4816fa7d541 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -141,6 +141,8 @@ enum xfeature {
#define XFEATURE_MASK_AVX512 (XFEATURE_MASK_OPMASK \
| XFEATURE_MASK_ZMM_Hi256 \
| XFEATURE_MASK_Hi16_ZMM)
+#define XFEATURE_MASK_ZMM (XFEATURE_MASK_ZMM_Hi256 \
+ | XFEATURE_MASK_Hi16_ZMM)
#define FIRST_EXTENDED_XFEATURE XFEATURE_YMM
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 7ada7bd03a32..342620a2e8ef 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -104,8 +104,10 @@ void save_fpregs_to_fpstate(struct fpu *fpu)
/*
* AVX512 state is tracked here because its use is
* known to slow the max clock speed of the core.
+ * Note: This has a false positive on Hi16 ymm and
+ * xmm registers.
*/
- if (fpu->state.xsave.header.xfeatures & XFEATURE_MASK_AVX512)
+ if (fpu->state.xsave.header.xfeatures & XFEATURE_MASK_ZMM)
fpu->avx512_timestamp = jiffies;
return;
}
--
2.25.1
On Mon, Sep 20, 2021 at 12:40 AM Noah Goldstein <[email protected]> wrote:
>
> This commit creates a new mask, XFEATURE_MASK_ZMM, to test against
> xfeatures for conditionally updating the axx512_timestamp.
>
> Based on the comments, the avx512 state is meant to track when the
> state would cause frequencey throttling. The opmasks (k0-k7) do not
> cause frequency throttling, so they don't make sense to include.
>
> The current implementation, as well as the old, still does have a
> false positive on ymm16-ymm31 and xmm16-31 because
> XFEATURE_MASK_Hi16_ZMM includes them.
>
> Signed-off-by: Noah Goldstein <[email protected]>
> ---
> Issue is reproducible with the following code on x86_64:
>
> ```
> .global _start
> .text
> _start:
> korq %k0, %k0, %k0
>
> loop:
> jmp loop
>
>
> movl $60, %eax
> xorl %edi, %edi
> syscall
> ```
>
> Pretending run as executable named "foo":
>
> $> cat /proc/$(pidof foo)/arch_status
>
>
> This should yield -1 as no frequency changing AVX512 instructions
> where used but instead tracks the process.
>
> Note there still is a false positive with ymm16-ymm31 and xmm16-xmm31
> but since there is no state to distinguish between there use and
> zmm16-31 that seems inevitable.
>
>
> arch/x86/include/asm/fpu/types.h | 2 ++
> arch/x86/kernel/fpu/core.c | 4 +++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
> index f5a38a5f3ae1..a4816fa7d541 100644
> --- a/arch/x86/include/asm/fpu/types.h
> +++ b/arch/x86/include/asm/fpu/types.h
> @@ -141,6 +141,8 @@ enum xfeature {
> #define XFEATURE_MASK_AVX512 (XFEATURE_MASK_OPMASK \
> | XFEATURE_MASK_ZMM_Hi256 \
> | XFEATURE_MASK_Hi16_ZMM)
> +#define XFEATURE_MASK_ZMM (XFEATURE_MASK_ZMM_Hi256 \
> + | XFEATURE_MASK_Hi16_ZMM)
>
> #define FIRST_EXTENDED_XFEATURE XFEATURE_YMM
>
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 7ada7bd03a32..342620a2e8ef 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -104,8 +104,10 @@ void save_fpregs_to_fpstate(struct fpu *fpu)
> /*
> * AVX512 state is tracked here because its use is
> * known to slow the max clock speed of the core.
> + * Note: This has a false positive on Hi16 ymm and
> + * xmm registers.
> */
> - if (fpu->state.xsave.header.xfeatures & XFEATURE_MASK_AVX512)
> + if (fpu->state.xsave.header.xfeatures & XFEATURE_MASK_ZMM)
> fpu->avx512_timestamp = jiffies;
> return;
> }
> --
> 2.25.1
>
Ping
(sorry if this has shown up multiple times for anyone,
was accidentally including HTML earlier)
On Mon, Sep 27, 2021 at 1:02 PM Noah Goldstein <[email protected]> wrote:
>
> On Mon, Sep 20, 2021 at 12:40 AM Noah Goldstein <[email protected]> wrote:
> >
> > This commit creates a new mask, XFEATURE_MASK_ZMM, to test against
> > xfeatures for conditionally updating the axx512_timestamp.
> >
> > Based on the comments, the avx512 state is meant to track when the
> > state would cause frequencey throttling. The opmasks (k0-k7) do not
> > cause frequency throttling, so they don't make sense to include.
> >
> > The current implementation, as well as the old, still does have a
> > false positive on ymm16-ymm31 and xmm16-31 because
> > XFEATURE_MASK_Hi16_ZMM includes them.
> >
> > Signed-off-by: Noah Goldstein <[email protected]>
> > ---
> > Issue is reproducible with the following code on x86_64:
> >
> > ```
> > .global _start
> > .text
> > _start:
> > korq %k0, %k0, %k0
> >
> > loop:
> > jmp loop
> >
> >
> > movl $60, %eax
> > xorl %edi, %edi
> > syscall
> > ```
> >
> > Pretending run as executable named "foo":
> >
> > $> cat /proc/$(pidof foo)/arch_status
> >
> >
> > This should yield -1 as no frequency changing AVX512 instructions
> > where used but instead tracks the process.
> >
> > Note there still is a false positive with ymm16-ymm31 and xmm16-xmm31
> > but since there is no state to distinguish between there use and
> > zmm16-31 that seems inevitable.
> >
> >
> > arch/x86/include/asm/fpu/types.h | 2 ++
> > arch/x86/kernel/fpu/core.c | 4 +++-
> > 2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
> > index f5a38a5f3ae1..a4816fa7d541 100644
> > --- a/arch/x86/include/asm/fpu/types.h
> > +++ b/arch/x86/include/asm/fpu/types.h
> > @@ -141,6 +141,8 @@ enum xfeature {
> > #define XFEATURE_MASK_AVX512 (XFEATURE_MASK_OPMASK \
> > | XFEATURE_MASK_ZMM_Hi256 \
> > | XFEATURE_MASK_Hi16_ZMM)
> > +#define XFEATURE_MASK_ZMM (XFEATURE_MASK_ZMM_Hi256 \
> > + | XFEATURE_MASK_Hi16_ZMM)
> >
> > #define FIRST_EXTENDED_XFEATURE XFEATURE_YMM
> >
> > diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> > index 7ada7bd03a32..342620a2e8ef 100644
> > --- a/arch/x86/kernel/fpu/core.c
> > +++ b/arch/x86/kernel/fpu/core.c
> > @@ -104,8 +104,10 @@ void save_fpregs_to_fpstate(struct fpu *fpu)
> > /*
> > * AVX512 state is tracked here because its use is
> > * known to slow the max clock speed of the core.
> > + * Note: This has a false positive on Hi16 ymm and
> > + * xmm registers.
> > */
> > - if (fpu->state.xsave.header.xfeatures & XFEATURE_MASK_AVX512)
> > + if (fpu->state.xsave.header.xfeatures & XFEATURE_MASK_ZMM)
> > fpu->avx512_timestamp = jiffies;
> > return;
> > }
> > --
> > 2.25.1
> >
> Ping
>
> (sorry if this has shown up multiple times for anyone,
> was accidentally including HTML earlier)
Ping2
On Wed, Oct 13, 2021 at 05:36:14PM -0500, Noah Goldstein wrote:
> Ping2
Why?
The original patch which added this abomination:
2f7726f95557 ("x86/fpu: Track AVX-512 usage of tasks")
says already:
the tracking mechanism is imprecise and can theoretically miss AVX-512
usage during context switch. But it has been measured to be precise
enough to be useful under real-world workloads like tensorflow and
linpack.
If higher precision is required, suggest user space tools to use the
PMU-based mechanisms in combination.
and as you've noticed, the high 16 regs would cause a false positive
too.
So what is the actual real-life use case for this?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Oct 14, 2021 at 3:28 AM Borislav Petkov <[email protected]> wrote:
>
> On Wed, Oct 13, 2021 at 05:36:14PM -0500, Noah Goldstein wrote:
> > Ping2
>
> Why?
This left me confused and annoyed for several hours because
the avx512 usage information on my applications didn't make
sense. Figured worth a patch for posterity.
>
> The original patch which added this abomination:
>
> 2f7726f95557 ("x86/fpu: Track AVX-512 usage of tasks")
>
> says already:
>
> the tracking mechanism is imprecise and can theoretically miss AVX-512
> usage during context switch. But it has been measured to be precise
> enough to be useful under real-world workloads like tensorflow and
> linpack.
>
> If higher precision is required, suggest user space tools to use the
> PMU-based mechanisms in combination.
This patch isn't about higher precision / false negatives.
It's about false positives.
>
> and as you've noticed, the high 16 regs would cause a false positive
> too.
>
> So what is the actual real-life use case for this?
No big project. There is a case for avoiding the extended avx512
registers (reg16..31) for various reasons (context switches,
code size, instruction limitations) that don't apply to mask registers.
Irrelevant of the still existing flaws, it makes the output more accurate.
Is there a cost to the change I am not seeing?
If not it seems to be a tradeoff between more accurate vs less accurate.
So why not take the former?
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Oct 14, 2021 at 10:49:25AM -0500, Noah Goldstein wrote:
> Is there a cost to the change I am not seeing?
The cost is having yet another macro in our crazy FPU code because
apparently everything but the kitchen sink is being put into xstates.
So I'd suggest you carve out that timestamp update into a separate
function, i.e., something like fpu_update_avx_timestamp() or so - and
add the macro in it along with the comment explaining what and why
is being tracked.
So that that functionality is separated out of the main flow,
save_fpregs_to_fpstate() simply calls it and it doesn't get in the way
of the next rewrite of the FPU code to accomodate new crap^W features.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 10/14/21 8:49 AM, Noah Goldstein wrote:
> Irrelevant of the still existing flaws, it makes the output more accurate.
>
> Is there a cost to the change I am not seeing?
We'd want to make sure that this doesn't break anything. It probably
won't, but it theoretically could.
For instance, if someone was doing:
avx512_foo();
xsave->xstate_bv &= ~XFEATURE_MASK_ZMMS;
XRSTOR(xsave, -1);
That would leave the opmask in place, but would lead to the ZMM
registers tracked as being in their init state.
This would be *very* unlikely, but it would be great if Aubrey (the
original avx512_timestamp patch author) could make sure that it doesn't
break anything.
Also, there's the side issue of AVX-256 use. AVX-256 uses the ZMM
registers which are a part of XFEATURE_MASK_AVX512, but does not incur
the same frequency penalties of the full 512-bit-wide instructions.
Since ZMM_Hi256 is the *only* ZMM state which is truly 512-bit-only, we
could argue that it's the only one we should consider.
Noah, thanks for bringing this up. I'm not opposed to your patch, but
let's just make sure that it doesn't break anything and also that we
shouldn't do a bit more at the same time (ignore Hi16_ZMM for
avx512_timestamp).
On Fri, Oct 15, 2021 at 10:25 AM Dave Hansen <[email protected]> wrote:
>
> On 10/14/21 8:49 AM, Noah Goldstein wrote:
> > Irrelevant of the still existing flaws, it makes the output more accurate.
> >
> > Is there a cost to the change I am not seeing?
>
> We'd want to make sure that this doesn't break anything. It probably
> won't, but it theoretically could.
>
> For instance, if someone was doing:
>
> avx512_foo();
> xsave->xstate_bv &= ~XFEATURE_MASK_ZMMS;
> XRSTOR(xsave, -1);
>
> That would leave the opmask in place, but would lead to the ZMM
> registers tracked as being in their init state.
The 'XFEATURE_MASK_ZMMS' is new to this patch so I don't think
this patch could be adding that issue.
>
> This would be *very* unlikely, but it would be great if Aubrey (the
> original avx512_timestamp patch author) could make sure that it doesn't
> break anything.
>
> Also, there's the side issue of AVX-256 use. AVX-256 uses the ZMM
> registers which are a part of XFEATURE_MASK_AVX512, but does not incur
> the same frequency penalties of the full 512-bit-wide instructions.
> Since ZMM_Hi256 is the *only* ZMM state which is truly 512-bit-only, we
> could argue that it's the only one we should consider.
>
> Noah, thanks for bringing this up. I'm not opposed to your patch, but
> let's just make sure that it doesn't break anything and also that we
> shouldn't do a bit more at the same time (ignore Hi16_ZMM for
> avx512_timestamp).
I think that may make sense. Or outputting separate timestamps for
both. Especially because in GLIBC we have moved to preferring
EVEX implementings for all x86_64 string functions because
vzeroupper aborts RTM transactions:
https://sourceware.org/bugzilla/show_bug.cgi?id=27457
So if an application is using GLIBC on an avx512 machine most likely
the avx512 indicator will be permanently set.
This commit adds a new helper function 'fpu_update_avx_timestamp' to
perform the logic from tracking AVX512 feature use.
Signed-off-by: Noah Goldstein <[email protected]>
---
Since Borislav Petkov is concerned about adding more macro masks and
inlining the AVX512 status this patch adds a new helper function. This
patch does not change the behavior of the current AVX512 status.
arch/x86/include/asm/fpu/xstate.h | 1 +
arch/x86/kernel/fpu/core.c | 6 ++----
arch/x86/kernel/fpu/xstate.c | 13 +++++++++++++
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 109dfcc75299..fe84ac5fb039 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -134,6 +134,7 @@ extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
extern void __init update_regset_xstate_info(unsigned int size,
u64 xstate_mask);
+void fpu_update_avx_timestamp(struct fpu *fpu);
void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr);
int xfeature_size(int xfeature_nr);
int copy_uabi_from_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf);
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 7ada7bd03a32..6dbf3ee642f9 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -102,11 +102,9 @@ void save_fpregs_to_fpstate(struct fpu *fpu)
os_xsave(&fpu->state.xsave);
/*
- * AVX512 state is tracked here because its use is
- * known to slow the max clock speed of the core.
+ * Track of the state of desired avx related xfeatures.
*/
- if (fpu->state.xsave.header.xfeatures & XFEATURE_MASK_AVX512)
- fpu->avx512_timestamp = jiffies;
+ fpu_update_avx_timestamp(fpu);
return;
}
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index c8def1b7f8fb..00b495914be2 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1245,6 +1245,19 @@ void xrstors(struct xregs_state *xstate, u64 mask)
WARN_ON_ONCE(err);
}
+/*
+ * Track of the state of desired avx architecture features.
+ */
+void fpu_update_avx_timestamp(struct fpu *fpu)
+{
+ /*
+ * AVX512 state is tracked here because its use is known to slow
+ * the max clock speed of the core.
+ */
+ if (fpu->state.xsave.header.xfeatures & XFEATURE_MASK_AVX512)
+ fpu->avx512_timestamp = jiffies;
+}
+
#ifdef CONFIG_PROC_PID_ARCH_STATUS
/*
* Report the amount of time elapsed in millisecond since last AVX512
--
2.25.1
On Fri, Oct 15, 2021 at 3:57 PM Noah Goldstein <[email protected]> wrote:
>
> This commit adds a new helper function 'fpu_update_avx_timestamp' to
> perform the logic from tracking AVX512 feature use.
>
> Signed-off-by: Noah Goldstein <[email protected]>
> ---
> Since Borislav Petkov is concerned about adding more macro masks and
> inlining the AVX512 status this patch adds a new helper function. This
> patch does not change the behavior of the current AVX512 status.
>
> arch/x86/include/asm/fpu/xstate.h | 1 +
> arch/x86/kernel/fpu/core.c | 6 ++----
> arch/x86/kernel/fpu/xstate.c | 13 +++++++++++++
> 3 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
> index 109dfcc75299..fe84ac5fb039 100644
> --- a/arch/x86/include/asm/fpu/xstate.h
> +++ b/arch/x86/include/asm/fpu/xstate.h
> @@ -134,6 +134,7 @@ extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
> extern void __init update_regset_xstate_info(unsigned int size,
> u64 xstate_mask);
>
> +void fpu_update_avx_timestamp(struct fpu *fpu);
> void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr);
> int xfeature_size(int xfeature_nr);
> int copy_uabi_from_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf);
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 7ada7bd03a32..6dbf3ee642f9 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -102,11 +102,9 @@ void save_fpregs_to_fpstate(struct fpu *fpu)
> os_xsave(&fpu->state.xsave);
>
> /*
> - * AVX512 state is tracked here because its use is
> - * known to slow the max clock speed of the core.
> + * Track of the state of desired avx related xfeatures.
> */
> - if (fpu->state.xsave.header.xfeatures & XFEATURE_MASK_AVX512)
> - fpu->avx512_timestamp = jiffies;
> + fpu_update_avx_timestamp(fpu);
> return;
> }
>
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index c8def1b7f8fb..00b495914be2 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1245,6 +1245,19 @@ void xrstors(struct xregs_state *xstate, u64 mask)
> WARN_ON_ONCE(err);
> }
>
> +/*
> + * Track of the state of desired avx architecture features.
> + */
> +void fpu_update_avx_timestamp(struct fpu *fpu)
> +{
> + /*
> + * AVX512 state is tracked here because its use is known to slow
> + * the max clock speed of the core.
> + */
> + if (fpu->state.xsave.header.xfeatures & XFEATURE_MASK_AVX512)
> + fpu->avx512_timestamp = jiffies;
> +}
> +
> #ifdef CONFIG_PROC_PID_ARCH_STATUS
> /*
> * Report the amount of time elapsed in millisecond since last AVX512
> --
> 2.25.1
>
Ping.
Add a new helper function 'fpu_update_avx_timestamp' to perform
the logic from tracking AVX512 feature use.
Signed-off-by: Noah Goldstein <[email protected]>
---
arch/x86/include/asm/fpu/xstate.h | 1 +
arch/x86/kernel/fpu/core.c | 6 ++----
arch/x86/kernel/fpu/xstate.c | 13 +++++++++++++
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 109dfcc75299..fe84ac5fb039 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -134,6 +134,7 @@ extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
extern void __init update_regset_xstate_info(unsigned int size,
u64 xstate_mask);
+void fpu_update_avx_timestamp(struct fpu *fpu);
void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr);
int xfeature_size(int xfeature_nr);
int copy_uabi_from_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf);
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 7ada7bd03a32..6dbf3ee642f9 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -102,11 +102,9 @@ void save_fpregs_to_fpstate(struct fpu *fpu)
os_xsave(&fpu->state.xsave);
/*
- * AVX512 state is tracked here because its use is
- * known to slow the max clock speed of the core.
+ * Track of the state of desired avx related xfeatures.
*/
- if (fpu->state.xsave.header.xfeatures & XFEATURE_MASK_AVX512)
- fpu->avx512_timestamp = jiffies;
+ fpu_update_avx_timestamp(fpu);
return;
}
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index c8def1b7f8fb..00b495914be2 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1245,6 +1245,19 @@ void xrstors(struct xregs_state *xstate, u64 mask)
WARN_ON_ONCE(err);
}
+/*
+ * Track of the state of desired avx architecture features.
+ */
+void fpu_update_avx_timestamp(struct fpu *fpu)
+{
+ /*
+ * AVX512 state is tracked here because its use is known to slow
+ * the max clock speed of the core.
+ */
+ if (fpu->state.xsave.header.xfeatures & XFEATURE_MASK_AVX512)
+ fpu->avx512_timestamp = jiffies;
+}
+
#ifdef CONFIG_PROC_PID_ARCH_STATUS
/*
* Report the amount of time elapsed in millisecond since last AVX512
--
2.25.1
This commit adds a new helper function 'fpu_update_avx_timestamp' to
perform the logic from tracking AVX512 feature use.
Signed-off-by: Noah Goldstein <[email protected]>
---
arch/x86/include/asm/fpu/xstate.h | 1 +
arch/x86/kernel/fpu/core.c | 6 ++----
arch/x86/kernel/fpu/xstate.c | 13 +++++++++++++
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 109dfcc75299..fe84ac5fb039 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -134,6 +134,7 @@ extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
extern void __init update_regset_xstate_info(unsigned int size,
u64 xstate_mask);
+void fpu_update_avx_timestamp(struct fpu *fpu);
void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr);
int xfeature_size(int xfeature_nr);
int copy_uabi_from_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf);
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 7ada7bd03a32..6dbf3ee642f9 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -102,11 +102,9 @@ void save_fpregs_to_fpstate(struct fpu *fpu)
os_xsave(&fpu->state.xsave);
/*
- * AVX512 state is tracked here because its use is
- * known to slow the max clock speed of the core.
+ * Track of the state of desired avx related xfeatures.
*/
- if (fpu->state.xsave.header.xfeatures & XFEATURE_MASK_AVX512)
- fpu->avx512_timestamp = jiffies;
+ fpu_update_avx_timestamp(fpu);
return;
}
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index c8def1b7f8fb..00b495914be2 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1245,6 +1245,19 @@ void xrstors(struct xregs_state *xstate, u64 mask)
WARN_ON_ONCE(err);
}
+/*
+ * Track of the state of desired avx architecture features.
+ */
+void fpu_update_avx_timestamp(struct fpu *fpu)
+{
+ /*
+ * AVX512 state is tracked here because its use is known to slow
+ * the max clock speed of the core.
+ */
+ if (fpu->state.xsave.header.xfeatures & XFEATURE_MASK_AVX512)
+ fpu->avx512_timestamp = jiffies;
+}
+
#ifdef CONFIG_PROC_PID_ARCH_STATUS
/*
* Report the amount of time elapsed in millisecond since last AVX512
--
2.25.1
Add a new helper function 'fpu_update_avx_timestamp' to perform
the logic from tracking AVX512 feature use.
Signed-off-by: Noah Goldstein <[email protected]>
---
arch/x86/include/asm/fpu/xstate.h | 1 +
arch/x86/kernel/fpu/core.c | 6 ++----
arch/x86/kernel/fpu/xstate.c | 13 +++++++++++++
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 109dfcc75299..fe84ac5fb039 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -134,6 +134,7 @@ extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
extern void __init update_regset_xstate_info(unsigned int size,
u64 xstate_mask);
+void fpu_update_avx_timestamp(struct fpu *fpu);
void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr);
int xfeature_size(int xfeature_nr);
int copy_uabi_from_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf);
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 7ada7bd03a32..6dbf3ee642f9 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -102,11 +102,9 @@ void save_fpregs_to_fpstate(struct fpu *fpu)
os_xsave(&fpu->state.xsave);
/*
- * AVX512 state is tracked here because its use is
- * known to slow the max clock speed of the core.
+ * Track of the state of desired avx related xfeatures.
*/
- if (fpu->state.xsave.header.xfeatures & XFEATURE_MASK_AVX512)
- fpu->avx512_timestamp = jiffies;
+ fpu_update_avx_timestamp(fpu);
return;
}
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index c8def1b7f8fb..00b495914be2 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1245,6 +1245,19 @@ void xrstors(struct xregs_state *xstate, u64 mask)
WARN_ON_ONCE(err);
}
+/*
+ * Track of the state of desired avx architecture features.
+ */
+void fpu_update_avx_timestamp(struct fpu *fpu)
+{
+ /*
+ * AVX512 state is tracked here because its use is known to slow
+ * the max clock speed of the core.
+ */
+ if (fpu->state.xsave.header.xfeatures & XFEATURE_MASK_AVX512)
+ fpu->avx512_timestamp = jiffies;
+}
+
#ifdef CONFIG_PROC_PID_ARCH_STATUS
/*
* Report the amount of time elapsed in millisecond since last AVX512
--
2.25.1
The following commit has been merged into the x86/fpu branch of tip:
Commit-ID: 0fe4ff885f8a50082d9dc241b657472894caba16
Gitweb: https://git.kernel.org/tip/0fe4ff885f8a50082d9dc241b657472894caba16
Author: Noah Goldstein <[email protected]>
AuthorDate: Tue, 16 Nov 2021 17:14:21 +01:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Tue, 16 Nov 2021 17:19:41 +01:00
x86/fpu: Correct AVX512 state tracking
Add a separate, local mask for tracking AVX512 usage which does not
include the opmask xfeature set. Opmask registers usage does not cause
frequency throttling so it is a completely unnecessary false positive.
While at it, carve it out into a separate function to keep that
abomination extracted out.
[ bp: Rediff and cleanup ontop of 5.16-rc1. ]
Signed-off-by: Noah Goldstein <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/kernel/fpu/core.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 8ea306b..dd3777a 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -99,6 +99,19 @@ bool irq_fpu_usable(void)
EXPORT_SYMBOL(irq_fpu_usable);
/*
+ * Track AVX512 state use because it is known to slow the max clock
+ * speed of the core.
+ */
+static void update_avx_timestamp(struct fpu *fpu)
+{
+
+#define AVX512_TRACKING_MASK (XFEATURE_MASK_ZMM_Hi256 | XFEATURE_MASK_Hi16_ZMM)
+
+ if (fpu->fpstate->regs.xsave.header.xfeatures & AVX512_TRACKING_MASK)
+ fpu->avx512_timestamp = jiffies;
+}
+
+/*
* Save the FPU register state in fpu->fpstate->regs. The register state is
* preserved.
*
@@ -116,13 +129,7 @@ void save_fpregs_to_fpstate(struct fpu *fpu)
{
if (likely(use_xsave())) {
os_xsave(fpu->fpstate);
-
- /*
- * AVX512 state is tracked here because its use is
- * known to slow the max clock speed of the core.
- */
- if (fpu->fpstate->regs.xsave.header.xfeatures & XFEATURE_MASK_AVX512)
- fpu->avx512_timestamp = jiffies;
+ update_avx_timestamp(fpu);
return;
}