302658 - sparc MPI code making run-time ISA determination

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Mikhail T.]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.4; FreeBSD) KHTML/3.4.1 (like Gecko)
Build Identifier: 

I built NSS on Solaris/sparc64 as follows:   
% env USE_64=1 gmake nss_build_all  
  
Unfortunately, that did not set the MP_USE_LONG_DIGIT somewhere, because  
isSparcV8PlusVis in freebl/mpi/mpi_sparc.c is still looking for string  
"sparcv8plus+vis" instead of "sparcv9+vis":  
  
            isSparcV8PlusVis  
[nss-3.10/mozilla/security/nss/lib/freebl/mpi/mpi_sparc.c:286]  
               #if defined(MP_USE_LONG_DIGIT)  
                       char * found = strstr(buf, "sparcv9+vis");  
               #else  
            =>         char * found = strstr(buf, "sparcv8plus+vis");  
               #endif  
                       rv = (found != 0);  
                   }  
            initPtrs        
[nss-3.10/mozilla/security/nss/lib/freebl/mpi/mpi_sparc.c:308]  
            sp_mpv_mul_d_add_prop  
[nss-3.10/mozilla/security/nss/lib/freebl/mpi/mpi_sparc.c:336]  
  
Also, this -- the call to strstr() -- happens at least 29 times for a command  
like:  
  
      certutil -s \  
          CN=TestUser47, E=[e-mail], O=BOGUS NSS, L=Mountain View,  
ST=California, C=US \  
          -R -d \  
          /net/mi/misha/ports/security/nss-new/work/nss-3.10/mozilla/tests_results/security/beaker.12/client  
\  
          -f ../tests.pw.9218 -z ../tests_noise.9218 -o req   
  
strstr() is not a cheap function to call. Should not isSparcV8PlusVis and other  
similar functions cache their results some place? Calling strstr() is sure to  
negate all speed-ups gained by using the Sparc-specific code... 

Reproducible: Always

Comment 1

[Wan-Teh Chang]

Nelson, could you take a look at this?

I believe the decision to define MP_USE_UINT_DIGIT instead
of MP_USE_LONG_DIGIT was done after performance measurement.

I agree the calls to strstr() could be avoided.  Also the
strstr() calls follow the system call sysinfo(), which is
expensive.  We should figure out a way to call initPtrs()
only once.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

The use of 32-bit integers in MPI for v8+ and v9 ABIs is intentional.
Is is required by the code that uses the FPU to do modular exponentiation.
The performance is greatly enhanced by that.

We build separate shared libs for v8 and v8+ ISAs.  Those shared libs should 
contain no code that looks at the ISA list.  The only code that should be
examining the isalist now should be in loader.c, which decides whether to 
load the v8 or v8+ shared lib.  Any code that remains in freebl (outside of 
loader.c) that examines the isalist should be dead code.  

The code that checks the ISA list should be absent in v9 builds.  
It should only be present in the v8/v8+ builds.  
The fact that the reporter is seeing it suggests to me that the reporter
is actually building v8/v8+ builds.

The code in freebl is highly ifdef'ed, in c source and in makefiles.  
We only support builds that use the sets of symbols that we have defined. 
Is the code under test built with exactly the same symbols defined as we
use in our stock NSS builds?   

Comment 3

[Mikhail T.]

=The fact that the reporter is seeing it suggests to me    
=that the reporter is actually building v8/v8+ builds.    
    
Out of the question for three reasons: 
  
1) I built using ``env USE_64=1 gmake nss_build_all'' 
2) All of the Purify results I posted today show 8-byte pointers. 
3) file 
work/nss-3.10/mozilla/security/nss/cmd/certutil/SunOS5.9_purify_64_DBG.OBJ/certutil  
work/nss-3.10/mozilla/security/nss/cmd/certutil/SunOS5.9_64_DBG.OBJ/certutil  
work/nss-3.10/mozilla/security/nss/cmd/certutil/SunOS5.9_purify_64_DBG.OBJ/certutil:  
ELF 64-bit MSB executable, SPARC V9, version 1 (SYSV), dynamically linked (uses  
shared libs), not stripped  
work/nss-3.10/mozilla/security/nss/cmd/certutil/SunOS5.9_64_DBG.OBJ/certutil:         
ELF 64-bit MSB executable, SPARC V9, version 1 (SYSV), dynamically linked (uses  
shared libs), not stripped  
 

Comment 4

[Julien Pierre]

Nelson,

I got the following stack in a sparcv9 binary of certutil .

=>[1] isSparcV8PlusVis(), line 286 in "mpi_sparc.c"
  [2] initPtrs(), line 308 in "mpi_sparc.c"
  [3] sp_mpv_mul_d_add_prop(a = 0x100193a10, a_len = 16U, b = 2739301503U, c =
0x100193b20), line 336 in "mpi_sparc.c"
  [4] s_mpv_mul_d_add_prop(a = 0x100193a10, a_len = 16U, b = 2739301503U, c =
0x100193b20), line 358 in "mpi_sparc.c"
  [5] s_mp_redc(T = 0xffffffff7fff5770, mmm = 0xffffffff7fff58f8), line 82 in
