diff mbox series

[v2] app/test-crypto-perf: fix segment size for IPsec operation

Message ID 20211116103714.2148007-1-g.singh@nxp.com (mailing list archive)
State Superseded, archived
Delegated to: akhil goyal
Headers
Series [v2] app/test-crypto-perf: fix segment size for IPsec operation |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-intel-Performance fail Performance Testing issues
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS

Commit Message

Gagandeep Singh Nov. 16, 2021, 10:37 a.m. UTC 
  Application calculates segment size based on buffer size plus
digest size only, But if the operation mode is IPsec then
packet length can be increased up to 73 bytes due to IPsec
overhead.

In this patch, adding the IPsec overhead length in segment size
when there is no user given segment size.

Fixes: 28dde5da503e ("app/crypto-perf: support lookaside IPsec")

Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
---
v2-change-log:
Update commit message with fixline.

 app/test-crypto-perf/cperf_options.h         | 1 +
 app/test-crypto-perf/cperf_options_parsing.c | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Akhil Goyal Nov. 16, 2021, 10:42 a.m. UTC | #1 
> Application calculates segment size based on buffer size plus
> digest size only, But if the operation mode is IPsec then
> packet length can be increased up to 73 bytes due to IPsec
> overhead.
> 

Can you explain the calculation for 73 bytes in the code?
Will it be sufficient for IPv6?

