272327 - AMD64 assembly multiply routine for freebl / mpi

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Julien Pierre]

The Solaris crypto group at Sun has decided to donate optimizations assembly
optimizations for the bignum library for the AMD64 instruction set.

This code roughly doubles the RSA performance . On a v20z with opteron model 248
CPU at 2.2 GHz, using gcc and -O3 optimizations on Solaris 10, rsaperf goes from
585 ops/s for the pure C implementation, to 1186 ops/s with this code . This is
for 1024 bit RSA keys, with rsaperf running single-threaded, ie. it is a per-CPU
ops number ...

Patch forthcoming later today.

Comment 1

[Julien Pierre]

Attachment: Use 64x64 multiply instruction

Comment 2

[Julien Pierre]

Comment on attachment 167713 [details] [diff] [review]
Use 64x64 multiply instruction

I realize we may want to do additional cleanup, but I'd like to get this
checked in to the tip so we can do performance runs from nightly tip builds .

Comment 3

[Julien Pierre]

Attachment: forgot the main files ...

Comment 4

[Julien Pierre]

Attachment: and another one ...

Comment 5

[Julien Pierre]

Guys, it's been 2 weeks since I asked for review on this. I'm sure you'll have
some review comments as the code isn't the cleanest, especially the C part. I
would like to see this checked in before the holiday break.

Thanks !

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 167718 [details] [diff] [review]
forgot the main files ...

3 issues with this patch:

a) please globally rename the new functions from big_mul_* to 
s_mpv_mul_* or mpi_mul_* or some other prefix that is already used
within MPI.

b) All of NSS has been converted to use the new "tri-license", which
allows use under MPL, GPL, or LGPL.  But the new files added here 
seem to be using the older dual license, which permits use only 
under MPL and GPL, but not LGPL.  I believe all of NSS needs to 
be liccensed under the same license.

c) Looks like you did a global search and replace in one file, 
replacing all slashes (/) with hashes (#).  So now there are lines
like the following, that need to be fixed.

>+# the License at http:##www.mozilla.org#MPL#

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 167719 [details]
and another one ...

>/*
> * Copyright 2004 Sun Microsystems, Inc.  All rights reserved.
> * Use is subject to license terms.
> */

mozilla cannot accept the file with this license. 
This file needs the MPL tri-license.  


>  d = big_mul_add_vec64(c, a, a_len, b);

Please change this name to use on MPI's existing prefixes.  
I'd suggest s_mpv_ for the following reasons:

a) s_ implies a private mpi-internal function, 
   not publicly callable

b) mpv_ prefix is used for functions tht operate directly 
   on vectors (arrays) of mp_digit.  mpi_ is used for 
   functions that operate on mp_int's.

Comment 8

[Julien Pierre]

*** Bug 256664 has been marked as a duplicate of this bug. ***

Comment 9

[Julien Pierre]

Attachment: updated patch

- update the license
- fix the types to use mpi types
- rename big_mul functions to s_mpv_
- fix comments in mpi_amd64_gas.s

Comment 10

[Julien Pierre]

Attachment: improved patch

- add -fPIC to ASFLAGS in Makefile for the gcc builds on Solaris and Linux
- add -DMPI_64 to the AMD64SOLARIS target in target.mk for the standalone mpi
build
- remove erroneous comments in assembly source

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 175509 [details] [diff] [review]
improved patch

r=nelson.  Looks ready.

Comment 12

[Julien Pierre]

Thanks, Nelson.

I checked in this patch to the tip.

Checking in Makefile;
/cvsroot/mozilla/security/nss/lib/freebl/Makefile,v  <--  Makefile
new revision: 1.60; previous revision: 1.59
done
Checking in mpi/mpi-priv.h;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/mpi-priv.h,v  <--  mpi-priv.h
new revision: 1.18; previous revision: 1.17
done
Checking in mpi/mpi-test.c;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/mpi-test.c,v  <--  mpi-test.c
new revision: 1.13; previous revision: 1.12
done
RCS file: /cvsroot/mozilla/security/nss/lib/freebl/mpi/mpi_amd64.c,v
done
Checking in mpi/mpi_amd64.c;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/mpi_amd64.c,v  <--  mpi_amd64.c
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/lib/freebl/mpi/mpi_amd64_gas.s,v
done
Checking in mpi/mpi_amd64_gas.s;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/mpi_amd64_gas.s,v  <--  mpi_amd64_gas.s
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/lib/freebl/mpi/mpi_amd64_sun.s,v
done
Checking in mpi/mpi_amd64_sun.s;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/mpi_amd64_sun.s,v  <--  mpi_amd64_sun.s
initial revision: 1.1
done
Checking in mpi/target.mk;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/target.mk,v  <--  target.mk
new revision: 1.3; previous revision: 1.2
done

Comment 13

[Wan-Teh Chang]

The target milestone 3.9.5 is wrong because
NSS 3.9.5 has been released.

Comment 14

[Julien Pierre]

Thanks for pointing that out, Wan-Teh. Changed to 3.10 .