Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions #4019

Closed
wants to merge 18 commits into from

Conversation

smita-kamath
Copy link

@smita-kamath smita-kamath commented May 14, 2021

I would like to submit AES-GCM optimization for x86_64 architectures supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES and GHASH operations.
Performance gain of ~1.5x - 2x for message sizes 8k and above.
/contributor add svkamath
/contributor add Tomasz Kantecki tomasz.kantecki@intel.com


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions

Reviewers

Contributors

  • Smita Kamath <svkamath@openjdk.org>
  • Tomasz Kantecki <tomasz.kantecki@intel.com>
  • Anthony Scarpino <ascarpino@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4019/head:pull/4019
$ git checkout pull/4019

Update a local copy of the PR:
$ git checkout pull/4019
$ git pull https://git.openjdk.java.net/jdk pull/4019/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4019

View PR using the GUI difftool:
$ git pr show -t 4019

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4019.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 14, 2021

👋 Welcome back svkamath! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot changed the title JDK-8267125: AES Galois CounterMode (GCM) interleaved implementation … 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions May 14, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 14, 2021

@smita-kamath this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout aes-gcm
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added the merge-conflict label May 14, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 14, 2021

@smita-kamath
Contributor Smita Kamath <svkamath@openjdk.org> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented May 14, 2021

@smita-kamath
Contributor Tomasz Kantecki <tomasz.kantecki@intel.com> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented May 14, 2021

@smita-kamath The following labels will be automatically applied to this pull request:

  • hotspot
  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added security hotspot labels May 14, 2021
@openjdk openjdk bot removed the merge-conflict label May 14, 2021
@smita-kamath smita-kamath marked this pull request as ready for review May 14, 2021
@openjdk openjdk bot added the rfr label May 14, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

@neliasso
Copy link

@neliasso neliasso commented May 28, 2021

Added hotspot-compiler label

@openjdk
Copy link

@openjdk openjdk bot commented May 28, 2021

@neliasso Unknown command add - for a list of valid commands use /help.

@neliasso
Copy link

@neliasso neliasso commented May 28, 2021

/label add hotspot-compiler

@openjdk openjdk bot added the hotspot-compiler label May 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 28, 2021

@neliasso
The hotspot-compiler label was successfully added.

@openjdk openjdk bot added the merge-conflict label May 28, 2021
@ascarpino
Copy link
Contributor

@ascarpino ascarpino commented Jun 9, 2021

With JDK-8255557 integrated, I'll provide you a merged copy of your java side changes.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Do you plan to implement decrypt intrinsic too?

@@ -543,6 +543,9 @@ bool LibraryCallKit::try_to_inline(int predicate) {
case vmIntrinsics::_counterMode_AESCrypt:
return inline_counterMode_AESCrypt(intrinsic_id());

case vmIntrinsics::_galoisCounterMode_AESCrypt:
return inline_galoisCounterMode_AESCrypt(intrinsic_id());
Copy link
Contributor

@vnkozlov vnkozlov Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to pass intrinsic_id() for this implementation unless you plan to add decrypt intrinsic later.

Copy link
Author

@smita-kamath smita-kamath Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comments Vladimir. The intrinsic is called for encrypt as well as decrypt operation.

Copy link
Contributor

@vnkozlov vnkozlov Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one intrinsic is declared in this change: _galoisCounterMode_AESCrypt. Other AES intrinsics have 2 that is why they have to pass intrinsic_id(). See lines before this.

Copy link
Contributor

@vnkozlov vnkozlov Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, _counterMode_AESCrypt is not example - it has the same issue.

top_ct != NULL && top_ct->klass() != NULL &&
top_out != NULL && top_out->klass() != NULL, "args are strange");

// checks are the responsibility of the caller
Copy link
Contributor

@vnkozlov vnkozlov Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have all NULL for all objects and range checks in Java code for this intrinsic?

Node* counter = load_field_from_object(gctr_object, "counter", "[B");
Node* subkeyHtbl = load_field_from_object(ghash_object, "subkeyHtbl", "[J");
Node* state = load_field_from_object(ghash_object, "state", "[J");
if (embeddedCipherObj == NULL || counter == NULL || subkeyHtbl == NULL || state == NULL) return false;
Copy link
Contributor

@vnkozlov vnkozlov Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow coding style for such long condition:

if () {
  return false;
}

Copy link
Author

@smita-kamath smita-kamath Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make the change. Thanks.

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 9, 2021

@smita-kamath This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@openjdk openjdk bot removed the merge-conflict label Jul 13, 2021
@openjdk openjdk bot removed the rfr label Jul 14, 2021
@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Jul 15, 2021

Looks like you have some issues: wrong file property.

