| Message ID | 20230811172911.3705552-1-goldstein.w.n@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] x86: Fix incorrect scope of setting `shared_per_thread` [BZ# 30745] | expand |
On Fri, Aug 11, 2023 at 10:29 AM Noah Goldstein <goldstein.w.n@gmail.com> wrote: > > The: > > ``` > if (shared_per_thread > 0 && threads > 0) > shared_per_thread /= threads; > ``` > > Code was accidentally moved to inside the else scope. This doesn't > match how it was previously (before af992e7abd). > > This patch fixes that by putting the division after the `else` block. > --- > sysdeps/x86/dl-cacheinfo.h | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/sysdeps/x86/dl-cacheinfo.h b/sysdeps/x86/dl-cacheinfo.h > index 285773039f..5ddb35c9d9 100644 > --- a/sysdeps/x86/dl-cacheinfo.h > +++ b/sysdeps/x86/dl-cacheinfo.h > @@ -770,11 +770,10 @@ get_common_cache_info (long int *shared_ptr, long int * shared_per_thread_ptr, u > level. */ > threads = ((cpu_features->features[CPUID_INDEX_1].cpuid.ebx >> 16) > & 0xff); > - > - /* Get per-thread size of highest level cache. */ > - if (shared_per_thread > 0 && threads > 0) > - shared_per_thread /= threads; > } > + /* Get per-thread size of highest level cache. */ > + if (shared_per_thread > 0 && threads > 0) > + shared_per_thread /= threads; > } > > /* Account for non-inclusive L2 and L3 caches. */ > -- > 2.34.1 > LGTM. Thanks.
On 8/11/23 13:31, H.J. Lu via Libc-alpha wrote: > On Fri, Aug 11, 2023 at 10:29 AM Noah Goldstein <goldstein.w.n@gmail.com> wrote: >> >> The: >> >> ``` >> if (shared_per_thread > 0 && threads > 0) >> shared_per_thread /= threads; >> ``` >> >> Code was accidentally moved to inside the else scope. This doesn't >> match how it was previously (before af992e7abd). >> >> This patch fixes that by putting the division after the `else` block. >> --- >> sysdeps/x86/dl-cacheinfo.h | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/sysdeps/x86/dl-cacheinfo.h b/sysdeps/x86/dl-cacheinfo.h >> index 285773039f..5ddb35c9d9 100644 >> --- a/sysdeps/x86/dl-cacheinfo.h >> +++ b/sysdeps/x86/dl-cacheinfo.h >> @@ -770,11 +770,10 @@ get_common_cache_info (long int *shared_ptr, long int * shared_per_thread_ptr, u >> level. */ >> threads = ((cpu_features->features[CPUID_INDEX_1].cpuid.ebx >> 16) >> & 0xff); >> - >> - /* Get per-thread size of highest level cache. */ >> - if (shared_per_thread > 0 && threads > 0) >> - shared_per_thread /= threads; >> } >> + /* Get per-thread size of highest level cache. */ >> + if (shared_per_thread > 0 && threads > 0) >> + shared_per_thread /= threads; >> } >> >> /* Account for non-inclusive L2 and L3 caches. */ >> -- >> 2.34.1 >> > > LGTM. > > Thanks. This impacts CentOS 8 Stream and CentOS 9 Stream, which have backports of this sequence of changes. I'm testing the changes right now.
On 8/11/23 13:29, Noah Goldstein via Libc-alpha wrote: > The: > > ``` > if (shared_per_thread > 0 && threads > 0) > shared_per_thread /= threads; > ``` > > Code was accidentally moved to inside the else scope. This doesn't > match how it was previously (before af992e7abd). > > This patch fixes that by putting the division after the `else` block. LGTM. Tested on i686 and x86_64. I also backported this to c8s and c9s and re-tested and everything passes there without regression on i686, x86_64. I'm going to do some additional testing, but that's enough for me to see this get pushed. Thank you! Tested-by: Carlos O'Donell <carlos@redhat.com> Reviewed-by: Carlos O'Donell <carlos@redhat.com> > --- > sysdeps/x86/dl-cacheinfo.h | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/sysdeps/x86/dl-cacheinfo.h b/sysdeps/x86/dl-cacheinfo.h > index 285773039f..5ddb35c9d9 100644 > --- a/sysdeps/x86/dl-cacheinfo.h > +++ b/sysdeps/x86/dl-cacheinfo.h > @@ -770,11 +770,10 @@ get_common_cache_info (long int *shared_ptr, long int * shared_per_thread_ptr, u > level. */ > threads = ((cpu_features->features[CPUID_INDEX_1].cpuid.ebx >> 16) > & 0xff); > - > - /* Get per-thread size of highest level cache. */ > - if (shared_per_thread > 0 && threads > 0) > - shared_per_thread /= threads; > } > + /* Get per-thread size of highest level cache. */ > + if (shared_per_thread > 0 && threads > 0) > + shared_per_thread /= threads; > } > > /* Account for non-inclusive L2 and L3 caches. */
On Fri, Aug 11, 2023 at 2:38 PM Carlos O'Donell <carlos@redhat.com> wrote: > > On 8/11/23 13:29, Noah Goldstein via Libc-alpha wrote: > > The: > > > > ``` > > if (shared_per_thread > 0 && threads > 0) > > shared_per_thread /= threads; > > ``` > > > > Code was accidentally moved to inside the else scope. This doesn't > > match how it was previously (before af992e7abd). > > > > This patch fixes that by putting the division after the `else` block. > > LGTM. Tested on i686 and x86_64. > > I also backported this to c8s and c9s and re-tested and everything > passes there without regression on i686, x86_64. > > I'm going to do some additional testing, but that's enough for me > to see this get pushed. Thank you! > > Tested-by: Carlos O'Donell <carlos@redhat.com> > Reviewed-by: Carlos O'Donell <carlos@redhat.com> > > > --- > > sysdeps/x86/dl-cacheinfo.h | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/sysdeps/x86/dl-cacheinfo.h b/sysdeps/x86/dl-cacheinfo.h > > index 285773039f..5ddb35c9d9 100644 > > --- a/sysdeps/x86/dl-cacheinfo.h > > +++ b/sysdeps/x86/dl-cacheinfo.h > > @@ -770,11 +770,10 @@ get_common_cache_info (long int *shared_ptr, long int * shared_per_thread_ptr, u > > level. */ > > threads = ((cpu_features->features[CPUID_INDEX_1].cpuid.ebx >> 16) > > & 0xff); > > - > > - /* Get per-thread size of highest level cache. */ > > - if (shared_per_thread > 0 && threads > 0) > > - shared_per_thread /= threads; > > } > > + /* Get per-thread size of highest level cache. */ > > + if (shared_per_thread > 0 && threads > 0) > > + shared_per_thread /= threads; > > } > > > > /* Account for non-inclusive L2 and L3 caches. */ > > -- > Cheers, > Carlos. > Any objections to me backporting this to 2.38 release branch?
diff --git a/sysdeps/x86/dl-cacheinfo.h b/sysdeps/x86/dl-cacheinfo.h index 285773039f..5ddb35c9d9 100644 --- a/sysdeps/x86/dl-cacheinfo.h +++ b/sysdeps/x86/dl-cacheinfo.h @@ -770,11 +770,10 @@ get_common_cache_info (long int *shared_ptr, long int * shared_per_thread_ptr, u level. */ threads = ((cpu_features->features[CPUID_INDEX_1].cpuid.ebx >> 16) & 0xff); - - /* Get per-thread size of highest level cache. */ - if (shared_per_thread > 0 && threads > 0) - shared_per_thread /= threads; } + /* Get per-thread size of highest level cache. */ + if (shared_per_thread > 0 && threads > 0) + shared_per_thread /= threads; } /* Account for non-inclusive L2 and L3 caches. */
The: ``` if (shared_per_thread > 0 && threads > 0) shared_per_thread /= threads; ``` Code was accidentally moved to inside the else scope. This doesn't match how it was previously (before af992e7abd). This patch fixes that by putting the division after the `else` block. --- sysdeps/x86/dl-cacheinfo.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)