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

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Sun, 24 Sep 2017 23:46:51 +0530
> From: "Rao, Nikhil" <nikhil.rao@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: bruce.richardson@intel.com, gage.eads@intel.com, dev@dpdk.org,
>  thomas@monjalon.net, harry.van.haaren@intel.com, hemant.agrawal@nxp.com,
>  nipun.gupta@nxp.com, narender.vangati@intel.com,
>  erik.g.carrillo@intel.com, abhinandan.gujjar@intel.com,
>  santosh.shukla@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 */




> > > +static inline uint32_t
> > > +eth_rx_poll(struct rte_event_eth_rx_adapter *rx_adapter)
> > > +static int
> > > +event_eth_rx_adapter_service_func(void *args)
> > > +{
> > > +	struct rte_event_eth_rx_adapter *rx_adapter = args;
> > > +	struct rte_eth_event_enqueue_buffer *buf;
> > > +
> > > +	buf = &rx_adapter->event_enqueue_buffer;
> > > +	if (!rte_spinlock_trylock(&rx_adapter->rx_lock))
> > > +		return 0;
> > > +	if (eth_rx_poll(rx_adapter) == 0 && buf->count)
> > > +		flush_event_buffer(rx_adapter);
> > > +	rte_spinlock_unlock(&rx_adapter->rx_lock);
> > > +	return 0;
> > > +}
> > > +
> > > +static int
> > > +rte_event_eth_rx_adapter_init(void)
> > > +{
> > > +	const char *name = "rte_event_eth_rx_adapter_array";
> > > +	const struct rte_memzone *mz;
> > > +	unsigned int sz;
> > > +	unsigned int rss_key_off;
> > > +
> > > +	sz = sizeof(*rte_event_eth_rx_adapter) *
> > > +	    RTE_MAX_EVENT_ETH_RX_ADAPTER_INSTANCE;
> > 
> > I think, you need to use size of struct rte_event_eth_rx_adapter here. if so,
> > we need **rte_event_eth_rx_adapter here. Right?
> > 
> > test code
> > struct abc {
> > 
> >          uint64_t a[64];
> > };
> > 
> > struct abc **k;
> > 
> > int main()
> > {
> > 	printf("%d %d %d\n", sizeof(k), sizeof(*k), sizeof(**k));
> > 
> > 	return 0;
> > }
> > 
> > $./a.out
> > 8 8 512
> > 
> 
> The struct rte_event_eth_rx_adapter gets allocated in
> rte_event_eth_rx_adapter_create_ext()

OK. I missed that.
> > 
> > > +		if (mz) {
> > > +			rte_convert_rss_key((uint32_t *)default_rss_key,
> > > +			    (uint32_t *)(uintptr_t)(mz->addr_64 + rss_key_off),
> > > +			    RTE_DIM(default_rss_key));
> > > +		} else {
> > > +			RTE_EDEV_LOG_ERR("failed to reserve memzone err = %"
> > > +					PRId32, rte_errno);
> > > +			return -rte_errno;
> > > +		}
> > > +	}
> > > +
> > > +	rte_event_eth_rx_adapter = mz->addr;
> > > +	rss_key_be = (uint8_t *)(mz->addr_64 + rss_key_off);
> > > +	return 0;
> > > +}
> > > +
> > > +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.

No another comments. Looks good to me.