-----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 */
> > > +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.
