From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id A8043326C for ; Fri, 18 May 2018 07:19:46 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 May 2018 22:19:44 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,413,1520924400"; d="scan'208,217";a="229568118" Received: from nikhilr-mobl.amr.corp.intel.com (HELO [10.106.138.3]) ([10.106.138.3]) by fmsmga005.fm.intel.com with ESMTP; 17 May 2018 22:19:42 -0700 To: Vipin Varghese , dev@dpdk.org, deepak.k.jain@intel.com References: <1526552256-56523-1-git-send-email-vipin.varghese@intel.com> From: "Rao, Nikhil" Cc: nikhil.rao@intel.com Message-ID: <875b1678-b7bc-ca2e-b11f-c9f9bc33b9aa@intel.com> Date: Fri, 18 May 2018 10:49:41 +0530 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <1526552256-56523-1-git-send-email-vipin.varghese@intel.com> Content-Language: en-US Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH] test/eventdev: add multi port test to suite X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 May 2018 05:19:47 -0000 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 > --- > 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