Re: [dpdk-dev] [PATCH v4 3/4] eventdev: Add eventdev ethernet Rx adapter

[Rao, Nikhil]

On 10/3/2017 7:22 PM, Jerin Jacob wrote:
-----Original Message-----
Date: Sun, 24 Sep 2017 23:46:51 +0530
From: "Rao, Nikhil" <nikhil....@intel.com>
To: Jerin Jacob <jerin.ja...@caviumnetworks.com>
CC: bruce.richard...@intel.com, gage.e...@intel.com, dev@dpdk.org,
  tho...@monjalon.net, harry.van.haa...@intel.com, hemant.agra...@nxp.com,
  nipun.gu...@nxp.com, narender.vang...@intel.com,
  erik.g.carri...@intel.com, abhinandan.guj...@intel.com,
  santosh.shu...@caviumnetworks.com
Subject: Re: [PATCH v4 3/4] eventdev: Add eventdev ethernet Rx adapter
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
  Thunderbird/52.3.0

OK, Thanks for the detailed review. Will add the programmer guide to RC1.

OK. Thanks.





Yes, if create() and queue_add() are called from different processes, it
wouldn't work.

+
+static uint8_t default_rss_key[] = {
+       0x6d, 0x5a, 0x56, 0xda, 0x25, 0x5b, 0x0e, 0xc2,
+       0x41, 0x67, 0x25, 0x3d, 0x43, 0xa3, 0x8f, 0xb0,
+       0xd0, 0xca, 0x2b, 0xcb, 0xae, 0x7b, 0x30, 0xb4,
+       0x77, 0xcb, 0x2d, 0xa3, 0x80, 0x30, 0xf2, 0x0c,
+       0x6a, 0x42, 0xb7, 0x3b, 0xbe, 0xac, 0x01, 0xfa,
+};

Looks like the scope of this array is only for rte_event_eth_rx_adapter_init,
if so please move it to stack.

OK.


+static uint8_t *rss_key_be;

Can we remove this global variable add it in in adapter memory?


There is currently struct rte_event_eth_rx_adapter
**rte_event_eth_rx_adapter that is an array of pointers to the adapters.
rss_key_be points to memory after this array.

are you thinking of something like:

struct {
struct rte_event_eth_rx_adapter **rte_event_eth_rx_adapter
uint8_t *rss_key_be;
} global;

I was thinking, to hold 40B in struct rte_event_eth_rx_adapter for
rss_key_be and initialize per rx_adapter to avoid global variable
as fill_event_buffer() has access to rte_event_eth_rx_adapter.

Something like below as rough idea.
➜ [dpdk-next-eventdev] $ git diff
lib/librte_eventdev/rte_event_eth_rx_adapter.c
diff --git a/lib/librte_eventdev/rte_event_eth_rx_adapter.c
b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
index cd19e7c28..ba6148931 100644
--- a/lib/librte_eventdev/rte_event_eth_rx_adapter.c
+++ b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
@@ -37,6 +37,7 @@ struct rte_eth_event_enqueue_buffer {
  };
  
  struct rte_event_eth_rx_adapter {
+       uint8_t rss_key[40];
         /* event device identifier */
         uint8_t eventdev_id;
         /* per ethernet device structure */

OK.


+
+static int
+default_conf_cb(uint8_t id, uint8_t dev_id,
+               struct rte_event_eth_rx_adapter_conf *conf, void *arg)
+{
+
+       ret = rte_event_port_setup(dev_id, port_id, port_conf);
+       if (ret) {
+               RTE_EDEV_LOG_ERR("failed to setup event port %u\n",
+                                       port_id);

return or add goto to exit from here to avoid calling rte_event_dev_start below

Could do the return but I wanted to leave the device in the same state as it
was at entry into this function. Thoughts ?

Will calling rte_event_dev_start() down(in case if wont return) change
the state? if not, it is fine.


OK, will put in the return. if the device were configured with an 
additional port and the setup for this port fails. The 
rte_event_dev_start() call will dereference a NULL ptr.

Nikhil



No another comments. Looks good to me.