Re: openssl s_time: refactor benchmark loops
On Sat, Aug 11, 2018 at 10:51:12AM -0500, Scott Cheloha wrote:
> On Sat, Aug 11, 2018 at 11:22:09PM +0900, Kinichiro Inoguchi wrote:
> > Hi,
> >
> > I saw that 2 loops are almost identical, and I think diff tidy up these 2
> > loops
> > in 1 function.
> >
> > I found these 2 lines were not in original code.
> >
> > > + shutdown(SSL_get_fd(scon), SHUT_RDWR);
> > > + close(SSL_get_fd(scon));
> >
> > Is this part needed ?
>
> Nope, good catch. Those are leftovers from an older
> revision of this patch and would be a regression.
>
> Updated diff below.
ok tb
>
> Index: usr.bin/openssl/s_time.c
> ===
> RCS file: /cvs/src/usr.bin/openssl/s_time.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 s_time.c
> --- usr.bin/openssl/s_time.c 13 Jul 2018 18:36:56 - 1.24
> +++ usr.bin/openssl/s_time.c 11 Aug 2018 15:48:02 -
> @@ -91,6 +91,7 @@ extern int verify_depth;
>
> static void s_time_usage(void);
> static SSL *doConnection(SSL * scon);
> +static int benchmark(int);
>
> static SSL_CTX *tm_ctx = NULL;
> static const SSL_METHOD *s_time_meth = NULL;
> @@ -244,13 +245,7 @@ tm_Time_F(int op)
> int
> s_time_main(int argc, char **argv)
> {
> - double totalTime = 0.0;
> - int nConn = 0;
> - SSL *scon = NULL;
> - time_t finishtime;
> int ret = 1;
> - char buf[1024 * 8];
> - int ver;
>
> if (single_execution) {
> if (pledge("stdio rpath inet dns", NULL) == -1) {
> @@ -328,60 +323,8 @@ 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;
> - tm_Time_F(START);
> - for (;;) {
> - if (finishtime < time(NULL))
> - break;
> - if ((scon = doConnection(NULL)) == NULL)
> - goto end;
> -
> - if (s_time_config.www_path != NULL) {
> - int i, retval = snprintf(buf, sizeof buf,
> - "GET %s HTTP/1.0\r\n\r\n", s_time_config.www_path);
> - if ((size_t)retval >= sizeof buf) {
> - fprintf(stderr, "URL too long\n");
> - goto end;
> - }
> - SSL_write(scon, buf, strlen(buf));
> - while ((i = SSL_read(scon, buf, sizeof(buf))) > 0)
> - bytes_read += i;
> - }
> - if (s_time_config.no_shutdown)
> - SSL_set_shutdown(scon, SSL_SENT_SHUTDOWN |
> - SSL_RECEIVED_SHUTDOWN);
> - else
> - SSL_shutdown(scon);
> -
> - nConn += 1;
> - if (SSL_session_reused(scon))
> - ver = 'r';
> - else {
> - ver = SSL_version(scon);
> - if (ver == TLS1_VERSION)
> - ver = 't';
> - else if (ver == SSL3_VERSION)
> - ver = '3';
> - else if (ver == SSL2_VERSION)
> - ver = '2';
> - else
> - ver = '*';
> - }
> - fputc(ver, stdout);
> - fflush(stdout);
> -
> - SSL_free(scon);
> - scon = NULL;
> - }
> - totalTime += tm_Time_F(STOP); /* Add the time for this iteration */
> -
> - 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),
> - bytes_read / nConn);
> + if (benchmark(0))
> + goto end;
>
> /*
>* Now loop and time connections using the same session id over and
> @@ -393,88 +336,11 @@ s_time_main(int argc, char **argv)
> goto end;
> printf("\n\nNow timing with session id reuse.\n");
>
> - /* Get an SSL object so we can reuse the session id */
> - if ((scon = doConnection(NULL)) == NULL) {
> - fprintf(stderr, "Unable to get connection\n");
> + if (benchmark(1))
> goto end;
> - }
> - if (s_time_config.www_path != NULL) {
> - int retval = snprintf(buf, sizeof buf,
> - "GET %s HTTP/1.0\r\n\r\n", s_time_config.www_path);
> - if ((size_t)retval >= sizeof buf) {
> - fprintf(stderr, "URL too long\n");
> - goto end;
> - }
> - SSL_write(scon, buf, strlen(buf));
> - while (SSL_read(scon, buf, sizeof(buf)) > 0);
> - }
> - if (s_time_config.no_shutdown)
> -
Re: openssl s_time: refactor benchmark loops
On Sat, Aug 11, 2018 at 11:22:09PM +0900, Kinichiro Inoguchi wrote:
> Hi,
>
> I saw that 2 loops are almost identical, and I think diff tidy up these 2
> loops
> in 1 function.
>
> I found these 2 lines were not in original code.
>
> > + shutdown(SSL_get_fd(scon), SHUT_RDWR);
> > + close(SSL_get_fd(scon));
>
> Is this part needed ?
Nope, good catch. Those are leftovers from an older
revision of this patch and would be a regression.
Updated diff below.
Index: usr.bin/openssl/s_time.c
===
RCS file: /cvs/src/usr.bin/openssl/s_time.c,v
retrieving revision 1.24
diff -u -p -r1.24 s_time.c
--- usr.bin/openssl/s_time.c13 Jul 2018 18:36:56 - 1.24
+++ usr.bin/openssl/s_time.c11 Aug 2018 15:48:02 -
@@ -91,6 +91,7 @@ extern int verify_depth;
static void s_time_usage(void);
static SSL *doConnection(SSL * scon);
+static int benchmark(int);
static SSL_CTX *tm_ctx = NULL;
static const SSL_METHOD *s_time_meth = NULL;
@@ -244,13 +245,7 @@ tm_Time_F(int op)
int
s_time_main(int argc, char **argv)
{
- double totalTime = 0.0;
- int nConn = 0;
- SSL *scon = NULL;
- time_t finishtime;
int ret = 1;
- char buf[1024 * 8];
- int ver;
if (single_execution) {
if (pledge("stdio rpath inet dns", NULL) == -1) {
@@ -328,60 +323,8 @@ 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;
- tm_Time_F(START);
- for (;;) {
- if (finishtime < time(NULL))
- break;
- if ((scon = doConnection(NULL)) == NULL)
- goto end;
-
- if (s_time_config.www_path != NULL) {
- int i, retval = snprintf(buf, sizeof buf,
- "GET %s HTTP/1.0\r\n\r\n", s_time_config.www_path);
- if ((size_t)retval >= sizeof buf) {
- fprintf(stderr, "URL too long\n");
- goto end;
- }
- SSL_write(scon, buf, strlen(buf));
- while ((i = SSL_read(scon, buf, sizeof(buf))) > 0)
- bytes_read += i;
- }
- if (s_time_config.no_shutdown)
- SSL_set_shutdown(scon, SSL_SENT_SHUTDOWN |
- SSL_RECEIVED_SHUTDOWN);
- else
- SSL_shutdown(scon);
-
- nConn += 1;
- if (SSL_session_reused(scon))
- ver = 'r';
- else {
- ver = SSL_version(scon);
- if (ver == TLS1_VERSION)
- ver = 't';
- else if (ver == SSL3_VERSION)
- ver = '3';
- else if (ver == SSL2_VERSION)
- ver = '2';
- else
- ver = '*';
- }
- fputc(ver, stdout);
- fflush(stdout);
-
- SSL_free(scon);
- scon = NULL;
- }
- totalTime += tm_Time_F(STOP); /* Add the time for this iteration */
-
- 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),
- bytes_read / nConn);
+ if (benchmark(0))
+ goto end;
/*
* Now loop and time connections using the same session id over and
@@ -393,88 +336,11 @@ s_time_main(int argc, char **argv)
goto end;
printf("\n\nNow timing with session id reuse.\n");
- /* Get an SSL object so we can reuse the session id */
- if ((scon = doConnection(NULL)) == NULL) {
- fprintf(stderr, "Unable to get connection\n");
+ if (benchmark(1))
goto end;
- }
- if (s_time_config.www_path != NULL) {
- int retval = snprintf(buf, sizeof buf,
- "GET %s HTTP/1.0\r\n\r\n", s_time_config.www_path);
- if ((size_t)retval >= sizeof buf) {
- fprintf(stderr, "URL too long\n");
- goto end;
- }
- SSL_write(scon, buf, strlen(buf));
- while (SSL_read(scon, buf, sizeof(buf)) > 0);
- }
- if (s_time_config.no_shutdown)
- SSL_set_shutdown(scon, SSL_SENT_SHUTDOWN |
- SSL_RECEIVED_SHUTDOWN);
- else
- SSL_shutdown(scon);
-
- nConn = 0;
-
