[Bug string/30429] New: Do not use x86 string functions with cache size dependencies if cache size missing

[Bug string/30429] New: Do not use x86 string functions with cache size dependencies if cache size missing

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30429

            Bug ID: 30429
           Summary: Do not use x86 string functions with cache size
                    dependencies if cache size missing
           Product: glibc
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: string
          Assignee: unassigned at sourceware dot org
          Reporter: fweimer at redhat dot com
  Target Milestone: ---

If we cannot get cache sizes from CPUID, we should not use string functions
that depend on cache sizes, in particular the non-temporal store threshold.

After commit 48b74865c63840b288bd85b4d8743533b73b339b("x86: Check
minimum/maximum of non_temporal_threshold [BZ #29953]") we clamp the the
minimum non-temporal store threshold at 0x4040, but this is at the extremely
low end likely bad for performance.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/30429] Do not use x86 string functions with cache size dependencies if cache size missing

[goldstein.w.n at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30429

Noah Goldstein <goldstein.w.n at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |goldstein.w.n at gmail dot com

--- Comment #1 from Noah Goldstein <goldstein.w.n at gmail dot com> ---
(In reply to Florian Weimer from comment #0)
> If we cannot get cache sizes from CPUID, we should not use string functions
> that depend on cache sizes, in particular the non-temporal store threshold.
> 
> After commit 48b74865c63840b288bd85b4d8743533b73b339b("x86: Check
> minimum/maximum of non_temporal_threshold [BZ #29953]") we clamp the the
> minimum non-temporal store threshold at 0x4040, but this is at the extremely
> low end likely bad for performance.

We probably would be better off using the maximum and skipping non-temporal
threshold all together.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/30429] Do not use x86 string functions with cache size dependencies if cache size missing

[hjl.tools at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30429

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hjl.tools at gmail dot com

--- Comment #2 from H.J. Lu <hjl.tools at gmail dot com> ---
(In reply to Noah Goldstein from comment #1)
> (In reply to Florian Weimer from comment #0)
> > If we cannot get cache sizes from CPUID, we should not use string functions
> > that depend on cache sizes, in particular the non-temporal store threshold.
> > 
> > After commit 48b74865c63840b288bd85b4d8743533b73b339b("x86: Check
> > minimum/maximum of non_temporal_threshold [BZ #29953]") we clamp the the
> > minimum non-temporal store threshold at 0x4040, but this is at the extremely
> > low end likely bad for performance.
> 
> We probably would be better off using the maximum and skipping non-temporal
> threshold all together.

It sounds reasonable.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/30429] Do not use x86 string functions with cache size dependencies if cache size missing

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30429

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fweimer at redhat dot com

--- Comment #3 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Noah Goldstein from comment #1)
> (In reply to Florian Weimer from comment #0)
> > If we cannot get cache sizes from CPUID, we should not use string functions
> > that depend on cache sizes, in particular the non-temporal store threshold.
> > 
> > After commit 48b74865c63840b288bd85b4d8743533b73b339b("x86: Check
> > minimum/maximum of non_temporal_threshold [BZ #29953]") we clamp the the
> > minimum non-temporal store threshold at 0x4040, but this is at the extremely
> > low end likely bad for performance.
> 
> We probably would be better off using the maximum and skipping non-temporal
> threshold all together.

True, that's probably better than switching implementations. Please feel to
retitle this bug as appropriate.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/30429] Do not use x86 string functions with cache size dependencies if cache size missing

[goldstein.w.n at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30429

--- Comment #4 from Noah Goldstein <goldstein.w.n at gmail dot com> ---
Posted patch to make default in this case 64MB
https://patchwork.sourceware.org/project/glibc/list/?series=19713

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/30429] Do not use x86 string functions with cache size dependencies if cache size missing

[goldstein.w.n at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30429

--- Comment #5 from Noah Goldstein <goldstein.w.n at gmail dot com> ---
Fix pushed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/30429] Assume large caches for string function tuning if x86 cache size information is missing

[fw at deneb dot enyo.de]

https://sourceware.org/bugzilla/show_bug.cgi?id=30429

Florian Weimer <fw at deneb dot enyo.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |security-
            Summary|Do not use x86 string       |Assume large caches for
                   |functions with cache size   |string function tuning if
                   |dependencies if cache size  |x86 cache size information
                   |missing                     |is missing
                 CC|                            |fw at deneb dot enyo.de
   Target Milestone|---                         |2.38

--- Comment #6 from Florian Weimer <fw at deneb dot enyo.de> ---
Are you going to backport this?

Fixed by:

commit ed2f9dc9420c4c61436328778a70459d0a35556a
Author: Noah Goldstein <goldstein.w.n@gmail.com>
Date:   Mon May 8 22:10:20 2023 -0500

    x86: Use 64MB as nt-store threshold if no cacheinfo [BZ #30429]

    If `non_temporal_threshold` is below `minimum_non_temporal_threshold`,
    it almost certainly means we failed to read the systems cache info.

    In this case, rather than defaulting the minimum correct value, we
    should default to a value that gets at least reasonable
    performance. 64MB is chosen conservatively to be at the very high
    end. This should never cause non-temporal stores when, if we had read
    cache info, we wouldn't have otherwise.
    Reviewed-by: Florian Weimer <fweimer@redhat.com>

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/30429] Assume large caches for string function tuning if x86 cache size information is missing

[goldstein.w.n at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30429

--- Comment #7 from Noah Goldstein <goldstein.w.n at gmail dot com> ---
(In reply to Florian Weimer from comment #6)
> Are you going to backport this?
> 

Didn't see this question earlier.

Sure I can backport. You think to 2.28?


> Fixed by:
> 
> commit ed2f9dc9420c4c61436328778a70459d0a35556a
> Author: Noah Goldstein <goldstein.w.n@gmail.com>
> Date:   Mon May 8 22:10:20 2023 -0500
> 
>     x86: Use 64MB as nt-store threshold if no cacheinfo [BZ #30429]
>     
>     If `non_temporal_threshold` is below `minimum_non_temporal_threshold`,
>     it almost certainly means we failed to read the systems cache info.
>     
>     In this case, rather than defaulting the minimum correct value, we
>     should default to a value that gets at least reasonable
>     performance. 64MB is chosen conservatively to be at the very high
>     end. This should never cause non-temporal stores when, if we had read
>     cache info, we wouldn't have otherwise.
>     Reviewed-by: Florian Weimer <fweimer@redhat.com>

-- 
You are receiving this mail because:
You are on the CC list for the bug.