This change hopes to provide a better alignement of pkeyread with other #203

Sashan · 2024-06-17T14:06:33Z

performance tools we have. The pkeyread enables as to evaluate speed of reading various keys in der/pem formats. This requires user to specifi desired key and format on command line. This makes pkeyread different to other tools which seem to have a common set of options:
./tool_name --terse 2
command line above executes tool tool_name running test using 2 threads with terse output. The idea of this change is to provide implicit mode for pkeyread where key and format is determined from command name. For example to run evaulation for rsa key in pem format using 2 threads one runs tool:
./rsa.pem 2
The tool for rsa essentially becames a symlink to pkeyread binary.

This should help us to create us more simple shell scripts to run performacne test. For example to run all tests for pem keys we can do something like this in shell
for perf_tool in *.pem ; do
./$perf_tool --terse 2 ;
done

Sashan · 2024-06-17T22:10:04Z

rebased with current changes.

t8m · 2024-06-19T09:22:26Z

perf/Makefile

+	dh.pem dhx.pem dsa.pem ec.pem rsa.pem x25519.pem \
+	dh.der dhx.der dsa.der ec.der rsa.der x25519.der


These are really confusing names for aliases of pkeyread. Would you please consider something like pkeyread-dh-pem?

Also on Windows this will not be practical IMO. So perhaps just keeping the key type and formats as arguments and handling this within the perftesting script somehow would make more sense.

New version of diff uses suggested naming convention. The whole point of this exercise is to unify command line options handling of performance tools. In my opinion the effort here definitely pays off because then we can construct performance testing scripts without too much thinking which arguments to pass to each tool. It shortens debugging time spent to get tests running.

My plan for windows is to use COPY instead of symlink.

I am not sure about this approach. IMO it is not scalable. @openssl/otc what are others opinions?

I think it could be made scalable using a make variable to hold the list of possibilities.
Perhaps using some string manipulation functions to get things correct and only have one copy of the list.

paulidale · 2024-06-20T02:02:36Z

perf/Makefile

@@ -49,3 +57,39 @@ regen_key_samples:

 pkeyread: pkeyread.c keys.h libperf.a
 	$(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) -o pkeyread pkeyread.c -lperf -lcrypto
+
+pkeyread-dh-pem: pkeyread


What about converting this lot into:

pkeyread-%: pkeyread $(LN) -s pkeyread $@

or if the suggestion about using a variable to hold them, use that on the left of the colon.

paulidale · 2024-06-20T02:04:10Z

perf/Makefile

-	rm -f libperf.a *.o randbytes handshake sslnew newrawkey rsasign x509storeissuer providerdoall rwlocks pkeyread evp_fetch
+	rm -f libperf.a *.o randbytes handshake sslnew newrawkey rsasign x509storeissuer providerdoall rwlocks pkeyread evp_fetch \
+	pkeyread-dh-pem pkeyread-dhx-pem pkeyread-dsa-pem pkeyread-ec-pem pkeyread-rsa-pem pkeyread-x25519-pem \
+	pkeyread-dh-der pkeyread-dhx-der pkeyread-dsa-der pkeyread-ec-der pkeyread-rsa-der pkeyread-x25519-der


Consider defining a make variable to avoid duplication of all of these names?

paulidale · 2024-06-20T02:05:02Z

perf/pkeyread.c

@@ -217,6 +218,103 @@ static void usage(char * const argv[])
    } while (*format_name != NULL);
 }

+static void implicit_report_result(char * const argv[], int terse)


style nit: no space after the *

paulidale · 2024-06-20T02:05:16Z

perf/pkeyread.c

+	    get_avcalltime());
+}
+
+static int implicit_main(int argc, char * const argv[])


style nit: no space after the *

Sashan · 2024-06-20T08:37:51Z

new version brings all @paulidale suggestions in.
thanks for the Makefile hint.

paulidale · 2024-06-20T22:27:47Z

perf/Makefile

@@ -49,3 +57,6 @@ regen_key_samples:

 pkeyread: pkeyread.c keys.h libperf.a
 	$(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) -o pkeyread pkeyread.c -lperf -lcrypto
+
+$(PKEYREAD_SYMLINKS) pkeyread-%: pkeyread


Drop the pkeyread-% it's not required

perf/Makefile

paulidale · 2024-06-20T22:30:39Z

perf/Makefile

