On Sat, Nov 11 2017, Scott Cheloha <[email protected]> wrote:
> Hi,
Hi,
> times(3) is okay for user CPU measurement but is inappropriate
> for absolute interval measurement as its output is subject to
> changes by both adjtime(2) and settimeofday(2).
>
> The attached diff replaces it with getrusage(2) for user CPU
> measurement and clock_gettime(2)'s CLOCK_MONOTONIC clock for
> absolute interval measurement.
>
> The attached diff also replaces time(3) in s_time with
> clock_gettime's CLOCK_MONOTONIC clock. This ensures that we
> only measure for about as long as the user said to.
>
> Neither timersub(2) nor timespecsub are standard, though many
> systems have them. Is this a problem for libressl-portable?
I doubt that timersub/timespecsub are a big problem to add to -portable,
they're just macros. clock_gettime and getrusage seem to already be
used in libressl(-portable), there's autoconf glue for the former at
least.
> Thoughts and feedback?
More comment inline.
> --
> Scott Cheloha
>
> Index: usr.bin/openssl/apps.h
> ===================================================================
> RCS file: /cvs/src/usr.bin/openssl/apps.h,v
> retrieving revision 1.19
> diff -u -p -r1.19 apps.h
> --- usr.bin/openssl/apps.h 30 Aug 2016 14:34:59 -0000 1.19
> +++ usr.bin/openssl/apps.h 10 Nov 2017 18:38:13 -0000
> @@ -277,9 +277,8 @@ unsigned char *next_protos_parse(unsigne
>
> int app_isdir(const char *);
>
> -#define TM_START 0
> -#define TM_STOP 1
> -double app_tminterval (int stop, int usertime);
> +double real_interval(int new);
> +double user_interval(int new);
>
> #define OPENSSL_NO_SSL_INTERN
>
> Index: usr.bin/openssl/apps_posix.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/openssl/apps_posix.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 apps_posix.c
> --- usr.bin/openssl/apps_posix.c 13 Sep 2015 12:41:01 -0000 1.2
> +++ usr.bin/openssl/apps_posix.c 10 Nov 2017 18:38:13 -0000
> @@ -116,31 +116,39 @@
> * Functions that need to be overridden by non-POSIX operating systems.
> */
>
> -#include <sys/times.h>
> +#include <sys/resource.h>
>
> -#include <unistd.h>
> +#include <time.h>
>
> #include "apps.h"
>
> double
> -app_tminterval(int stop, int usertime)
> +real_interval(int new)
I suggest you keep app_tminterval() as the entry point here. As
mentioned by the comment, non-POSIX systems need to implement the same
interface, and indeed apps/openssl/apps_win.c in libressl-portable also
provides app_tminterval().
> {
> - double ret = 0;
> - struct tms rus;
> - clock_t now = times(&rus);
> - static clock_t tmstart;
> -
> - if (usertime)
> - now = rus.tms_utime;
> -
> - if (stop == TM_START)
> - tmstart = now;
> - else {
> - long int tck = sysconf(_SC_CLK_TCK);
> - ret = (now - tmstart) / (double) tck;
> + static struct timespec elapsed, now, start;
Only "start" need to be static here.
> +
> + clock_gettime(CLOCK_MONOTONIC, &now);
> + if (new) {
> + start = now;
> + return 0.0;
> }
> + timespecsub(&now, &start, &elapsed);
> + return elapsed.tv_sec + elapsed.tv_nsec / 1000000000.0;
> +}
>
> - return (ret);
> +double
> +user_interval(int new)
> +{
> + static struct timeval elapsed, start;
> + static struct rusage now;
Same here.
> +
> + getrusage(RUSAGE_SELF, &now);
> + if (new) {
> + start = now.ru_utime;
> + return 0.0;
> + }
> + timersub(&now.ru_utime, &start, &elapsed);
> + return elapsed.tv_sec + elapsed.tv_usec / 1000000.0;
> }
>
> int
> Index: usr.bin/openssl/s_time.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/openssl/s_time.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 s_time.c
> --- usr.bin/openssl/s_time.c 2 Nov 2017 00:31:49 -0000 1.18
> +++ usr.bin/openssl/s_time.c 10 Nov 2017 18:38:13 -0000
> @@ -68,6 +68,7 @@
> #include <stdlib.h>
> #include <limits.h>
> #include <string.h>
> +#include <time.h>
> #include <unistd.h>
> #include <poll.h>
>
> @@ -229,14 +230,9 @@ s_time_usage(void)
> /***********************************************************************
> * TIME - time functions
> */
> -#define START 0
> -#define STOP 1
> -
> -static double
> -tm_Time_F(int s)
> -{
> - return app_tminterval(s, 1);
> -}
> +#define START 1
> +#define STOP 0
> +#define tm_Time_F(s) user_interval(s);
>
> /***********************************************************************
> * MAIN - main processing area for client
> @@ -248,7 +244,7 @@ s_time_main(int argc, char **argv)
> double totalTime = 0.0;
> int nConn = 0;
> SSL *scon = NULL;
> - time_t finishtime;
> + struct timespec finishtime, now;
> int ret = 1;
> char buf[1024 * 8];
> int ver;
> @@ -330,10 +326,12 @@ s_time_main(int argc, char **argv)
> /* Loop and time how long it takes to make connections */
>
> bytes_read = 0;
> - finishtime = time(NULL) + s_time_config.maxtime;
> + clock_gettime(CLOCK_MONOTONIC, &finishtime);
> + finishtime.tv_sec += s_time_config.maxtime;
> tm_Time_F(START);
> for (;;) {
> - if (finishtime < time(NULL))
> + clock_gettime(CLOCK_MONOTONIC, &now);
> + if (timespeccmp(&finishtime, &now, <))
> break;
> if ((scon = doConnection(NULL)) == NULL)
> goto end;
> @@ -383,7 +381,7 @@ s_time_main(int argc, char **argv)
> nConn, totalTime, ((double) nConn / totalTime), bytes_read);
> printf("%d connections in %lld real seconds, %ld bytes read per
> connection\n",
> nConn,
> - (long long)(time(NULL) - finishtime + s_time_config.maxtime),
> + (long long)(now.tv_sec - finishtime.tv_sec + s_time_config.maxtime),
> bytes_read / nConn);
>
> /*
> @@ -422,14 +420,16 @@ next:
> nConn = 0;
> totalTime = 0.0;
>
> - finishtime = time(NULL) + s_time_config.maxtime;
> + clock_gettime(CLOCK_MONOTONIC, &finishtime);
> + finishtime.tv_sec += s_time_config.maxtime;
>
> printf("starting\n");
> bytes_read = 0;
> tm_Time_F(START);
>
> for (;;) {
> - if (finishtime < time(NULL))
> + clock_gettime(CLOCK_MONOTONIC, &now);
> + if (timespeccmp(&finishtime, &now, <))
> break;
> if ((doConnection(scon)) == NULL)
> goto end;
> @@ -475,7 +475,7 @@ next:
> printf("\n\n%d connections in %.2fs; %.2f connections/user sec, bytes
> read %ld\n", nConn, totalTime, ((double) nConn / totalTime), bytes_read);
> printf("%d connections in %lld real seconds, %ld bytes read per
> connection\n",
> nConn,
> - (long long)(time(NULL) - finishtime + s_time_config.maxtime),
> + (long long)(now.tv_sec - finishtime.tv_sec + s_time_config.maxtime),
> bytes_read / nConn);
>
> ret = 0;
> Index: usr.bin/openssl/speed.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/openssl/speed.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 speed.c
> --- usr.bin/openssl/speed.c 7 Oct 2017 06:16:54 -0000 1.20
> +++ usr.bin/openssl/speed.c 10 Nov 2017 18:38:13 -0000
> @@ -85,6 +85,7 @@
> #include <stdlib.h>
> #include <limits.h>
> #include <string.h>
> +#include <time.h>
> #include <unistd.h>
>
> #include "apps.h"
> @@ -153,7 +154,6 @@ int run = 0;
> static int mr = 0;
> static int usertime = 1;
>
> -static double Time_F(int s);
> static void print_message(const char *s, long num, int length);
> static void
> pkey_print_message(const char *str, const char *str2,
> @@ -195,16 +195,11 @@ sig_done(int sig)
> run = 0;
> }
>
> -#define START 0
> -#define STOP 1
> -
> -
> -static double
> -Time_F(int s)
> -{
> - return app_tminterval(s, usertime);
> -}
> +#define START 1
> +#define STOP 0
>
> +static double (*interval_function)(int) = user_interval;
> +#define Time_F(s) interval_function(s)
>
> static const int KDF1_SHA1_len = 20;
> static void *
> @@ -947,8 +942,10 @@ speed_main(int argc, char **argv)
> if (doit[i])
> pr_header++;
>
> - if (usertime == 0 && !mr)
> + if (usertime == 0 && !mr) {
> BIO_printf(bio_err, "You have chosen to measure elapsed time
> instead of user CPU time.\n");
> + interval_function = real_interval;
It seems a bit overengineered to use a function pointer for this. No
need to change this code if you just keep the app_tminterval()
interface.
> + }
>
> for (i = 0; i < RSA_NUM; i++) {
> const unsigned char *p;
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
