Re: [dpdk-dev] [PATCH] test/eventdev: add multi port test to suite

["Rao, Nikhil" <nikhil.rao@intel.com>]

Hi Vipin,

Thanks for the patch, looks good overall, a few comments below

On 5/17/2018 3:47 PM, Vipin Varghese wrote:
> Add a new test to enhance the test suite, to allow multiple eth
> ports rx queue to be added to rx bridge adapter. Update the test
to allow rx queues from multiple ports to be added to the rx adapter
> function to reflect change of port index from 8 to 16 bit
>
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> ---
>   test/test/test_event_eth_rx_adapter.c | 87 +++++++++++++++++++++++++++++++++--
>   1 file changed, 84 insertions(+), 3 deletions(-)
>
> diff --git a/test/test/test_event_eth_rx_adapter.c b/test/test/test_event_eth_rx_adapter.c
> index ab55398..0af95be 100644
> --- a/test/test/test_event_eth_rx_adapter.c
> +++ b/test/test/test_event_eth_rx_adapter.c
> @@ -30,7 +30,7 @@ struct event_eth_rx_adapter_test_params {
>   static struct event_eth_rx_adapter_test_params default_params;
>   
>   static inline int
> -port_init(uint8_t port, struct rte_mempool *mp)
> +port_init(uint16_t port, struct rte_mempool *mp)
>   {
>   	static const struct rte_eth_conf port_conf_default = {
>   		.rxmode = {
> @@ -107,15 +107,21 @@ port_init(uint8_t port, struct rte_mempool *mp)
>   static int
>   init_ports(int num_ports)
>   {
> -	uint8_t portid;
> +	uint16_t portid;
>   	int retval;
>   
> -	default_params.mp = rte_pktmbuf_pool_create("packet_pool",
> +	struct rte_mempool *ptr = rte_mempool_lookup("packet_pool");
> +
> +	if (ptr == NULL)
> +		default_params.mp = rte_pktmbuf_pool_create("packet_pool",
>   						NB_MBUFS,
>   						MBUF_CACHE_SIZE,
>   						MBUF_PRIV_SIZE,
>   						RTE_MBUF_DEFAULT_BUF_SIZE,
>   						rte_socket_id());
> +	else
> +		default_params.mp = ptr;
> +
>   	if (!default_params.mp)
>   		return -ENOMEM;
>   
> @@ -333,6 +339,80 @@ adapter_queue_add_del(void)
>   }
>   
>   static int
> +adapter_multi_eth_add_del(void)
> +{
> +	int err;
> +	struct rte_event ev;
> +	uint32_t cap;
> +
> +	char tun_drv_name[20] = "net_null";
The tun_drv_name is unnecessary, You could use "net_null" in the sprintf 
call below.
> +	uint16_t port_index, tun_id = 0;
> +	char tun_driver_name[50] = "\0";
Initialization isn't required.
> +
> +	struct rte_event_eth_rx_adapter_queue_conf queue_config;
> +
> +	err = rte_event_eth_rx_adapter_caps_get(TEST_DEV_ID, TEST_ETHDEV_ID,
> +			&cap);
> +	TEST_ASSERT(err == 0, "Expected 0 got %d", err);
> +
> +	ev.queue_id = 0;
> +	ev.sched_type = RTE_SCHED_TYPE_ATOMIC;
> +	ev.priority = 0;
> +
> +	queue_config.rx_queue_flags = 0;

> +	if (cap & RTE_EVENT_ETH_RX_ADAPTER_CAP_OVERRIDE_FLOW_ID) {
> +		ev.flow_id = 1;
> +		queue_config.rx_queue_flags =
> +		RTE_EVENT_ETH_RX_ADAPTER_QUEUE_FLOW_ID_VALID;
> +	}
The cap check is unnecessary, further, it is based on TEST_ETHDEV_ID, 
this flag may not be set for the vdevs below.

> +	queue_config.ev = ev;
> +	queue_config.servicing_weight = 1;
> +
> +	/* stop eth devices for existing */
> +	port_index = 0;
> +	for (; port_index < rte_eth_dev_count_total(); port_index += 1)
> +		rte_eth_dev_stop(port_index);
> +
> +	/* add the max port for rx_adapter */
> +	port_index = rte_eth_dev_count_total();
> +	for (; port_index < RTE_MAX_ETHPORTS; port_index += 1) {
> +		sprintf(tun_driver_name, "%s%u", tun_drv_name, tun_id);
> +		err = rte_vdev_init(tun_driver_name, NULL);
> +		TEST_ASSERT(err == 0, "Failed driver %s got %d",
> +		tun_driver_name, err);
> +		tun_id += 1;
> +	}
> +
> +	err = init_ports(rte_eth_dev_count_total());
> +	TEST_ASSERT(err == 0, "Port initialization failed err %d\n", err);
> +

> +	/* to work around the issue in rte_event_eth_rx_adapter_create_ext */
replace "issue" with "rx adapter does not support adding devices created 
after adapter creation"
> +	adapter_free();
Is it necessary to call adapter_free() above ?

> +	adapter_create();
> +
> +	/* eth_rx_adapter_queue_add for n ports */
> +	port_index = 0;
> +	for (; port_index < rte_eth_dev_count_total(); port_index += 1) {
> +		err = rte_event_eth_rx_adapter_queue_add(TEST_INST_ID,
> +				port_index, 0,
> +				&queue_config);
> +		TEST_ASSERT(err == 0, "Expected 0 got %d", err);
> +	}
> +
> +	/* eth_rx_adapter_queue_del n ports */
> +	port_index = 0;
> +	for (; port_index < rte_eth_dev_count_total(); port_index += 1) {
> +		err = rte_event_eth_rx_adapter_queue_del(TEST_INST_ID,
> +				port_index, 0);
> +		TEST_ASSERT(err == 0, "Expected 0 got %d", err);
> +	}
> +
> +	adapter_free();
> +
> +	return TEST_SUCCESS;
> +}
> +
> +static int
>   adapter_start_stop(void)
>   {
>   	int err;
> @@ -410,6 +490,7 @@ static struct unit_test_suite service_tests  = {
>   		TEST_CASE_ST(NULL, NULL, adapter_create_free),
>   		TEST_CASE_ST(adapter_create, adapter_free,
>   					adapter_queue_add_del),
> +		TEST_CASE_ST(NULL, NULL, adapter_multi_eth_add_del),
>   		TEST_CASE_ST(adapter_create, adapter_free, adapter_start_stop),
>   		TEST_CASE_ST(adapter_create, adapter_free, adapter_stats),
>   		TEST_CASES_END() /**< NULL terminate unit test array */
With the changes suggested above
Acked-by: Nikhil Rao <nikhil.rao@intel.com>