"mpmontg.c"
  [6] mp_exptmod_f(montBase = 0xffffffff7fff5930, exponent = 0xffffffff7fffdb70,
modulus = 0xffffffff7fffdb70, result = 0xffffffff7fff5a50, mmm =
0xffffffff7fff58f8, nLen = 16, bits_in_exponent = 516U, window_bits = 6U,
odd_ints = 32U), line 328 in "mpmontg.c"
  [7] mp_exptmod(inBase = 0xffffffff7fff5a68, exponent = 0xffffffff7fffdb70,
modulus = 0xffffffff7fffdb70, result = 0xffffffff7fff5a50), line 570 in "mpmontg.c"
  [8] mpp_fermat(a = 0xffffffff7fffdb70, w = 2U), line 249 in "mpprime.c"
  [9] mpp_make_prime(start = 0xffffffff7fffdda8, nBits = 512U, strong = 0,
nTries = 0xffffffff7fffdc80), line 510 in "mpprime.c"
  [10] generate_prime(prime = 0xffffffff7fffdda8, primeLen = 64), line 202 in
"rsa.c"
  [11] RSA_NewKey(keySizeInBits = 1024, publicExponent = 0xffffffff7fffdf38),
line 275 in "rsa.c"
  [12] NSC_GenerateKeyPair(hSession = 16777218U, pMechanism =
0xffffffff7fffe1e0, pPublicKeyTemplate = 0xffffffff7fffe3c8,
ulPublicKeyAttributeCount = 8U, pPrivateKeyTemplate = 0xffffffff7fffe488,
ulPrivateKeyAttributeCount = 7U, phPublicKey = 0xffffffff7fffe1a8, phPrivateKey
= 0xffffffff7fffe1b0), line 3077 in "pkcs11c.c"
  [13] PK11_GenerateKeyPair(slot = 0x100182af0, type = 0, param =
0xffffffff7fffe660, pubKey = 0xffffffff7ffff0d8, token = 1, sensitive = 1, wincx
= 0xffffffff7ffff0f0), line 1221 in "pk11akey.c"
  [14] CERTUTIL_GeneratePrivateKey(keytype = rsaKey, slot = 0x100182af0, size =
1024, publicExponent = 65537, noise = 0xffffffff7ffff717 "noise", pubkeyp =
0xffffffff7ffff0d8, pqgFile = (nil), pwdata = 0xffffffff7ffff0f0), line 572 in
"keystuff.c"
  [15] certutil_main(argc = 21, argv = 0xffffffff7ffff318, initialize = 1), line
2854 in "certutil.c"
  [16] main(argc = 21, argv = 0xffffffff7ffff318), line 3104 in "certutil.c

The reporter is correct that we are testing for the wrong isalist string for the
v9 code. However, that is probably not an issue, because any CPU that supports
sparcv8+vis and sparcv9 (we know this by virtue of the binary being 64-bit) is
going to also support sparcv9+vis. We should replace the #ifdef
MP_USE_LONG_DIGIT with a better one, perhaps #ifdef __sparcv9 , which is set by
the compiler.

