[dpdk-dev] [EXT] [PATCH v4 3/6] app/test-compress-perf: add verification test case

[Tomasz Jozwiak]

Hi Shally,

Thx for the review.

My comments below:

>
>> -----Original Message-----
>> From: Tomasz Jozwiak <tjozwiakgm at gmail.com>
>> Sent: Friday, June 28, 2019 3:56 AM
>> To: dev at dpdk.org; fiona.trahe at intel.com; tjozwiakgm at gmail.com; Shally
>> Verma <shallyv at marvell.com>; arturx.trybula at intel.com
>> Subject: [EXT] [PATCH v4 3/6] app/test-compress-perf: add verification test
>> case
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> From: Tomasz Jozwiak <tomaszx.jozwiak at intel.com>
> ...
>
>> diff --git a/app/test-compress-perf/comp_perf_test_verify.c b/app/test-
>> compress-perf/comp_perf_test_verify.c
>> index 28a0fe8..c2aab70 100644
>> --- a/app/test-compress-perf/comp_perf_test_verify.c
>> +++ b/app/test-compress-perf/comp_perf_test_verify.c
>> @@ -8,14 +8,48 @@
>>   #include <rte_compressdev.h>
>>
>>   #include "comp_perf_test_verify.h"
>> +#include "comp_perf_test_common.h"
>> +
>> +void
>> +cperf_verify_test_destructor(void *arg) {
>> +	if (arg) {
>> +		comp_perf_free_memory(&((struct cperf_verify_ctx *)arg)-
>>> mem);
>> +		rte_free(arg);
>> +	}
>> +}
>> +
>> +void *
>> +cperf_verify_test_constructor(uint8_t dev_id, uint16_t qp_id,
>> +		struct comp_test_data *options)
>> +{
>> +	struct cperf_verify_ctx *ctx = NULL;
>> +
>> +	ctx = rte_malloc(NULL, sizeof(struct cperf_verify_ctx), 0);
>> +
> Better just return from here

[Tomek]. Yes you right. we wasn't able to allocate 'cperf_verify_ctx 
struct',

so we don't need to call destructor here. I assume there's the same issue

in benchmark patch - will align both in V5. Thx.


>
>> +	if (ctx != NULL) {
>> +		ctx->mem.dev_id = dev_id;
>> +		ctx->mem.qp_id = qp_id;
>> +		ctx->options = options;
>> +
>> +		if (!comp_perf_allocate_memory(ctx->options, &ctx->mem)
>> &&
>> +		    !prepare_bufs(ctx->options, &ctx->mem))
>> +			return ctx;
> What if condition fails on comp_per_allocate_memory(), then it will go to verify_test_destructor(), so comp_perf_free_memory() check if mem != NULL before calling actual free?

[Tomek] I mean it's ok. Please take in to account that we was able to 
allocate 'cperf_verify_ctx struct' - cause

ctx != NULL here. that means 'mem struct' inside 'cperf_verify_ctx 
struct' exists for sure:

struct cperf_verify_ctx {
*struct cperf_mem_resources mem;*
     struct comp_test_data *options;

     int silent;
     size_t comp_data_sz;
     size_t decomp_data_sz;
     double ratio;
};

and all fields inside 'struct cperf_mem_resources mem' are zeroed.

We don't need to check mem != NULL before free, because in this place 
mem != NULL for sure. Also it's ok to call 'rte_free',

'rte_mempool_free' and 'rte_pktmbuf_free' with NULL ptr.

as a argument because the check is inside all of these functions.




Thx for the comments.


--

Tomek