Bug 136583 - LTC18371- [RHEL3 U4]cpu_sibling_map[] is incorrect on x445/x440
LTC18371- [RHEL3 U4]cpu_sibling_map[] is incorrect on x445/x440
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 3
Classification: Red Hat
Component: kernel (Show other bugs)
3.0
i686 Linux
medium Severity low
: ---
: ---
Assigned To: Ingo Molnar
Brian Brock
:
Depends On:
Blocks: 168424
  Show dependency treegraph
 
Reported: 2004-10-20 19:34 EDT by john stultz
Modified: 2007-11-30 17:07 EST (History)
10 users (show)

See Also:
Fixed In Version: RHSA-2006-0144
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-03-15 10:42:34 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Full dmesg (28.81 KB, text/plain)
2004-10-20 19:36 EDT, john stultz
no flags Details
8 bit ID masks and first try at hard_smp_processor_id() (1.17 KB, patch)
2004-10-20 23:01 EDT, James Cleverdon
no flags Details | Diff
Better hack to hard_smp_processor_id() and apicdef.h (1.17 KB, text/plain)
2004-10-21 00:07 EDT, James Cleverdon
no flags Details
Recent change to setup.c (1.92 KB, patch)
2004-10-27 21:18 EDT, john stultz
no flags Details | Diff

  None (edit)
Description john stultz 2004-10-20 19:34:43 EDT
From Bugzilla Helper: 
User-Agent: Mozilla/5.0 (compatible; Konqueror/3.3; Linux) (KHTML, 
like Gecko) 
 
Description of problem: 
Just noticed booting RHEL3 U4 beta1 on an x445 or x440 that the 
cpu_sibling_map looks broken. 
 
cpu_sibling_map[0] = 2 
cpu_sibling_map[1] = 3 
cpu_sibling_map[2] = 0 
cpu_sibling_map[3] = 1 
cpu_sibling_map[4] = 0 
cpu_sibling_map[5] = 1 
cpu_sibling_map[6] = 0 
cpu_sibling_map[7] = 1 
cpu_sibling_map[8] = 0 
cpu_sibling_map[9] = 1 
cpu_sibling_map[10] = 0 
cpu_sibling_map[11] = 1 
cpu_sibling_map[12] = 0 
cpu_sibling_map[13] = 1 
cpu_sibling_map[14] = 0 
cpu_sibling_map[15] = 1 
 
 
Version-Release number of selected component (if applicable): 
kernel-smp-2.4.21-21.EL 
 
How reproducible: 
Always 
 
Steps to Reproduce: 
1. Install RHEL3U4 beta1 on x445/x440 
2. Boot 
3. Look at /var/log/dmesg 
     
 
Actual Results:  cpu_sibling_map[] is incorrect. 
 
Expected Results:  cpu_sibling_map[] should be correct.  
I believe the proper pairing is (0,8)(1,9)(2,10)(3,11)(4,12)(5,13)
(6,14)(7,15) 
 
Additional info: 
I believe there were previous disputed attempts by James Cleverdon at 
fixing this, but I don't recall exactly.
Comment 1 john stultz 2004-10-20 19:36:17 EDT
Created attachment 105565 [details]
Full dmesg

Full dmseg from x445 bootup.
Comment 3 James Cleverdon 2004-10-20 22:47:26 EDT
Hmmm.... init_intel() in i386/kernel/setup.c already has the patch for
this bug.  However, the disassembly looks wrong:

c03f74fe: 0f b6 05 23 d0 ff ff    movzbl 0xffffd023,%eax
c03f7505: c7 04 24 90 7a 2b c0    movl   $0xc02b7a90,(%esp,1)
c03f750c: 83 e0 0e                and    $0xe,%eax
c03f750f: 89 04 b5 60 a2 45 c0    mov    %eax,0xc045a260(,%esi,4)

The optimizer is getting a bit frisky and changing
hard_smp_processor_id's unsigned long read into a movzbl.  Clever, but
totally wrong in this case.  The local APIC must always be read and
written with 32-bit accesses, otherwise the results are undefined.

And-ing %eax with 0xe instead of 0xfe isn't right either. Looks like
the patches for 8 bit IDs have been reverted somehow in
asm-i386/apicdef.h:

#define                 APIC_ID_MASK            (0x0F<<24)
#define                 GET_APIC_ID(x)          (((x)>>24)&0x0F)

The mask values should be 0xFF instead of 0x0F.
Comment 4 James Cleverdon 2004-10-20 23:01:55 EDT
Created attachment 105575 [details]
8 bit ID masks and first try at hard_smp_processor_id()

This patch works, but as the comment promises, using the volatile storage class
makes for really inefficient code.
Comment 5 James Cleverdon 2004-10-21 00:07:04 EDT
Created attachment 105576 [details]
Better hack to hard_smp_processor_id() and apicdef.h

This produces better code from gcc.
Comment 6 James Cleverdon 2004-10-22 19:41:35 EDT
Hmmm... The previous patch does work, but gcc loses some variable
value info when I insert the barrier().  It doesn't realize that
~(smp_num_siblings - 1) is a constant.  That doesn't matter in
init_intel() because it only is called at init time, but we use
hard_smp_processor_id a fair amount when running.

