Summary: | Invalid x86_non_temporal_threshold without cache info | ||
---|---|---|---|
Product: | glibc | Reporter: | Alexis Engelke <engelke> |
Component: | dynamic-link | Assignee: | Not yet assigned to anyone <unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | minor | CC: | decui, fw, fweimer, goldstein.w.n, hjl.tools |
Priority: | P2 | Flags: | fweimer:
security-
|
Version: | 2.36 | ||
Target Milestone: | 2.37 | ||
See Also: |
https://sourceware.org/bugzilla/show_bug.cgi?id=30429 https://sourceware.org/bugzilla/show_bug.cgi?id=30428 https://sourceware.org/bugzilla/show_bug.cgi?id=30643 |
||
Host: | Target: | ||
Build: | Last reconfirmed: | ||
Attachments: |
Add minval
Ensure minimum value A patch Fix with new API for clamping tunable value |
Description
Alexis Engelke
2023-01-02 17:33:37 UTC
Created attachment 14545 [details]
Add minval
(In reply to Noah Goldstein from comment #1) > Created attachment 14545 [details] > Add minval Sorry wasn't clear, does that fix the issue? Created attachment 14547 [details] Ensure minimum value (In reply to Noah Goldstein from comment #2) > Sorry wasn't clear, does that fix the issue? No. Now --list-tunables reports: glibc.cpu.x86_non_temporal_threshold: 0x0 (min: 0x4040, max: 0xffffffffffffffff) The attached patch fixes the problem that the default value is never checked against the limits. Created attachment 14551 [details]
A patch
Try this.
(In reply to H.J. Lu from comment #4) > Created attachment 14551 [details] > A patch > > Try this. Oh wow, I had always assumed `TUNABLE_SET_WITH_BOUNDS` would just clamp the value. We also have a bug with `x86_rep_movsb_threshold` which requires at least 128 (2x AVX512 aligning vectors). (In reply to Noah Goldstein from comment #5) > We also have a bug with `x86_rep_movsb_threshold` which requires at least > 128 (2x AVX512 aligning vectors). x86_rep_movsb_threshold should be fine. minimum_rep_movsb_threshold should be set and checked properly. (In reply to H.J. Lu from comment #6) > (In reply to Noah Goldstein from comment #5) > > We also have a bug with `x86_rep_movsb_threshold` which requires at least > > 128 (2x AVX512 aligning vectors). > > x86_rep_movsb_threshold should be fine. minimum_rep_movsb_threshold should > be set and checked properly. Seems like the exact same issue no? ``` tunable_size = TUNABLE_GET (x86_rep_movsb_threshold, long int, NULL); if (tunable_size > minimum_rep_movsb_threshold) rep_movsb_threshold = tunable_size; // if tunable_size was zero, rep_movsb_threshold == 0 ... // Won't clamp, will leave x86_rep_movsb_threshold at zero. TUNABLE_SET_WITH_BOUNDS (x86_rep_movsb_threshold, rep_movsb_threshold, minimum_rep_movsb_threshold, SIZE_MAX); ``` (In reply to Noah Goldstein from comment #7) > (In reply to H.J. Lu from comment #6) > > (In reply to Noah Goldstein from comment #5) > > > We also have a bug with `x86_rep_movsb_threshold` which requires at least > > > 128 (2x AVX512 aligning vectors). > > > > x86_rep_movsb_threshold should be fine. minimum_rep_movsb_threshold should > > be set and checked properly. > > Seems like the exact same issue no? > > ``` > tunable_size = TUNABLE_GET (x86_rep_movsb_threshold, long int, NULL); > if (tunable_size > minimum_rep_movsb_threshold) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > rep_movsb_threshold = tunable_size; > > // if tunable_size was zero, rep_movsb_threshold == 0 Since minimum_rep_movsb_threshold is > 0, rep_movsb_threshold won't be zero. > > ... > > // Won't clamp, will leave x86_rep_movsb_threshold at zero. > TUNABLE_SET_WITH_BOUNDS (x86_rep_movsb_threshold, rep_movsb_threshold, > minimum_rep_movsb_threshold, SIZE_MAX); > > > > > ``` Created attachment 14552 [details]
Fix with new API for clamping tunable value
Personally more infavor of an approach like this.
Seems a bit insane that when we have a minval > 0, we default to potentially leaving the tunable at zero.
(In reply to Noah Goldstein from comment #9) > Created attachment 14552 [details] > Fix with new API for clamping tunable value > > Personally more infavor of an approach like this. > > Seems a bit insane that when we have a minval > 0, we default to potentially > leaving the tunable at zero. TUNABLE_SET_WITH_BOUNDS is used to update dynamic bounds for display purpose. The tunable value may be retrieved before TUNABLE_SET_WITH_BOUNDS. (In reply to H.J. Lu from comment #4) > Try this. This fixes the problem, thanks. (In reply to Noah Goldstein from comment #9) > Created attachment 14552 [details] > Fix with new API for clamping tunable value This also fixes the bug when tunables are enabled, but has the problem that no clamping will happen when tunables are disabled. (In reply to Alexis Engelke from comment #11) > (In reply to H.J. Lu from comment #4) > > Try this. > This fixes the problem, thanks. > > (In reply to Noah Goldstein from comment #9) > > Created attachment 14552 [details] > > Fix with new API for clamping tunable value > This also fixes the bug when tunables are enabled, but has the problem that > no clamping will happen when tunables are disabled. We can go with H.J's fix, making the tunable setter clamping can be an independent QOL change later on. The master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=48b74865c63840b288bd85b4d8743533b73b339b commit 48b74865c63840b288bd85b4d8743533b73b339b Author: H.J. Lu <hjl.tools@gmail.com> Date: Tue Jan 3 13:06:48 2023 -0800 x86: Check minimum/maximum of non_temporal_threshold [BZ #29953] The minimum non_temporal_threshold is 0x4040. non_temporal_threshold may be set to less than the minimum value when the shared cache size isn't available (e.g., in an emulator) or by the tunable. Add checks for minimum and maximum of non_temporal_threshold. This fixes BZ #29953. *** Bug 30037 has been marked as a duplicate of this bug. *** (In reply to cvs-commit@gcc.gnu.org from comment #13) > The master branch has been updated by H.J. Lu <hjl@sourceware.org>: > > https://sourceware.org/git/gitweb.cgi?p=glibc.git; > h=48b74865c63840b288bd85b4d8743533b73b339b > > commit 48b74865c63840b288bd85b4d8743533b73b339b > Author: H.J. Lu <hjl.tools@gmail.com> > Date: Tue Jan 3 13:06:48 2023 -0800 > > x86: Check minimum/maximum of non_temporal_threshold [BZ #29953] > > The minimum non_temporal_threshold is 0x4040. non_temporal_threshold may > be set to less than the minimum value when the shared cache size isn't > available (e.g., in an emulator) or by the tunable. Add checks for > minimum and maximum of non_temporal_threshold. > > This fixes BZ #29953. H.J., do you intend to backport this? Can we close this bug? (In reply to Florian Weimer from comment #15) > (In reply to cvs-commit@gcc.gnu.org from comment #13) > > The master branch has been updated by H.J. Lu <hjl@sourceware.org>: > > > > https://sourceware.org/git/gitweb.cgi?p=glibc.git; > > h=48b74865c63840b288bd85b4d8743533b73b339b > > > > commit 48b74865c63840b288bd85b4d8743533b73b339b > > Author: H.J. Lu <hjl.tools@gmail.com> > > Date: Tue Jan 3 13:06:48 2023 -0800 > > > > x86: Check minimum/maximum of non_temporal_threshold [BZ #29953] > > > > The minimum non_temporal_threshold is 0x4040. non_temporal_threshold may > > be set to less than the minimum value when the shared cache size isn't > > available (e.g., in an emulator) or by the tunable. Add checks for > > minimum and maximum of non_temporal_threshold. > > > > This fixes BZ #29953. > > H.J., do you intend to backport this? Can we close this bug? I will backport it. The release/2.36/master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=f3991fec8071dbcf3ec9f13a91c738b66fcd4159 commit f3991fec8071dbcf3ec9f13a91c738b66fcd4159 Author: H.J. Lu <hjl.tools@gmail.com> Date: Tue Jan 3 13:06:48 2023 -0800 x86: Check minimum/maximum of non_temporal_threshold [BZ #29953] The minimum non_temporal_threshold is 0x4040. non_temporal_threshold may be set to less than the minimum value when the shared cache size isn't available (e.g., in an emulator) or by the tunable. Add checks for minimum and maximum of non_temporal_threshold. This fixes BZ #29953. (cherry picked from commit 48b74865c63840b288bd85b4d8743533b73b339b) The release/2.34/master branch has been updated by Florian Weimer <fw@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=89c017de2f52d17862bda9a6f8382e913457bfbe commit 89c017de2f52d17862bda9a6f8382e913457bfbe Author: H.J. Lu <hjl.tools@gmail.com> Date: Tue Jan 3 13:06:48 2023 -0800 x86: Check minimum/maximum of non_temporal_threshold [BZ #29953] The minimum non_temporal_threshold is 0x4040. non_temporal_threshold may be set to less than the minimum value when the shared cache size isn't available (e.g., in an emulator) or by the tunable. Add checks for minimum and maximum of non_temporal_threshold. This fixes BZ #29953. (cherry picked from commit 48b74865c63840b288bd85b4d8743533b73b339b) The release/2.35/master branch has been updated by Florian Weimer <fw@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=b7cc55a24ea303875d95d0db3f73f9239bba2583 commit b7cc55a24ea303875d95d0db3f73f9239bba2583 Author: H.J. Lu <hjl.tools@gmail.com> Date: Tue Jan 3 13:06:48 2023 -0800 x86: Check minimum/maximum of non_temporal_threshold [BZ #29953] The minimum non_temporal_threshold is 0x4040. non_temporal_threshold may be set to less than the minimum value when the shared cache size isn't available (e.g., in an emulator) or by the tunable. Add checks for minimum and maximum of non_temporal_threshold. This fixes BZ #29953. (cherry picked from commit 48b74865c63840b288bd85b4d8743533b73b339b) Backports completed. |