Re: [dpdk-dev] [PATCH v2 4/4] eventdev: add auto test for eth Tx adapter

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

On 8/17/2018 5:25 PM, Pavan Nikhilesh wrote:
> Hi Nikhil,
> 
> Few comments inline.
> 
> Nit: Please use --in-reply-to while sending next versions.
>       (https://core.dpdk.org/contribute/)
> 
> Thanks,
> Pavan
> 
> On Fri, Aug 17, 2018 at 09:50:52AM +0530, Nikhil Rao wrote:
>>
>> This patch adds tests for the eth Tx adapter APIs. It also
>> tests the data path for the rte_service function based
>> implementation of the APIs.
>>
>> Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
>> ---
>>   test/test/test_event_eth_tx_adapter.c | 676 ++++++++++++++++++++++++++++++++++
>>   MAINTAINERS                           |   1 +
>>   test/test/Makefile                    |   1 +
>>   test/test/meson.build                 |   2 +
>>   4 files changed, 680 insertions(+)
>>   create mode 100644 test/test/test_event_eth_tx_adapter.c
>>
>> diff --git a/test/test/test_event_eth_tx_adapter.c b/test/test/test_event_eth_tx_adapter.c
>> new file mode 100644
>> index 0000000..2dc487b
>> --- /dev/null
>> +++ b/test/test/test_event_eth_tx_adapter.c
> 
> <snip>
> 
>> +static int
>> +testsuite_setup(void)
>> +{
>> +       int err;
>> +       uint8_t count;
>> +       struct rte_event_dev_info dev_info;
>> +       uint8_t priority;
>> +       uint8_t queue_id;
>> +
>> +       count = rte_event_dev_count();
>> +       if (!count) {
>> +               printf("Failed to find a valid event device,"
>> +                       " testing with event_sw0 device\n");
>> +               rte_vdev_init("event_sw0", NULL);
>> +               event_dev_delete = 1;
>> +       }
>> +
>> +       struct rte_event_dev_config config = {
>> +                       .nb_event_queues = 1,
>> +                       .nb_event_ports = 1,
>> +       };
>> +
>> +       struct rte_event_queue_conf wkr_q_conf = {
>> +                       .schedule_type = RTE_SCHED_TYPE_ORDERED,
>> +                       .priority = RTE_EVENT_DEV_PRIORITY_NORMAL,
>> +                       .nb_atomic_flows = 1024,
>> +                       .nb_atomic_order_sequences = 1024,
>> +       };
>> +
>> +       err = rte_event_dev_info_get(TEST_DEV_ID, &dev_info);
>> +       config.nb_event_queue_flows = dev_info.max_event_queue_flows;
>> +       config.nb_event_port_dequeue_depth =
>> +                       dev_info.max_event_port_dequeue_depth;
>> +       config.nb_event_port_enqueue_depth =
>> +                       dev_info.max_event_port_enqueue_depth;
>> +       config.nb_events_limit =
>> +                       dev_info.max_num_events;
>> +
>> +       rte_log_set_level(0, RTE_LOG_DEBUG);
>> +       err = rte_event_dev_configure(TEST_DEV_ID, &config);
>> +       TEST_ASSERT(err == 0, "Event device initialization failed err %d\n",
>> +                       err);
>> +       if (rte_event_queue_setup(TEST_DEV_ID, 0, &wkr_q_conf) < 0) {
>> +               printf("%d: error creating qid %d\n", __LINE__, 0);
>> +               return -1;
>> +       }
>> +       if (rte_event_port_setup(TEST_DEV_ID, 0, NULL) < 0) {
>> +               printf("Error setting up port %d\n", 0);
>> +               return -1;
>> +       }
>> +
>> +       priority = RTE_EVENT_DEV_PRIORITY_LOWEST;
> 
> queue_id is uninitilized here, garbage value might be passed.

Ok.
> 
>> +       if (rte_event_port_link(TEST_DEV_ID, 0, &queue_id, &priority, 1) != 1) {
>> +               printf("Error linking port\n");
>> +               return -1;
>> +       }
>> +
>> +       err = init_ports();
>> +       TEST_ASSERT(err == 0, "Port initialization failed err %d\n", err);
>> +
>> +       err = rte_event_eth_tx_adapter_caps_get(TEST_DEV_ID, TEST_ETHDEV_ID,
>> +                                               &default_params.caps);
>> +       TEST_ASSERT(err == 0, "Failed to get adapter cap err %d\n",
>> +                       err);
>> +
>> +       return err;
>> +}
>> +
> 
> <snip>
> 
>> +static uint32_t eid, tid;
>> +
>> +static int
>> +tx_adapter_single(uint16_t port, uint16_t tx_queue_id,
>> +               struct rte_mbuf *m, uint8_t qid,
>> +               uint8_t sched_type)
>> +{
>> +       struct rte_event event;
>> +       struct rte_mbuf *r;
>> +       int ret;
>> +       unsigned int l;
>> +
>> +       event.queue_id = qid;
> 
> Set event_type to RTE_EVENT_TYPE_CPU so that underlying drivers don't mess up the
> packet.
Ok.

> 
> <snip>
> 
>> +
>> +static int
>> +tx_adapter_service(void)
>> +{
>> +       struct rte_event_eth_tx_adapter_stats stats;
>> +       uint32_t i;
>> +       int err;
>> +       uint8_t ev_port, ev_qid;
>> +       struct rte_mbuf  bufs[RING_SIZE];
>> +       struct rte_mbuf *pbufs[RING_SIZE];
>> +       struct rte_event_dev_info dev_info;
>> +       struct rte_event_dev_config dev_conf;
>> +       struct rte_event_queue_conf qconf;
>> +       uint32_t qcnt, pcnt;
>> +       uint16_t q;
>> +       int internal_port;
>> +
>> +       err = rte_event_eth_tx_adapter_caps_get(TEST_DEV_ID, TEST_ETHDEV_ID,
>> +                                               &default_params.caps);
>> +       TEST_ASSERT(err == 0, "Failed to get adapter cap err %d\n",
>> +                       err);
>> +
>> +       internal_port = !!(default_params.caps &
>> +                       RTE_EVENT_ETH_TX_ADAPTER_CAP_INTERNAL_PORT);
>> +
>> +       if (internal_port)
>> +               return TEST_SUCCESS;
>> +
>> +       err = rte_event_eth_tx_adapter_queue_add(TEST_INST_ID, TEST_ETHDEV_ID,
>> +                                               -1);
>> +       TEST_ASSERT(err == 0, "Expected 0 got %d", err);
>> +
>> +       err = rte_event_eth_tx_adapter_event_port_get(TEST_INST_ID,
>> +                                               &ev_port);
>> +       TEST_ASSERT_SUCCESS(err, "Failed to get event port %d", err);
>> +
>> +       err = rte_event_dev_attr_get(TEST_DEV_ID, RTE_EVENT_DEV_ATTR_PORT_COUNT,
>> +                                       &pcnt);
>> +       TEST_ASSERT_SUCCESS(err, "Port count get failed");
>> +
>> +       err = rte_event_dev_attr_get(TEST_DEV_ID,
>> +                               RTE_EVENT_DEV_ATTR_QUEUE_COUNT, &qcnt);
>> +       TEST_ASSERT_SUCCESS(err, "Queue count get failed");
>> +
>> +       err = rte_event_dev_info_get(TEST_DEV_ID, &dev_info);
>> +       TEST_ASSERT_SUCCESS(err, "Dev info failed");
>> +
> 
> memset dev_config to avoid invalid values.
> 
Ok.

>> +       dev_conf.nb_event_queue_flows = dev_info.max_event_queue_flows;
>> +       dev_conf.nb_event_port_dequeue_depth =
>> +                       dev_info.max_event_port_dequeue_depth;
>> +       dev_conf.nb_event_port_enqueue_depth =
>> +                       dev_info.max_event_port_enqueue_depth;
>> +       dev_conf.nb_events_limit =
>> +                       dev_info.max_num_events;
>> +       dev_conf.nb_event_queues = qcnt + 1;
>> +       dev_conf.nb_event_ports = pcnt;
>> +       err = rte_event_dev_configure(TEST_DEV_ID, &dev_conf);
>> +       TEST_ASSERT(err == 0, "Event device initialization failed err %d\n",
>> +                       err);
>> +
>> +       ev_qid = qcnt;
>> +       qconf.nb_atomic_flows = dev_info.max_event_queue_flows;
>> +       qconf.nb_atomic_order_sequences = 32;
>> +       qconf.schedule_type = RTE_SCHED_TYPE_ATOMIC;
>> +       qconf.priority = RTE_EVENT_DEV_PRIORITY_HIGHEST;
>> +       qconf.event_queue_cfg = RTE_EVENT_QUEUE_CFG_SINGLE_LINK;
>> +       err = rte_event_queue_setup(TEST_DEV_ID, ev_qid, &qconf);
>> +       TEST_ASSERT_SUCCESS(err, "Failed to setup queue %u", ev_qid);
> 
> On reconfigure, setup all the ports and queues so that the newly configured
> values are seen by them.

Is that required since dev_conf matches dev_info except for the number 
of queues ?

> 
>> +
>> +       err = rte_event_port_link(TEST_DEV_ID, ev_port, &ev_qid, NULL, 1);
>> +       TEST_ASSERT(err == 1, "Failed to link queue port %u",
>> +                   ev_port);
>> +
>> +       err = rte_event_eth_tx_adapter_start(TEST_INST_ID);
>> +       TEST_ASSERT(err == 0, "Expected 0 got %d", err);
>> +
>> +       err = rte_event_dev_service_id_get(0, &eid);
>> +       TEST_ASSERT(err == 0, "Expected 0 got %d", err);
> 
> An event device might not be needing a service core, check the capabilities
> before requesting service id above.
>

The internal_port capability is checked at the beginning of the 
function, if its not set, I think its Ok to assume that we need a 
service core ?

Thanks,
Nikhil