Re: [dpdk-dev] [PATCH v8 7/9] test: add event timer adapter auto-test

["Bhagavatula, Pavan" <Pavan.Bhagavatula@cavium.com>]

Hi Erik,

Few comments below,

On Thu, Mar 29, 2018 at 04:27:30PM -0500, Erik Gabriel Carrillo wrote:
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> ---
>  test/test/Makefile                   |    1 +
>  test/test/test_event_timer_adapter.c | 1837 ++++++++++++++++++++++++++++++++++
>  2 files changed, 1838 insertions(+)
>  create mode 100644 test/test/test_event_timer_adapter.c
>
<snip>
> +/* Test that adapter stops correctly. */
> +static int
> +adapter_stop(void)
> +{
> +	uint32_t evdev_service_id, adapter_service_id;
> +	struct rte_event_timer_adapter *l_adapter = NULL;
> +
Please use INTERNAL_PORT capability to determine if service core is required.

> +	/* retrieve service ids */
> +	TEST_ASSERT_SUCCESS(rte_event_dev_service_id_get(evdev,
> +			&evdev_service_id), "Failed to get event device "
> +			"service id");
> +	TEST_ASSERT_SUCCESS(rte_event_timer_adapter_service_id_get(timdev,
> +			&adapter_service_id), "Failed to get event timer "
> +			"adapter service id");
> +
> +	/* Test adapter stop */
> +	TEST_ASSERT_SUCCESS(rte_event_timer_adapter_stop(timdev),
> +			"Failed to stop event adapter");
> +
> +	TEST_ASSERT_FAIL(rte_event_timer_adapter_stop(l_adapter),
> +			"Erroneously stopped null event adapter");
> +
> +	TEST_ASSERT_SUCCESS(rte_event_timer_adapter_free(timdev),
> +			"Failed to free adapter");
> +
> +	rte_mempool_free(eventdev_test_mempool);
> +
> +	return TEST_SUCCESS;
> +}
> +
<snip>
> +stat_inc_reset_ev_enq(void)
> +{
> +	int ret, i, n;
> +	int num_evtims = MAX_TIMERS;
> +	struct rte_event_timer *evtims[num_evtims];
> +	struct rte_event evs[BATCH_SIZE];
> +	struct rte_event_timer_adapter_stats stats;
> +	const struct rte_event_timer init_tim = {
> +		.ev.op = RTE_EVENT_OP_NEW,
> +		.ev.queue_id = TEST_QUEUE_ID,
> +		.ev.sched_type = RTE_SCHED_TYPE_ATOMIC,
> +		.ev.priority = RTE_EVENT_DEV_PRIORITY_NORMAL,
> +		.ev.event_type =  RTE_EVENT_TYPE_TIMER,
> +		.state = RTE_EVENT_TIMER_NOT_ARMED,
> +		.timeout_ticks = 5,	// expire in .5 sec
> +	};
> +
> +	ret = rte_mempool_get_bulk(eventdev_test_mempool, (void **)evtims,
> +				   num_evtims);
> +	TEST_ASSERT_EQUAL(ret, 0, "Failed to get array of timer objs: ret = %d",
> +			  ret);
> +
> +	for (i = 0; i < num_evtims; i++) {
> +		*evtims[i] = init_tim;
> +		evtims[i]->ev.event_ptr = evtims[i];
> +	}
> +
> +	ret = rte_event_timer_adapter_stats_get(timdev, &stats);
> +	TEST_ASSERT_EQUAL(ret, 0, "Failed to get stats");
> +	TEST_ASSERT_EQUAL((int)stats.ev_enq_count, 0, "Stats not clear at "
> +			  "startup");
> +
> +	/* Test with the max value for the adapter */
> +	ret = rte_event_timer_arm_burst(timdev, evtims, num_evtims);
> +	TEST_ASSERT_EQUAL(ret, num_evtims,
> +			  "Failed to arm all event timers: attempted = %d, "
> +			  "succeeded = %d, rte_errno = %s",
> +			  num_evtims, ret, rte_strerror(rte_errno));
> +
> +	rte_delay_ms(1000);
> +
> +#define MAX_TRIES 1000

Please make MAX_TRIES equivalent to num_evtims, here we are trying to deq 10
events in burst and assume it to succeed but in case event dev doesn't support
burst mode event will be stuck as it will have only 1000 tries instead of 4096.

> +	int sum = 0;
> +	int tries = 0;
> +	bool done = false;
> +	while (!done) {
> +		sum += rte_event_dequeue_burst(evdev, TEST_PORT_ID, evs,
> +					       RTE_DIM(evs), 10);
> +		if (sum >= num_evtims || ++tries >= MAX_TRIES)
> +			done = true;
> +
> +		rte_delay_ms(10);
> +	}
> +
> +	TEST_ASSERT_EQUAL(sum, num_evtims, "Expected %d timer expiry events, "
> +			  "got %d", num_evtims, sum);
> +
<snip>
> +event_timer_arm_max(void)
> +{
> +	int ret, i, n;
> +	int num_evtims = MAX_TIMERS;
> +	struct rte_event_timer *evtims[num_evtims];
> +	struct rte_event evs[BATCH_SIZE];
> +	const struct rte_event_timer init_tim = {
> +		.ev.op = RTE_EVENT_OP_NEW,
> +		.ev.queue_id = TEST_QUEUE_ID,
> +		.ev.sched_type = RTE_SCHED_TYPE_ATOMIC,
> +		.ev.priority = RTE_EVENT_DEV_PRIORITY_NORMAL,
> +		.ev.event_type =  RTE_EVENT_TYPE_TIMER,
> +		.state = RTE_EVENT_TIMER_NOT_ARMED,
> +		.timeout_ticks = 5,	// expire in .5 sec
> +	};
> +
> +	ret = rte_mempool_get_bulk(eventdev_test_mempool, (void **)evtims,
> +				   num_evtims);
> +	TEST_ASSERT_EQUAL(ret, 0, "Failed to get array of timer objs: ret = %d",
> +			  ret);
> +
> +	for (i = 0; i < num_evtims; i++) {
> +		*evtims[i] = init_tim;
> +		evtims[i]->ev.event_ptr = evtims[i];
> +	}
> +
> +	/* Test with the max value for the adapter */
> +	ret = rte_event_timer_arm_burst(timdev, evtims, num_evtims);
> +	TEST_ASSERT_EQUAL(ret, num_evtims,
> +			  "Failed to arm all event timers: attempted = %d, "
> +			  "succeeded = %d, rte_errno = %s",
> +			  num_evtims, ret, rte_strerror(rte_errno));
> +
> +	rte_delay_ms(1000);
> +
> +#define MAX_TRIES 1000

Same as above.

> +	int sum = 0;
> +	int tries = 0;
> +	bool done = false;

<snip>

> +static int
> +adapter_create_max(void)
> +{
> +	int i;
> +	uint32_t svc_start_count, svc_end_count;
> +	struct rte_event_timer_adapter *adapters[
> +					RTE_EVENT_TIMER_ADAPTER_NUM_MAX + 1];
> +
> +	struct rte_event_timer_adapter_conf conf = {
> +		.event_dev_id = evdev,
> +		// timer_adapter_id set in loop
> +		.clk_src = RTE_EVENT_TIMER_ADAPTER_CPU_CLK,
> +		.timer_tick_ns = NSECPERSEC / 10,
> +		.max_tmo_ns = 180 * NSECPERSEC,
> +		.nb_timers = MAX_TIMERS,
> +		.flags = 0,
> +	};
> +
> +	svc_start_count = rte_service_get_count();
> +
> +	/* This test expects that there are sufficient service IDs available
> +	 * to be allocated. I.e., RTE_EVENT_TIMER_ADAPTER_NUM_MAX may need to
> +	 * be less than RTE_SERVICE_NUM_MAX if anything else uses a service
> +	 * (the SW event device, for example).
> +	 */
Same as above service use need to be dependent on INTERNAL_PORT capability.

Also, in software event dev case this can be a valid test but in case of a hw
event dev RTE_EVENT_TIMER_ADAPTER_NUM_MAX number of event devicesmight not be
binded to dpdk. Either we need to provide a API to check how many event devices
are supported or use eventdev_timer_adapter_caps_get_t to get the number.

Thoughts?

> +	for (i = 0; i < RTE_EVENT_TIMER_ADAPTER_NUM_MAX; i++) {
> +		conf.timer_adapter_id = i;
> +		adapters[i] = rte_event_timer_adapter_create_ext(&conf,
> +				test_port_conf_cb, NULL);
> +		TEST_ASSERT_NOT_NULL(adapters[i], "Failed to create adapter "
> +				"%d", i);
> +	}
> +
<snip>

Thanks,
Pavan.