> In this patch, adding the IPsec overhead length in segment size
> when there is no user given segment size.
> 
> Fixes: 28dde5da503e ("app/crypto-perf: support lookaside IPsec")
> 
> Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
> ---
> v2-change-log:
> Update commit message with fixline.
> 
>  app/test-crypto-perf/cperf_options.h         | 1 +
>  app/test-crypto-perf/cperf_options_parsing.c | 5 ++++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-crypto-perf/cperf_options.h b/app/test-crypto-
> perf/cperf_options.h
> index 031b238b20..cdbc027b89 100644
> --- a/app/test-crypto-perf/cperf_options.h
> +++ b/app/test-crypto-perf/cperf_options.h
> @@ -61,6 +61,7 @@
>  #define CPERF_PMDCC_DELAY_MS	("pmd-cyclecount-delay-ms")
> 
>  #define MAX_LIST 32
> +#define CPERF_IPSEC_OVERHEAD 73
> 
>  enum cperf_perf_test_type {
>  	CPERF_TEST_TYPE_THROUGHPUT,
> diff --git a/app/test-crypto-perf/cperf_options_parsing.c b/app/test-crypto-
> perf/cperf_options_parsing.c
> index c244f81bbf..268f544936 100644
> --- a/app/test-crypto-perf/cperf_options_parsing.c
> +++ b/app/test-crypto-perf/cperf_options_parsing.c
> @@ -1132,9 +1132,12 @@ cperf_options_check(struct cperf_options
> *options)
>  	 * If segment size is not set, assume only one segment,
>  	 * big enough to contain the largest buffer and the digest
>  	 */
> -	if (options->segment_sz == 0)
> +	if (options->segment_sz == 0) {
>  		options->segment_sz = options->max_buffer_size +
>  				options->digest_sz;
> +		if (options->op_type == CPERF_IPSEC)
> +			options->segment_sz += CPERF_IPSEC_OVERHEAD;
> +	}
> 
>  	if (options->segment_sz < options->digest_sz) {
>  		RTE_LOG(ERR, USER1,
> --
> 2.25.1
Gagandeep Singh Nov. 16, 2021, 11:47 a.m. UTC | #2 
> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Tuesday, November 16, 2021 4:12 PM
> To: Gagandeep Singh <G.Singh@nxp.com>; dev@dpdk.org
> Subject: RE: [EXT] [PATCH v2] app/test-crypto-perf: fix segment size for IPsec
> operation
> 
> > Application calculates segment size based on buffer size plus
> > digest size only, But if the operation mode is IPsec then
> > packet length can be increased up to 73 bytes due to IPsec
> > overhead.
> >
> 
> Can you explain the calculation for 73 bytes in the code?
> Will it be sufficient for IPv6?
No, it will not cover IPv6, As currently only IPv4 test cases are there in the app.
But I guess it covers all the scenario which are supported by the app.
73 are the maximum bytes which can be added in 
AES - SHA  algo mode (41 +12 + 20 (including any padding)) 

But it will also not cover other complex scenario like NAT-T, AH.
I have verified this change with aes-cbc-hmac-sha1 and aes-gcm algos for 64,128,256, 512, 1280 bytes.

 What's your opinion on this? 

> 
> > In this patch, adding the IPsec overhead length in segment size
> > when there is no user given segment size.
> >
> > Fixes: 28dde5da503e ("app/crypto-perf: support lookaside IPsec")
> >
> > Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
> > ---
> > v2-change-log:
> > Update commit message with fixline.
> >
> >  app/test-crypto-perf/cperf_options.h         | 1 +
> >  app/test-crypto-perf/cperf_options_parsing.c | 5 ++++-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test-crypto-perf/cperf_options.h b/app/test-crypto-
> > perf/cperf_options.h
> > index 031b238b20..cdbc027b89 100644
> > --- a/app/test-crypto-perf/cperf_options.h
> > +++ b/app/test-crypto-perf/cperf_options.h
> > @@ -61,6 +61,7 @@
> >  #define CPERF_PMDCC_DELAY_MS	("pmd-cyclecount-delay-ms")
> >
> >  #define MAX_LIST 32
> > +#define CPERF_IPSEC_OVERHEAD 73
> >
> >  enum cperf_perf_test_type {
> >  	CPERF_TEST_TYPE_THROUGHPUT,
> > diff --git a/app/test-crypto-perf/cperf_options_parsing.c b/app/test-crypto-
> > perf/cperf_options_parsing.c
> > index c244f81bbf..268f544936 100644
> > --- a/app/test-crypto-perf/cperf_options_parsing.c
> > +++ b/app/test-crypto-perf/cperf_options_parsing.c
> > @@ -1132,9 +1132,12 @@ cperf_options_check(struct cperf_options
> > *options)
> >  	 * If segment size is not set, assume only one segment,
> >  	 * big enough to contain the largest buffer and the digest
> >  	 */
> > -	if (options->segment_sz == 0)
> > +	if (options->segment_sz == 0) {
> >  		options->segment_sz = options->max_buffer_size +
> >  				options->digest_sz;
> > +		if (options->op_type == CPERF_IPSEC)
> > +			options->segment_sz += CPERF_IPSEC_OVERHEAD;
> > +	}
> >
> >  	if (options->segment_sz < options->digest_sz) {
> >  		RTE_LOG(ERR, USER1,
> > --
> > 2.25.1
Akhil Goyal Nov. 17, 2021, 6:36 a.m. UTC | #3 
> > Can you explain the calculation for 73 bytes in the code?
> > Will it be sufficient for IPv6?
> No, it will not cover IPv6, As currently only IPv4 test cases are there in the
> app.
> But I guess it covers all the scenario which are supported by the app.
> 73 are the maximum bytes which can be added in
> AES - SHA  algo mode (41 +12 + 20 (including any padding))

Please explain and define macros appropriately so that we know what all is
Covered in 73Bytes. Or why not increase it by RTE_PKTMBUF_HEADROOM.

> 
> But it will also not cover other complex scenario like NAT-T, AH.
> I have verified this change with aes-cbc-hmac-sha1 and aes-gcm algos for
> 64,128,256, 512, 1280 bytes.
> 
>  What's your opinion on this?
> 
I believe using RTE_PKTMBUF_HEADROOM will resolve most of the scenarios.
Gagandeep Singh Nov. 17, 2021, 7 a.m. UTC | #4 
> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Wednesday, November 17, 2021 12:06 PM
> To: Gagandeep Singh <G.Singh@nxp.com>; dev@dpdk.org
> Subject: RE: [EXT] [PATCH v2] app/test-crypto-perf: fix segment size for IPsec
> operation
> 
> > > Can you explain the calculation for 73 bytes in the code?
> > > Will it be sufficient for IPv6?
> > No, it will not cover IPv6, As currently only IPv4 test cases are there in the
> > app.
> > But I guess it covers all the scenario which are supported by the app.
> > 73 are the maximum bytes which can be added in
> > AES - SHA  algo mode (41 +12 + 20 (including any padding))
> 
> Please explain and define macros appropriately so that we know what all is
> Covered in 73Bytes. Or why not increase it by RTE_PKTMBUF_HEADROOM.
> 
> >
> > But it will also not cover other complex scenario like NAT-T, AH.
> > I have verified this change with aes-cbc-hmac-sha1 and aes-gcm algos for
> > 64,128,256, 512, 1280 bytes.
> >
> >  What's your opinion on this?
> >
> I believe using RTE_PKTMBUF_HEADROOM will resolve most of the scenarios.

Agree with you, I will verify it and send the next version soon.
diff mbox series

Patch

diff --git a/app/test-crypto-perf/cperf_options.h b/app/test-crypto-perf/cperf_options.h
index 031b238b20..cdbc027b89 100644
--- a/app/test-crypto-perf/cperf_options.h
+++ b/app/test-crypto-perf/cperf_options.h
@@ -61,6 +61,7 @@ 
 #define CPERF_PMDCC_DELAY_MS	("pmd-cyclecount-delay-ms")
 
 #define MAX_LIST 32
+#define CPERF_IPSEC_OVERHEAD 73
 
 enum cperf_perf_test_type {
 	CPERF_TEST_TYPE_THROUGHPUT,
diff --git a/app/test-crypto-perf/cperf_options_parsing.c b/app/test-crypto-perf/cperf_options_parsing.c
index c244f81bbf..268f544936 100644
--- a/app/test-crypto-perf/cperf_options_parsing.c
+++ b/app/test-crypto-perf/cperf_options_parsing.c
@@ -1132,9 +1132,12 @@  cperf_options_check(struct cperf_options *options)
 	 * If segment size is not set, assume only one segment,
 	 * big enough to contain the largest buffer and the digest
 	 */
-	if (options->segment_sz == 0)
+	if (options->segment_sz == 0) {
 		options->segment_sz = options->max_buffer_size +
 				options->digest_sz;
+		if (options->op_type == CPERF_IPSEC)
+			options->segment_sz += CPERF_IPSEC_OVERHEAD;
+	}
 
 	if (options->segment_sz < options->digest_sz) {
 		RTE_LOG(ERR, USER1,