@@ -13,6 +19,8 @@ LDFLAGS += -L$(TARGET_OSSL_LIBRARY_PATH) -L.
 # For setting RUNPATH on built executables uncomment this:
 # LDFLAGS += -Wl,-rpath,$(TARGET_OSSL_LIBRARY_PATH)

+LN=/bin/ln


This precludes the makefile working on non-unix like targets. I'm not sure that's a good idea.

the easiest hack for platforms with no symlinks we can do is just LN=copy.

The current Makefile as-is assumes unix-like environment anyway. more work is required to build tools on windows.

given we can not agree on wether implicit arguments approach is acceptable for rwlocks tests here: #205 I'm going to extend run-perf.sh script we discuss here in #207 so it will be able to deal with explicit arguments. I feel this approach to be easier for everyone.

performance tools we have. The pkeyread enables as to evaluate speed of reading various keys in der/pem formats. This requires user to specifi desired key and format on command line. This makes pkeyread different to other tools which seem to have a common set of options: ./tool_name --terse 2 command line above executes tool tool_name running test using 2 threads with terse output. The idea of this change is to provide implicit mode for pkeyread where key and format is determined from command name. For example to run evaulation for rsa key in pem format using 2 threads one runs tool: ./rsa.pem 2 The tool for rsa essentially becames a symlink to pkeyread binary. This should help us to create us more simple shell scripts to run performacne test. For example to run all tests for pem keys we can do something like this in shell for perf_tool in *.pem ; do ./$perf_tool --terse 2 ; done

of implicit arguments (symlinks to tools). The approach is discussed in those two PRs: openssl#205 openssl#203

Sashan · 2024-07-02T20:26:21Z

@vdukhovni version of run-perf.sh no longer requires this hack to pkeyread. I'd like to close this pull request as unmerged.

Sashan force-pushed the implicit.pkeyread branch from 444ffec to 5f73b2a Compare June 17, 2024 22:07

t8m requested changes Jun 19, 2024

View reviewed changes

Sashan force-pushed the implicit.pkeyread branch 3 times, most recently from eee3bcd to 3eb94e6 Compare June 20, 2024 01:38

paulidale requested changes Jun 20, 2024

View reviewed changes

Sashan force-pushed the implicit.pkeyread branch from 3eb94e6 to 6ef81f3 Compare June 20, 2024 08:36

Sashan mentioned this pull request Jun 20, 2024

This change adds run-perf.sh script. The script runs all tools #207

Closed

paulidale reviewed Jun 20, 2024

View reviewed changes

perf/Makefile Show resolved Hide resolved

paulidale reviewed Jun 20, 2024

View reviewed changes

Sashan force-pushed the implicit.pkeyread branch from 6ef81f3 to af71db5 Compare June 20, 2024 23:10

Sashan added a commit to Sashan/tools that referenced this pull request Jun 21, 2024

this branch saves run-perf.sh in variant which require support

ff5eb19

of implicit arguments (symlinks to tools). The approach is discussed in those two PRs: openssl#205 openssl#203

nhorman approved these changes Jul 1, 2024

View reviewed changes

Sashan closed this Jul 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

This change hopes to provide a better alignement of pkeyread with other #203

This change hopes to provide a better alignement of pkeyread with other #203

Uh oh!

Sashan commented Jun 17, 2024

Uh oh!

Sashan commented Jun 17, 2024

Uh oh!

t8m Jun 19, 2024

Uh oh!

Sashan Jun 19, 2024

Uh oh!

t8m Jun 19, 2024

Uh oh!

paulidale Jun 20, 2024

Uh oh!

paulidale Jun 20, 2024

Uh oh!

paulidale Jun 20, 2024

Uh oh!

paulidale Jun 20, 2024

Uh oh!

paulidale Jun 20, 2024

Uh oh!

Sashan commented Jun 20, 2024

Uh oh!

paulidale Jun 20, 2024

Uh oh!

Uh oh!

paulidale Jun 20, 2024

Uh oh!

Sashan Jun 20, 2024

Uh oh!

Sashan commented Jul 2, 2024

Uh oh!

Uh oh!

		dh.pem dhx.pem dsa.pem ec.pem rsa.pem x25519.pem \
		dh.der dhx.der dsa.der ec.der rsa.der x25519.der

Uh oh!

This change hopes to provide a better alignement of pkeyread with other #203

This change hopes to provide a better alignement of pkeyread with other #203

Uh oh!

Conversation

Sashan commented Jun 17, 2024

Uh oh!

Sashan commented Jun 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sashan commented Jun 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sashan commented Jul 2, 2024

Uh oh!

Uh oh!