Closed Bug 272327 Opened 20 years ago Closed 19 years ago

AMD64 assembly multiply routine for freebl / mpi

Categories

(NSS :: Libraries, enhancement, P1)

3.9.3
Sun
SunOS
enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julien.pierre, Assigned: julien.pierre)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch Use 64x64 multiply instruction (obsolete) — Splinter Review
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 .
Attachment #167713 - Flags: review?(nelson)
Attached patch forgot the main files ... (obsolete) — Splinter Review
Attachment #167713 - Attachment is obsolete: true
Attachment #167713 - Flags: review?(nelson)
Attached file and another one ... (obsolete) —
Attachment #167718 - Flags: superreview?(rrelyea0264)
Attachment #167718 - Flags: review?(nelson)
Attachment #167719 - Flags: superreview?
Attachment #167719 - Flags: review?(nelson)
Severity: normal → enhancement
Priority: -- → P1
Target Milestone: --- → 3.9.5
Attachment #167719 - Flags: superreview?(nelson)
Attachment #167719 - Flags: superreview?
Attachment #167719 - Flags: review?(saul.edwards.bugs)
Attachment #167719 - Flags: review?(nelson)
Attachment #167718 - Flags: superreview?(rrelyea)
Attachment #167718 - Flags: superreview?(nelson)
Attachment #167718 - Flags: review?(saul.edwards.bugs)
Attachment #167718 - Flags: review?(nelson)
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 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#
Attachment #167718 - Flags: superreview?(nelson) → superreview-
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.
Attachment #167719 - Flags: superreview?(nelson) → superreview-
Attachment #167718 - Flags: review?(saul.edwards.bugs)
Attachment #167719 - Flags: review?(saul.edwards.bugs)
*** Bug 256664 has been marked as a duplicate of this bug. ***
Attached patch updated patch (obsolete) — Splinter Review
- update the license
- fix the types to use mpi types
- rename big_mul functions to s_mpv_
- fix comments in mpi_amd64_gas.s
Attachment #167718 - Attachment is obsolete: true
Attachment #167719 - Attachment is obsolete: true
Attachment #175487 - Flags: review?(nelson)
Attached patch improved patchSplinter Review
- 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
Attachment #175509 - Flags: review?(nelson)
Attachment #175487 - Attachment is obsolete: true
Attachment #175487 - Flags: review?(nelson)
Comment on attachment 175509 [details] [diff] [review]
improved patch

r=nelson.  Looks ready.
Attachment #175509 - Flags: review?(nelson) → review+
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
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
The target milestone 3.9.5 is wrong because
NSS 3.9.5 has been released.
Thanks for pointing that out, Wan-Teh. Changed to 3.10 .
Target Milestone: 3.9.5 → 3.10
You need to log in before you can comment on or make changes to this bug.