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

  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
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(-)
  

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>
  

Hi Nikhil,

Thanks for the inputs, I have shared the V2 with suggested changes. As for

<snipped>




+        adapter_free();
Is it necessary to call adapter_free() above ?

<Snipped>

With the changes suggested above
Acked-by: Nikhil Rao <nikhil.rao@intel.com><mailto:nikhil.rao@intel.com>


Yes it not required by current test sequence. This is added for safety, in case in previous free is incomplete or interrupted.
  

Hi Vipin,

Thanks for posting V2.

On 5/21/2018 4:30 PM, Varghese, Vipin wrote:

> Hi Nikhil,
>
> Thanks for the inputs, I have shared the V2 with suggested changes. As 
> for**
>
> <snipped>
>
>     +        adapter_free();
>
> Is it necessary to call adapter_free() above ?
>
> <Snipped>
>
> With the changes suggested above
> Acked-by: Nikhil Rao <nikhil.rao@intel.com> <mailto:nikhil.rao@intel.com>
>
> Yes it not required by current test sequence.
>

> This is added for safety, in case in previous free is incomplete or 
> interrupted.
>
I don't see a case where the previous free can be incomplete or interrupted.

Nikhil
  

Thanks, send v3 with the suggested changes.

From: Rao, Nikhil

Sent: Monday, May 21, 2018 5:09 PM
To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org; Jain, Deepak K <deepak.k.jain@intel.com>
Cc: Rao, Nikhil <nikhil.rao@intel.com>
Subject: Re: [PATCH] test/eventdev: add multi port test to suite


Hi Vipin,

Thanks for posting V2.

On 5/21/2018 4:30 PM, Varghese, Vipin wrote:
Hi Nikhil,

Thanks for the inputs, I have shared the V2 with suggested changes. As for

<snipped>




+        adapter_free();
Is it necessary to call adapter_free() above ?

<Snipped>

With the changes suggested above
Acked-by: Nikhil Rao <nikhil.rao@intel.com><mailto:nikhil.rao@intel.com>


Yes it not required by current test sequence.


This is added for safety, in case in previous free is incomplete or interrupted.
I don't see a case where the previous free can be incomplete or interrupted.

Nikhil