StubRoutines::x86::_counter_mask_addr = counter_mask_addr();
StubRoutines::x86::_ghash_poly512_addr = ghash_polynomial512_addr();
StubRoutines::x86::_ghash_long_swap_mask_addr = generate_ghash_long_swap_mask();
Copy link
Contributor

@vnkozlov vnkozlov Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_counter_mask_addr = counter_mask_addr() and _ghash_long_swap_mask_addr = generate_ghash_long_swap_mask() are called by other intrinsics too. Which duplicates code in codeCache.
They should be called only once when they are used. May be counter_mask_addr() and generate_ghash_long_swap_mask() method should check that addresses already recorded and return it.

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Aug 18, 2021

For the record, I withdraw my request to put this on hold until Lazy stub generation is complete.
As discussed offline I agree with pushing this changes first.

Second, when final changes are ready before push, please, ask Oracle engineer to run tier1-3 in our testing infrastructure.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Good.

I will test it.

@smita-kamath
Copy link
Author

@smita-kamath smita-kamath commented Aug 23, 2021

/contributor add ascarpino

@openjdk
Copy link

@openjdk openjdk bot commented Aug 23, 2021

@smita-kamath Could not parse ascarpino anthony.scarpino@oracle.com as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

@ascarpino
Copy link
Contributor

@ascarpino ascarpino commented Aug 23, 2021

Tier 1-3 passed on linux-x64, windows-x64, macosx-x64, linux-aarch64

@smita-kamath
Copy link
Author

@smita-kamath smita-kamath commented Aug 23, 2021

/contributor add Anthony Scarpino ascarpino@openjdk.org

1 similar comment
@smita-kamath
Copy link
Author

@smita-kamath smita-kamath commented Aug 23, 2021

/contributor add Anthony Scarpino ascarpino@openjdk.org

@openjdk
Copy link

@openjdk openjdk bot commented Aug 23, 2021

@smita-kamath Could not parse Anthony Scarpino ascarpino@openjdk.org as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

@openjdk
Copy link

@openjdk openjdk bot commented Aug 23, 2021

@smita-kamath Could not parse Anthony Scarpino ascarpino@openjdk.org as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

@smita-kamath
Copy link
Author

@smita-kamath smita-kamath commented Aug 23, 2021

/contributor add ascarpino

@openjdk
Copy link

@openjdk openjdk bot commented Aug 23, 2021

@smita-kamath
Contributor Anthony Scarpino <ascarpino@openjdk.org> successfully added.

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Aug 24, 2021

Yes, my testing during weekend passed too.
Changes are good for integration.

@smita-kamath
Copy link
Author

@smita-kamath smita-kamath commented Aug 24, 2021

/integrate

@openjdk openjdk bot added the sponsor label Aug 24, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 24, 2021

@smita-kamath
Your change (at version c31bfe9) is now ready to be sponsored by a Committer.

@smita-kamath
Copy link
Author

@smita-kamath smita-kamath commented Aug 24, 2021

@ascarpino Looks like AES-GCM is ready to be integrated. Can you sponsor this patch? Thank you.

@ascarpino
Copy link
Contributor

@ascarpino ascarpino commented Aug 24, 2021

/sponsor

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Aug 24, 2021

Checking to see if the bot is awake (it should reject my attempt).

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Aug 24, 2021

Going to push as commit 0e7288f.
Since your change was applied there have been 39 commits pushed to the master branch:

  • 6ace805: 8272856: DoubleFlagWithIntegerValue uses G1GC-only flag
  • c547ead: 8272916: Copyright year was modified unintentionally in jlink.properties and ImagePluginStack.java
  • aaedac6: 8265261: java/nio/file/Files/InterruptCopy.java fails with java.lang.RuntimeException: Copy was not interrupted
  • d34f17c: 8270195: Add missing links between methods of JavaFX properties
  • f608e81: 8264322: Generate CDS archive when creating custom JDK image
  • f681d65: 8272806: [macOS] "Apple AWT Internal Exception" when input method is changed
  • 6e0328f: 8272725: G1: add documentation on needs_remset_update_t vs bool
  • 2309b7d: 8253178: Replace LinkedList Impl in net.http.FilterFactory
  • 94f5e44: 8271258: @param with non-ascii variable names produces incorrect results
  • 7454306: 8272526: Cleanup ThreadStateTransition class
  • ... and 29 more: https://git.openjdk.java.net/jdk/compare/f4be211ae290824cb6c678dcdff0df91a20117d6...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Aug 24, 2021
@openjdk openjdk bot added integrated and removed ready rfr sponsor labels Aug 24, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 24, 2021

@ascarpino @smita-kamath Pushed as commit 0e7288f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 24, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-compiler integrated security
7 participants