Should this better be classed as a gcc bug, or maybe the kernel is
being compiled with a few too many optimizations turned on?
Comment 7 Ernie Petrides 2004-10-22 20:00:47 EDT
James, I think the gcc behavior is correct (in both its reaction
to your use of barrier() and originally in the validity of its
optimization into a byte-wide access at +3).

I believe the correct implementation of hard_smp_processor_id() is:

  return GET_APIC_ID(*(volatile unsigned long *)(APIC_BASE+APIC_ID));

But I have not tried building/testing/disassembling this.

Cheers.  -ernie
Comment 8 john stultz 2004-10-22 20:05:20 EDT
It should be noted that I did not see this problem w/ 2.4.21-20. Is 
there a list of what changed since then? 
Comment 9 James Cleverdon 2004-10-22 20:23:12 EDT
Ernie,

As the comment in hard_smp_processor_id() notes, if you declare the
access as volatile, the code quality suffers.

Namely, it does a long read into eax, puts the value into a temp local
on the stack, loads it back into eax, maybe copies it into another
scratch register, then starts to use it.  I have no idea why gcc does
all the extra steps.  8^)

In principle, I agree with you, but gcc has to cooperate.

Comment 10 Ernie Petrides 2004-10-22 21:10:19 EDT
John, there have been 158 BZs/FZs resolved in 149 patches during
U4 development (since 2.4.21-20.EL).  The complete list of bugs
fixed is in the errata form for RHSA-2004:550, although you're
probably not allowed access to that until the final U4 is pushed
on RHN.  If you want to know the U3->U4 diff of a specific source
file, I can generate one and mail it to you.

James, I'd hope that all the extra stack references wouldn't be
generated simply from adding the "volatile" into the cast as I've
done in comment #7.  But if they are, then maybe reducing the high-
frequency calls to hard_smp_processor_id() is a possibility.

Anyway, this is Ingo's bug, so I should stop interfering!  :-)
Comment 11 john stultz 2004-10-27 21:18:47 EDT
Created attachment 105877 [details]
Recent change to setup.c

Here is the recent change to setup.c (not present in 2.4.21-20.EL) which when
reverted seems to fix this issue. It comes from part of
linux-2.4.21-o1-ht.patch 

I know there was some dispute over this code awhile back, I'm not sure that
reverting it is the correct thing to do, but I believe this isolates the
relevant new bits.
Comment 12 James Cleverdon 2004-10-27 21:39:17 EDT
Yeah, Jun Nakajima and I had some discussions on this in the past.  He
asserted that the code to deal with non-power-of-2 siblings was vital,
and always preferred the cpuinfo APIC ID to the one in the local
APIC's ID register.

The non-power-of-2 thing I could understand, although I don't recall
any such CPU on Intel's roadmap (Tusla will have 4 "agents" per package).

I could never figure out his objection to the local APIC ID register.
 It seemed to have something to do with deranged BIOSes wildly
renumbering siblings to very different numbers, thus losing the
sibling association.  This never seemed to happen in real life.

But, don't take my misconceptions and misunderstandings too seriously.
 His replies are in the archives.

Executive summary:  as far as I'm concerned, the patch is a very good
thing, vital for systems that must renumber APICs on boot (i.e. summit
and other NUMA boxen).  Too bad hard_smp_processor_id() is weirding
out.  One of the patches proposed above should fix it.
Comment 13 john stultz 2004-11-29 15:04:52 EST
Any update on this? Is RedHat taking James' patch? 
Comment 14 Chris McDermott 2004-12-01 14:53:45 EST
Red Hat, could you please let us know whether this patch was picked up
for RHEL3 U4?  
Comment 15 Ernie Petrides 2004-12-03 17:00:38 EST
This did not make U4.  Ingo, could you please follow up on this for U5?
Comment 16 john stultz 2004-12-21 14:11:20 EST
Yes, please could someone look at this? James' fix has been sitting 
here for two months. 
Comment 17 john stultz 2005-03-31 17:45:26 EST
This issue is still present in the 2.4.21-31.ELsmp kernel.
Comment 18 Guy Streeter 2005-05-12 11:38:32 EDT
Changing "Product" from Beta to U4
Comment 19 keith mannth 2005-09-14 15:03:37 EDT
Linux version 2.4.21-37.ELsmp (bhcompile@tweety.build.redhat.com) (gcc version
3.2.3 20030502 (Red Hat Linux 3.2.3-53)) #1 SMP Wed Sep 7 13:28:55 EDT 2005

Still shows this problem.  
Comment 20 Ingo Molnar 2005-09-15 14:36:25 EDT
posted the apicdef.h / smp.h patch for the next taroon update.
Comment 21 Ernie Petrides 2005-09-21 20:51:49 EDT
A fix for this problem has just been committed to the RHEL3 U7
patch pool this evening (in kernel version 2.4.21-37.3.EL).
Comment 26 Red Hat Bugzilla 2006-03-15 10:42:34 EST
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on the solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHSA-2006-0144.html

Note You need to log in before you can comment on or make changes to this bug.