CLI: parallelize mfkey32v2 processes #187

p-l- · 2023-12-09T19:17:34Z

This PR will run as many mfkey32v2 processes in parallel than the host CPUs count. It makes the hf mf elog --decrypt command much faster.

github-actions · 2023-12-09T19:17:51Z

You are welcome to add an entry to the CHANGELOG.md as well

github-actions · 2023-12-09T19:20:12Z

Built artifacts for commit `cc674f1`

Firmware

Client

MrFlou · 2023-12-14T12:16:57Z

software/script/chameleon_cli_unit.py

+            items[1]["nt"],
+            items[1]["nr"],
+            items[1]["ar"],
+        ],


Personally needed to add shell=True in the subprocess.run() args
Running this one a windows machine, not sure how this will affect a different OS

Suggested change

],

],

shell=True,

Thanks! Can you confirm that with this patch you needed to add shell=True? With the exact same command, without changing anything else?
That's very surprising to me! Could you share the output without adding shell=True?

Sure! Here it is without shell=True

multiprocessing.pool.RemoteTraceback: """ Traceback (most recent call last): File "C:/Users/USER/Desktop/Chamilion Ultra/CLI/ProxSpace/msys2/mingw64/lib/python3.11/multiprocessing/pool.py", line 125, in worker result = (True, func(*args, **kwds)) ^^^^^^^^^^^^^^^^^^^ File "C:/Users/USER/Desktop/Chamilion Ultra/CLI/ProxSpace/msys2/home/USER/ChameleonUltra/software/script/chameleon_cli_unit.py", line 1030, in _run_mfkey32v2 output_str = subprocess.run( ^^^^^^^^^^^^^^^ File "C:/Users/USER/Desktop/Chamilion Ultra/CLI/ProxSpace/msys2/mingw64/lib/python3.11/subprocess.py", line 548, in run with Popen(*popenargs, **kwargs) as process: ^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "C:/Users/USER/Desktop/Chamilion Ultra/CLI/ProxSpace/msys2/mingw64/lib/python3.11/subprocess.py", line 1026, in __init__ self._execute_child(args, executable, preexec_fn, close_fds, File "C:/Users/USER/Desktop/Chamilion Ultra/CLI/ProxSpace/msys2/mingw64/lib/python3.11/subprocess.py", line 1538, in _execute_child hp, ht, pid, tid = _winapi.CreateProcess(executable, args, ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FileNotFoundError: [WinError 2] The system cannot find the file specified """ The above exception was the direct cause of the following exception: Traceback (most recent call last): File "C:/Users/USER/Desktop/Chamilion Ultra/CLI/ProxSpace/msys2/home/USER/ChameleonUltra/software/script/chameleon_cli_main.py", line 144, in exec_cmd raise error File "C:/Users/USER/Desktop/Chamilion Ultra/CLI/ProxSpace/msys2/home/USER/ChameleonUltra/software/script/chameleon_cli_main.py", line 139, in exec_cmd unit.on_exec(args_parse_result) File "C:/Users/USER/Desktop/Chamilion Ultra/CLI/ProxSpace/msys2/home/USER/ChameleonUltra/software/script/chameleon_cli_unit.py", line 1137, in on_exec result_maps[uid][block]['A'] = self.decrypt_by_list(records) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "C:/Users/USER/Desktop/Chamilion Ultra/CLI/ProxSpace/msys2/home/USER/ChameleonUltra/software/script/chameleon_cli_unit.py", line 1076, in decrypt_by_list for key in pool.imap( File "C:/Users/USER/Desktop/Chamilion Ultra/CLI/ProxSpace/msys2/mingw64/lib/python3.11/multiprocessing/pool.py", line 873, in next raise value FileNotFoundError: [WinError 2] The system cannot find the file specified

Oh OK, I'm having a look. Thanks for your answer!

Can you try if it if fixed @MrFlou?

MrFlou · 2023-12-15T20:51:34Z

software/script/chameleon_cli_unit.py

@@ -1029,7 +1029,7 @@ def res_value(self, src_blk, src_type, src_key, dst_blk, dst_type, dst_key):
 def _run_mfkey32v2(items):
    output_str = subprocess.run(
        [
-            "mfkey32v2.exe" if sys.platform == "win32" else "./mfkey32v2",
+            ".\\mfkey32v2.exe" if sys.platform == "win32" else "./mfkey32v2",


Tried following the changes you did and got this working.

Suggested change

".\\mfkey32v2.exe" if sys.platform == "win32" else "./mfkey32v2",

"./bin/mfkey32v2.exe" if sys.platform == "win32" else "./mfkey32v2",

That is really weird, I don't understand...
Can you add a print(default_cwd) just after the default_cwd = definition (line 41), then run the CLI and report the path displayed? On Linux I get /home/user/Github/RfidResearchGroup/ChameleonUltra/software/script/bin. I expect you'd get something similar under Windows, but based on your suggestion it seems you won't have the bin final directory?

C:/Users/USER/Desktop/Chamilion Ultra/CLI/ProxSpace/msys2/home/USER/ChameleonUltra/software/script/bin
I do have the bin. Could it be I'm running it with the ProxySpace guide?

OK thanks a lot! I've tried something else, that should work in any environment. Can you check again?

@p-l- This works!

Thanks, appreciate your help! Did you notice a performance improvement?

Yes, running much faster!

p-l- · 2023-12-19T21:50:51Z

Hello @xianglin1998 @doegox sorry for the ping. This PR has now been tested on both Windows (thanks @MrFlou) and Linux. Could you have a look? It really speeds up running hf mf elog --decrypt.

doegox · 2023-12-19T23:08:44Z

CHANGELOG.md

@@ -26,6 +26,7 @@ This project uses the changelog in accordance with [keepchangelog](http://keepac
 - Added support for timestamped comments in CLI via `rem`, `;`, `%` or `#` (@doegox)
 - Fixed watchdog trigger during `hw factory_reset` (@doegox)
 - Added PyInstaller support for CLI client (@augustozanellato)
+ - Parallelize mfkey32v2 processes called from CLI (@p-l-)


new changelog entries should be moved to the top of the list

Sure, makes sense, sorry. That's done!

doegox · 2023-12-20T15:29:03Z

Thanks! you know you don't need to rewrite and force-push commits, you can just add a commit in your branch and it'll be added to the existing PR.

This (often largely) improves the speed of the decrypt process. On my laptop, with the same logs (37 records for one block and 37 records for another block), here are the performances, as measuerd using a simple command: ```bash time echo -e "hw connect\nhf mf elog --decrypt\nhw disconnect" | ./chameleon_cli_main.py ``` - Before parallelisation (RfidResearchGroup#187): 14m59,277s - With parallelisation (current main): 6m13,513s - With item skipping (this PR): 2m42,491s

p-l- · 2023-12-21T00:02:43Z

Thanks @doegox. Actually I just amended the commit (hence the push force), I did not rewrite the PR!

doegox · 2023-12-21T08:54:37Z

You don't need to rewrite a PR.
You don't need to amend a commit and force-push it.
You can just push a new commit in your existing branch and it will automatically appear in the existing PR.
That way, it's easier for reviewers to know what you changed after the reviewers comments. But if you amend the commit, then we've to read it all again and try to spot what you changed.

Not an issue here as it was just the changelog but this workflow may help in future commits.
Thanks!

p-l- · 2023-12-21T10:17:29Z

OK noted.

As a maintainer I use the link "force-pushed" in the message (@p-l- force-pushed the enh-hf-mf-elog-parallel branch from 9174b02 to cc674f1), but that's totally OK for me to add new commits, I'll do that for my future PRs to this project.

Thanks for the work & the super cool project BTW!

doegox · 2023-12-21T10:44:24Z

oh I didn't know that force-pushed link, thanks !

It is done by adding a "server" mode (on STDIN/STDOUT) to `mfkey32v2` using `--server`. This improves the speed of the decrypt process, as it prevents a lot of Python calls to `subprocess.Popen()` (and of course, a lot of fork/execve syscalls). On my laptop, with the same logs (37 records for one block and 37 records for another block), here are the performances, as measuerd using a simple command: ```bash time echo -e "hw connect\nhf mf elog --decrypt\nhw disconnect" | ./chameleon_cli_main.py ``` | | | real | user | sys | |---------------------------|---------|------------|------------|-----------| | Before parallelisation | 05ea03d | 14m59,277s | 14m47,995s | 0m8,490s | | With parallelisation | RfidResearchGroup#187 | 6m13,513s | 35m2,926s | 0m22,038s | | With item skipping | RfidResearchGroup#189 | 2m42,491s | 15m43,425s | 0m9,881s | | With `mfkey32v2 --server` | this PR | 1m55,160s | 0m1,315s | 0m0,250s |

p-l- force-pushed the enh-hf-mf-elog-parallel branch 2 times, most recently from 09d0174 to 02165a0 Compare December 9, 2023 19:21

p-l- marked this pull request as ready for review December 9, 2023 19:25

MrFlou suggested changes Dec 14, 2023

View reviewed changes

MrFlou suggested changes Dec 15, 2023

View reviewed changes

p-l- force-pushed the enh-hf-mf-elog-parallel branch from 426dd30 to 9174b02 Compare December 18, 2023 08:26

MrFlou approved these changes Dec 19, 2023

View reviewed changes

doegox reviewed Dec 19, 2023

View reviewed changes

p-l- added 2 commits December 20, 2023 16:20

CLI: parallelize mfkey32v2 processes

42317a7

CLI: fix mfkey32v2 call for Windows

cc674f1

p-l- force-pushed the enh-hf-mf-elog-parallel branch from 9174b02 to cc674f1 Compare December 20, 2023 15:20

doegox merged commit 0124709 into RfidResearchGroup:main Dec 20, 2023
8 checks passed

p-l- deleted the enh-hf-mf-elog-parallel branch December 20, 2023 15:30

p-l- mentioned this pull request Dec 20, 2023

CLI: skip already used items in hf mf elog --decrypt #189

Merged

p-l- mentioned this pull request Dec 21, 2023

CLI: use one process to test many keys for mfkey32v2 #191

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI: parallelize mfkey32v2 processes #187

CLI: parallelize mfkey32v2 processes #187

p-l- commented Dec 9, 2023

github-actions bot commented Dec 9, 2023

github-actions bot commented Dec 9, 2023 •

edited

Loading

MrFlou Dec 14, 2023

p-l- Dec 14, 2023

MrFlou Dec 14, 2023

p-l- Dec 15, 2023

p-l- Dec 15, 2023

MrFlou Dec 15, 2023 •

edited

Loading

p-l- Dec 17, 2023

MrFlou Dec 17, 2023 •

edited

Loading

p-l- Dec 18, 2023

MrFlou Dec 18, 2023

p-l- Dec 18, 2023

MrFlou Dec 19, 2023

p-l- commented Dec 19, 2023

doegox Dec 19, 2023

p-l- Dec 20, 2023

doegox commented Dec 20, 2023

p-l- commented Dec 21, 2023

doegox commented Dec 21, 2023 •

edited

Loading

p-l- commented Dec 21, 2023

doegox commented Dec 21, 2023

	".\\mfkey32v2.exe" if sys.platform == "win32" else "./mfkey32v2",
	"./bin/mfkey32v2.exe" if sys.platform == "win32" else "./mfkey32v2",

CLI: parallelize mfkey32v2 processes #187

CLI: parallelize mfkey32v2 processes #187

Conversation

p-l- commented Dec 9, 2023

github-actions bot commented Dec 9, 2023

github-actions bot commented Dec 9, 2023 • edited Loading

Built artifacts for commit cc674f1

Firmware

Client

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrFlou Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrFlou Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-l- commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doegox commented Dec 20, 2023

p-l- commented Dec 21, 2023

doegox commented Dec 21, 2023 • edited Loading

p-l- commented Dec 21, 2023

doegox commented Dec 21, 2023

github-actions bot commented Dec 9, 2023 •

edited

Loading

Built artifacts for commit `cc674f1`

MrFlou Dec 15, 2023 •

edited

Loading

MrFlou Dec 17, 2023 •

edited

Loading

doegox commented Dec 21, 2023 •

edited

Loading