A more serious problem is the logic in initPtrs. Its role is to set some
function pointers to either the VIS code, or some "v8" code - which in this case
is really v9 code . (I wasn't even aware we had non-VIS code in 64-bit).

initPtrs is called the first time that either 
sp_mpv_mul_d, sp_mpv_mul_d_add, and sp_mpv_mul_d_add_prop are invoked for the
first time. After that, these functions are never invoked again, since the
global function pointers p_mpv_mul_d, p_mpv_mul_d_add, and p_mpv_mul_d_add_prop
have been reset to point to other functions.
The problem is that this pointer initialization may not be not thread-safe, if
the 64-bit pointer setting isn't an atomic operation - which it is not
guaranteed to be with the C assignments in initPtrs() and the absence of locks.

The fix should be to invoke initPtrs only during the initialization of NSS, not
at runtime from crypto functions.

Because initPtrs and isSparcV8PlusVis are only invoked once, strstr() is also
invoked only once. I can't see from the current code how it could be called 29
times in certutil - maybe a few times in a multithreaded program if there is a
race condition in initPtrs, but this isn't the case in certutil.

Comment 5

[Julien Pierre]

Currently, the 64-bit libsoftokn3.so is built with object files containing VIS
code, and thus the shared library is marked as requiring a machine with
sparcv9+vis . Solaris will not load that library unless the machine supports VIS
instructions, either natively or through emulation .

Thus, the isalist test in the 64-bit sparc build is completely superfluous. The
test will never fail - VIS will always be present on the machine, if the code
gets to execute. The alternate implementation is dead code in the 64-bit
libsoftokn3.so binary. It is code using 32-bit integer arithmetic, ie. the
equivalent of the "pure32" code, compiled into a 64-bit binary. This
implementation should never be used on any 64-bit sparc machine. If one needs to
do integer arithmetic on 64-bit sparc - as we do for certain new sparc chips -
then we want to use 64-bit integer arithmetic. This will result into splitting
up the crypto code into two additional freebl shared libraries - one with the
floating point & VIS code, and another with 64-bit integer code.

Comment 6

[Julien Pierre]

Marking P2 for 3.11, and updating subject .

Comment 7

[Mikhail T.]

(In reply to comment #4) 
 
> Because initPtrs and isSparcV8PlusVis are only invoked once, strstr() is also 
> invoked only once. I can't see from the current code how it could be called 
29 
> times in certutil - maybe a few times in a multithreaded program if there is 
a 
> race condition in initPtrs, but this isn't the case in certutil. 
 
Actually, it is 35 times according to Purify. 1 time with one stack, 29 times 
with another, stack and 5 more times with another. 
 
See the attachments https://bugzilla.mozilla.org/attachment.cgi?id=190969 
(unexpanded UMRs) and the https://bugzilla.mozilla.org/attachment.cgi?id=190970 
(search this Purify's log for "isSparcV8PlusVis"). 
 

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

The UMRs reported in the attachments to bug 302663 do not indicate that 
there were 35 calls to initptrs, but that there were 35 UMR's inside strstr,
perhaps all from a single call to initptrs.

This could should all be conditionally compiled, and should not be used in
a v9 build.  we'll fix that for 3.11.  Thanks for pointing this out.

Comment 9

[Julien Pierre]

Mikhail,

I looked at your Purify data. There were three stack containing
isSparcV8PlusVis, and they were apparently all the same.

Is it possible that Purify is reporting 35 errors on the same instance of a
single call to strstr within isSparcV8PlusVis ? I think Purify might be
reporting an error for every byte of UMR, which I certainly wouldn't consider
distinct problems.

What I'm trying to say is that isSparcV8PlusVis is only ever getting called
once, so any performance problem that exists within it is a non-issue.
Are you certain that it is getting called more than once ? Try a breakpoint with
dbx, or adding printf statements .

Comment 10

[Wan-Teh Chang]

I verified by both code inspection and a test that
isSparcV8PlusVis is only called once (in a single
threaded application like certutil).

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

The issue raised by this bug's discovery is broader than just v9 builds.
The only place where NSS should be choosing code based on the ISA is in 
loader.c.  Once that determination is made, and the proper version of the 
freebl DSO is loaded, there should be no code in freebl that asks AGAIN 
which ISA to use.  

Note that the number of different freebl DSOs is expected to increase
(for sparc) in 3.11, due to new CPUs' capabilities.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 (checked in on trunk)

Looks like the solution might be as simple as this simple patch.
Still being tested.

Comment 13

[Julien Pierre]

Nelson, have you completed the testing of your patch ? I think it is worthwile fixing this for 3.11 given that we have new freebl shared libraries.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 192138 [details] [diff] [review]
patch v1 (checked in on trunk)

I am stunned that this fix didn't go into NSS 3.11.  
Julien, please review.

Comment 15

[Julien Pierre]

Comment on attachment 192138 [details] [diff] [review]
patch v1 (checked in on trunk)

The patch looks fine because this source file is only built in the freebl shared libs that run on Sparc V8+vis and V9+vis . It would be useful to note that fact in the comment at the top of this patch, which was  left unchanged.

This bug was P2, there was no review request, and earlier updates to the bug went unanswered, so I'm not surprised the fix fell through the cracks.

Comment 16

[Wan-Teh Chang]

Comment on attachment 192138 [details] [diff] [review]
patch v1 (checked in on trunk)

lib/freebl/Makefile shows that this file (mpi_sparc.c)
is only used in the following two freebl shared
libraries for Solaris SPARC: USE_ABI32_FPU and USE_ABI64_FPU.

In the makefile, these two shared libraries are described
with these comments, which do not mention "vis" at all:

  ifdef USE_ABI32_FPU
      # this builds for Sparc v8+a ABI32_FPU architecture, 64-bit registers, 
      # 32-bit ABI, it uses FPU code, and 32-bit word size
      MPI_SRCS += mpi_sparc.c
      ...
  endif
  ...
  ifdef USE_ABI64_FPU
      # this builds for Sparc v9a pure 64-bit architecture
      # It uses floating point, and 32-bit word size

Does the "a" in "v8+a" and "v9a" mean "vis"?

I agree the comment at the beginning of this patch can now
simply say:

  /* these functions run only on v8+vis or v9+vis CPUs. */

because this file no longer has other versions of these
functions.

Comment 17

[Wan-Teh Chang]

Comment on attachment 192138 [details] [diff] [review]
patch v1 (checked in on trunk)

One more minor change to the comment at the beginning of
this patch:

  /* these functions run only on v8plus+vis or v9+vis CPUs. */

Note the change of "v8+vis" to "v8plus+vis".

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 192138 [details] [diff] [review]
patch v1 (checked in on trunk)

Patch checked in on trunk with Wan-Teh's suggested comment changes.
Checking in mpi/mpi_sparc.c; new revision: 1.7; previous revision: 1.6

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Checked in on 3.11 branch
Checking in mpi_sparc.c; new revision: 1.6.30.1; previous revision: 1.6