Re: [dpdk-dev] [EXT] Re: [PATCH v4 2/5] test/crypto: add combined mode tests

[Anoob Joseph <anoobj@marvell.com>]

Hi Hemant,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Hemant Agrawal <hemant.agrawal@oss.nxp.com>
> Sent: Friday, September 24, 2021 12:54 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal <gakhil@marvell.com>;
> Declan Doherty <declan.doherty@intel.com>; Fan Zhang
> <roy.fan.zhang@intel.com>; Konstantin Ananyev
> <konstantin.ananyev@intel.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Archana Muniganti
> <marchana@marvell.com>; Tejasree Kondoj <ktejasree@marvell.com>; Hemant
> Agrawal <hemant.agrawal@nxp.com>; Radu Nicolau
> <radu.nicolau@intel.com>; Ciara Power <ciara.power@intel.com>; Gagandeep
> Singh <g.singh@nxp.com>; dev@dpdk.org
> Subject: [EXT] Re: [dpdk-dev] [PATCH v4 2/5] test/crypto: add combined mode
> tests
> 
> External Email
> 
> ----------------------------------------------------------------------
> HI Anoob,
> 
> On 9/17/2021 6:45 PM, Anoob Joseph wrote:
> > Add framework to test IPsec features with all supported combinations
> > of ciphers.
> >
> > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > Signed-off-by: Tejasree Kondoj <ktejasree@marvell.com>
> >
> > ---
> >   app/test/test_cryptodev.c                |  73 +++++++++++++++++++--
> >   app/test/test_cryptodev_security_ipsec.c | 107
> +++++++++++++++++++++++++++++--
> >   app/test/test_cryptodev_security_ipsec.h |  52 ++++++++++++++-
> >   3 files changed, 223 insertions(+), 9 deletions(-)
> >
> > diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
> > index 9c7875c..7fd246e 100644
> > --- a/app/test/test_cryptodev.c
> > +++ b/app/test/test_cryptodev.c
> > @@ -8904,7 +8904,8 @@ static int
> >   test_ipsec_proto_process(const struct ipsec_test_data td[],
> >   			 struct ipsec_test_data res_d[],
> >   			 int nb_td,
> > -			 bool silent)
> > +			 bool silent,
> > +			 const struct ipsec_test_flags *flags)
> >   {
> >   	struct crypto_testsuite_params *ts_params = &testsuite_params;
> >   	struct crypto_unittest_params *ut_params = &unittest_params; @@
> > -9021,7 +9022,7 @@ test_ipsec_proto_process(const struct ipsec_test_data
> td[],
> >   		/* Process crypto operation */
> >   		process_crypto_request(dev_id, ut_params->op);
> >
> > -		ret = test_ipsec_status_check(ut_params->op, dir);
> > +		ret = test_ipsec_status_check(ut_params->op, flags, dir);
> >   		if (ret != TEST_SUCCESS)
> >   			goto crypto_op_free;
> >
> > @@ -9029,7 +9030,7 @@ test_ipsec_proto_process(const struct
> ipsec_test_data td[],
> >   			res_d_tmp = &res_d[i];
> >
> >   		ret = test_ipsec_post_process(ut_params->ibuf, &td[i],
> > -					      res_d_tmp, silent);
> > +					      res_d_tmp, silent, flags);
> >   		if (ret != TEST_SUCCESS)
> >   			goto crypto_op_free;
> >
> > @@ -9057,11 +9058,71 @@ test_ipsec_proto_process(const struct
> ipsec_test_data td[],
> >   static int
> >   test_ipsec_proto_known_vec_inb(const void *td_outb)
> >   {
> > +	struct ipsec_test_flags flags;
> >   	struct ipsec_test_data td_inb;
> >
> > +	memset(&flags, 0, sizeof(flags));
> > +
> >   	test_ipsec_td_in_from_out(td_outb, &td_inb);
> >
> > -	return test_ipsec_proto_process(&td_inb, NULL, 1, false);
> > +	return test_ipsec_proto_process(&td_inb, NULL, 1, false, &flags);
> 
> In most places, you have hardcoded this silent flag - this is for debug prints.
> 
> Will you please driver it from some macro or variable or log-level, where it can
> be changed easily.
> 

[Anoob] Currently, for known vector tests, we have "silent" set as false to have more verbose output by default. For combined mode, since we attempt multiple ciphers, there would be lot of prints. Hence we kept "silent" flag as true to disable prints for such cases. The idea is, if a developer wants to quickly analyze one combined mode test case, he can do so by changing silent flag for just that. Enabling it universally could dump more info than desired. 

Making it a global flag would mean, we would have same kind of prints with known vector & combined mode (which attempts combinations). Do you think a global setting is better? If yes, what do you recommend? A custom flag or RTE_LOG?