Support argon2 on GPUs through OpenCL#5382
Conversation
|
Thank you @alainesp! Since the OpenCL code (not just ideas, right?) is based on https://gitlab.com/omos/argon2-gpu I think we need to add their copyright statement + license to the kernel file. And also your copyright statement for your edits of their code. In fact, maybe add a preceding commit of their original code as-is, and then your commit with your changes to the OpenCL code + addition of your host code? So that we have revision history of their code vs. first our revision of it in here. I'm sorry this idea didn't occur to me in our e-mail discussion. |
|
CI failure is due to Intel driver update to 2024.0. Please let me know if you guys want to roll back and use a VM with Ubuntu 16 and Intel 18 driver or if we should disable the problematic formats for this test. |
|
The failure of Anyway, this is also our reminder that we either need to get |
How to do that? I think we can't add commits on the past. Do I close this PR and Create a new one with a new fork of JtR code? |
I think let's roll back. Will you be sending a separate PR for that, which we'll need to rebase on? Thank you! |
We can, e.g. with
We normally just rebase/amend commits and force-push, without closing/reopening PRs. However, if your use of git isn't from the command-line and you're uncomfortable switching to the command-line (although I highly recommend it), you may do the new PR thing. |
An easy way to do this:
You can also use |
|
Just for the record, here are the speeds at GTX 1080: (was 1416 c/s with turbo off on host CPUs, like I normally keep them lately to save power) Even the speed on Titan Kepler isn't bad: CPU format, 2x E5-2670, turbo on, slight unrelated load on ~1 core: So there's a 5x speedup at these Argon2 settings (which are more reasonable than the 4 MiB ones we historically use in |
ed8009f to
4b63b45
Compare
I did that. I think now the PR is in order? |
| //------------------------------------------------------------------------------------- | ||
| // | ||
| // Based on OpenCL code from https://gitlab.com/omos/argon2-gpu. | ||
| // Copyright (c) 2016 Ondrej Mosnáček |
There was a problem hiding this comment.
If what we have here is a derived work to an extent that @WOnder93 is a copyright holder, then we have to use his MIT license for this source file. That license says: "this permission notice shall be included in all copies or substantial portions of the Software." It says "this permission notice" (emphasis mine), so we should not replace MIT's permission notice with ours.
Alternatively, we need @WOnder93's permission to use our 0-clause BSD for the derived work.
There was a problem hiding this comment.
Happy to see my 7 years old code being useful :)
I'd prefer to keep the licensing as is. Does having an MIT-licensed code in john pose a problem for you? If yes, I can consider dual-licensing it also under 0BSD.
There was a problem hiding this comment.
@WOnder93 This isn't really a problem, but if you simply reply here saying we can relicense the code under 0BSD, that would make things more consistent for us. There's no need for you to make changes in your repo. Thank you!
I think we'll be switching this code to use our shared header files e.g. providing device type detection and bit rotate macros, which are under 0BSD.
What you did is as desired, thank you! However, now we (1) expect CI failure when it starts testing this new format (and we'll need to deal with that, e.g. by excluding this format from CI for now), and (2) need to address the kernel license issue (maybe Ondrej will be quick to comment, so please don't hurry to make changes yet). |
Oh, I was too quick to state that. First we need to merge @claudioandre-br's PR reverting us to older Intel OpenCL, then you need to rebase on top of that. |
|
Just run (after the merge of #5383 ). |
|
No, we don't do things like that! Please remove this merge commit and rebase instead. |
e2ff584 to
0c60418
Compare
|
Looks good now, thanks @alainesp! Now waiting to see what CI says about the new format (expecting failure since it's known to fail on CPU in our manual tests). |
Using more GPU memory may result in better performance. You can change it using an appropriate GWS. |
Yes, got slight speedup below - but it's hard to tune GWS for this format manually given that it does not report what value of GWS it had auto-selected (our typical formats report LWS and GWS, and this one eventually should too). Also, trying some GWS values gave me |
CI passed test - turns out we're only testing 16 OpenCL formats, @claudioandre-br? |
|
@alainesp Please also add a |
This is an encoding test (not OpenCL one). Only formats with some tag (FMT_UNICODE or FMT_ENC or ...) are being tested. |
|
Titan X Maxwell defaults: Doubled(?) GWS is slightly faster: Nearly max GWS is slower: |
|
Vega 64 defaults: Tuned: Looks like we'll eventually (beyond this PR) need some trick to bypass the max allocation on AMD, as the above is using at most half the memory. |
Hmm, this makes some sense. Can we afford to test all OpenCL formats (or excluding only those where failure is somehow expected)? |
|
Anyway, I think this PR is good enough to merge now! So I will. Further improvements are to be in their own PRs then. |
solardiz
left a comment
solardiz
left a comment
There was a problem hiding this comment.
There's of course a lot to clean up here, but this is a good start.
|
@alainesp For further occasions, please prefix your commit titles with subsystem name (where applicable), e.g. this one could have been |
The problem is that this would take a long time (the encoding test alone takes more than 10 minutes).
The Intel driver/runtime is slow. |
| HANDLE_CLERROR(clEnqueueWriteBufferRect(queue[gpu_id], memory_buffer, CL_FALSE, | ||
| zero3, zero3, | ||
| region3_in, | ||
| jobSize, 0, copySize, 0, blocks_in, 0, NULL, NULL), "Copy data to gpu"); |
There was a problem hiding this comment.
This is the only usage of clEnqueueWriteBufferRect currently in our tree. Is there any special reason why we need this specific OpenCL function here?
There was a problem hiding this comment.
Because we need to copy the memory to the GPU by pieces and not continuously, and that's what clEnqueueWriteBufferRect provides.
Looks like I underestimated the effect of that slight unrelated load back then. Testing now, I get only 4x speedup for GTX 1080 vs. 2x E5-2670 on idle system (CPU turbo on, GPU not yet fully heated up so also at max turbo). That's for our latest code, beyond what was in this PR. (The wordlist is different from what I used before. I now put the correct password around line 50k. I think previous one was with it at around like 10k.) |
|
Curiously, the above test (with our latest code, not from this PR's) works even on the ancient GTX 570 with 1280 MB: and our default tuning gets really close to the full memory size: This speed isn't bad for this ancient machine, too. Running the same on its FX-8120 CPU gives ~110 c/s. So even with such low GPU memory size, the GPU provides a 3x speedup over CPU on this 12 year old machine. |
|
Same test on RTX 5090: That's with correct password around line 50k, as in previous tests. Now trying with it around like 500k (since 4 seconds feels too quick for a proper test): |
3 participants
No description provided.