Re: [PATCH v2 04/16] test/bbdev: add timeout for latency tests

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 2/24/23 17:59, Vargas, Hernan wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Thursday, February 23, 2023 2:32 AM
>> To: Vargas, Hernan <hernan.vargas@intel.com>; dev@dpdk.org;
>> gakhil@marvell.com; Rix, Tom <trix@redhat.com>
>> Cc: Chautru, Nicolas <nicolas.chautru@intel.com>; Zhang, Qi Z
>> <qi.z.zhang@intel.com>
>> Subject: Re: [PATCH v2 04/16] test/bbdev: add timeout for latency tests
>>
>>
>>
>> On 2/22/23 22:13, Vargas, Hernan wrote:
>>> Hi Maxime,
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Sent: Monday, February 20, 2023 10:33 AM
>>>> To: Vargas, Hernan <hernan.vargas@intel.com>; dev@dpdk.org;
>>>> gakhil@marvell.com; Rix, Tom <trix@redhat.com>
>>>> Cc: Chautru, Nicolas <nicolas.chautru@intel.com>; Zhang, Qi Z
>>>> <qi.z.zhang@intel.com>
>>>> Subject: Re: [PATCH v2 04/16] test/bbdev: add timeout for latency
>>>> tests
>>>>
>>>>
>>>>
>>>> On 2/15/23 18:09, Hernan Vargas wrote:
>>>>> Add a timeout to force exit the latency tests in case dequeue never
>>>>> happens.
>>>>>
>>>>> Signed-off-by: Hernan Vargas <hernan.vargas@intel.com>
>>>>> ---
>>>>>     app/test-bbdev/test_bbdev_perf.c | 24 +++++++++++++++++++-----
>>>>>     1 file changed, 19 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/app/test-bbdev/test_bbdev_perf.c
>>>>> b/app/test-bbdev/test_bbdev_perf.c
>>>>> index 19b9a5b119..dede0f900e 100644
>>>>> --- a/app/test-bbdev/test_bbdev_perf.c
>>>>> +++ b/app/test-bbdev/test_bbdev_perf.c
>>>>> @@ -26,6 +26,7 @@
>>>>>
>>>>>     #define MAX_QUEUES RTE_MAX_LCORE
>>>>>     #define TEST_REPETITIONS 100
>>>>> +#define TIME_OUT_POLL 1e8
>>>>>     #define WAIT_OFFLOAD_US 1000
>>>>>
>>>>>     #ifdef RTE_BASEBAND_FPGA_LTE_FEC
>>>>> @@ -4546,6 +4547,7 @@ latency_test_ldpc_dec(struct rte_mempool
>>>>> *mempool,
>>>>>
>>>>>     	for (i = 0, dequeued = 0; dequeued < num_to_process; ++i) {
>>>>>     		uint16_t enq = 0, deq = 0;
>>>>> +		uint32_t time_out = 0;
>>>>>     		bool first_time = true;
>>>>>     		last_time = 0;
>>>>>
>>>>> @@ -4597,7 +4599,8 @@ latency_test_ldpc_dec(struct rte_mempool
>>>> *mempool,
>>>>>     				last_time = rte_rdtsc_precise() - start_time;
>>>>>     				first_time = false;
>>>>>     			}
>>>>> -		} while (unlikely(burst_sz != deq));
>>>>> +			time_out++;
>>>>> +		} while ((burst_sz != deq) && (time_out < TIME_OUT_POLL));
>>>>>
>>>>>     		*max_time = RTE_MAX(*max_time, last_time);
>>>>>     		*min_time = RTE_MIN(*min_time, last_time); @@ -4606,7
>>>> +4609,12 @@
>>>>> latency_test_ldpc_dec(struct rte_mempool *mempool,
>>>>>     		if (extDdr)
>>>>>     			retrieve_harq_ddr(dev_id, queue_id, ops_enq,
>>>> burst_sz);
>>>>>
>>>>> -		if (test_vector.op_type != RTE_BBDEV_OP_NONE) {
>>>>> +		if (burst_sz != deq) {
>>>>> +			struct rte_bbdev_info info;
>>>>> +			ret = TEST_FAILED;
>>>>> +			rte_bbdev_info_get(dev_id, &info);
>>>>
>>>> What is the point of calling rte_bbdev_info_get() here and below?
>>>> info is not used afterwards.
>>>
>>> The reason for calling this function is to check the device status and if there
>> is something wrong the PMD would display it to standard output.
>>
>> What kind of info exactly, I don't see much meaningful logs in
>> rte_bbdev_info_get() except printing error if dev_info == NULL.
> 
> rte_bbdev_info_get() calls the device's info_get function that is specified in the PMD.
> For example, for ACC100, acc100_dev_info_get() gets called to check the device status.

Ok, I looked at this function and it might be more relevant to mention 
vrb1 than acc100, because acc100 does not support device status:

	/* Check the status of device */
	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;

Also, this dev_info->device_status field is set by all the drivers but
never read (neither by the driver, nor library nor apps).

So if this is the only reason rte_bbdev_info_get() is called, that is
quite useless. Am I missing someting?

Thanks,
Maxime

> Thanks,
> Hernan
>>
>> Regards,
>> Maxime
>>
>>>>
>>>>> +			TEST_ASSERT_SUCCESS(ret, "Dequeue timeout!");
>>>>> +		} else if (test_vector.op_type != RTE_BBDEV_OP_NONE) {
>>>>>     			ret = validate_ldpc_dec_op(ops_deq, burst_sz,
>>>> ref_op,
>>>>>     					vector_mask);
>>>>>     			TEST_ASSERT_SUCCESS(ret, "Validation failed!"); @@
>>>> -4632,6
>>>>> +4640,7 @@ latency_test_enc(struct rte_mempool *mempool,
>>>>>
>>>>>     	for (i = 0, dequeued = 0; dequeued < num_to_process; ++i) {
>>>>>     		uint16_t enq = 0, deq = 0;
>>>>> +		uint32_t time_out = 0;
>>>>>     		bool first_time = true;
>>>>>     		last_time = 0;
>>>>>
>>>>> @@ -4667,13 +4676,18 @@ latency_test_enc(struct rte_mempool
>>>> *mempool,
>>>>>     				last_time += rte_rdtsc_precise() - start_time;
>>>>>     				first_time = false;
>>>>>     			}
>>>>> -		} while (unlikely(burst_sz != deq));
>>>>> +			time_out++;
>>>>> +		} while ((burst_sz != deq) && (time_out < TIME_OUT_POLL));
>>>>>
>>>>>     		*max_time = RTE_MAX(*max_time, last_time);
>>>>>     		*min_time = RTE_MIN(*min_time, last_time);
>>>>>     		*total_time += last_time;
>>>>> -
>>>>> -		if (test_vector.op_type != RTE_BBDEV_OP_NONE) {
>>>>> +		if (burst_sz != deq) {
>>>>> +			struct rte_bbdev_info info;
>>>>> +			ret = TEST_FAILED;
>>>>> +			rte_bbdev_info_get(dev_id, &info);
>>>>
>>>> Same here.
>>>>
>>>>> +			TEST_ASSERT_SUCCESS(ret, "Dequeue timeout!");
>>>>> +		} else if (test_vector.op_type != RTE_BBDEV_OP_NONE) {
>>>>>     			ret = validate_enc_op(ops_deq, burst_sz, ref_op);
>>>>>     			TEST_ASSERT_SUCCESS(ret, "Validation failed!");
>>>>>     		}
>>>
>