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.vargh...@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....@intel.com>

Reply via email to