Re: [dpdk-dev] [PATCH 1/2] eventdev: add event adapter for ethernet Rx queues

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

-----Original Message-----
> Date: Mon, 10 Jul 2017 11:44:10 +0530
> From: "Rao, Nikhil" <nikhil.rao@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: gage.eads@intel.com, dev@dpdk.org, thomas@monjalon.net,
>  bruce.richardson@intel.com, harry.van.haaren@intel.com,
>  hemant.agrawal@nxp.com, nipun.gupta@nxp.com, narender.vangati@intel.com,
>  Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> Subject: Re: [PATCH 1/2] eventdev: add event adapter for ethernet Rx queues
> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>  Thunderbird/52.2.1
> 
> Hi Jerin,

Hi Nikhil,

> 
> thanks for your feedback, further comments below.
> 
> On 7/7/2017 9:27 PM, Jerin Jacob wrote:
> > -----Original Message-----
> > > Date: Fri, 7 Jul 2017 20:33:19 +0530
> > > From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > > /* adapter has inbuilt port, no need to create producer port */
> > > > > #define RTE_EVENT_ETHDEV_CAP_INBUILT_PORT  (1ULL << 0)
> > > > > /* adapter does not need service function */
> > > > > #define RTE_EVENT_ETHDEV_CAP_NO_SERVICE_FUNC (1ULL << 1)
> > > 
> > > I just provided a name to share the view. You can choose better name.
> > > 
> OK.
> 
> > > > > int rte_event_eth_rx_adapter_create(uint8_t id, uint8_t dev_id, uint8_t
> > > > eth_port_id);
> > > > 
> 
> I also think that the application should be able to call create() with > 1
> ports. This would allow a single service to poll multiple NICs with WRR
> priority.

Good point.

Can we realize the same use case like below?
- Instead of applying WRR over multiple NIC ports and adding the logic in Rx
  adapter, How about applying the WRR over multiple service function and
  move the WRR logic to service function layer.

i.e
one adapter is
- one service function(adapter_queue_add() will be used to add more
  queues)
- one constant set of ops.

Advantages:
- WRR over service functions will be useful as other service functions
can utilize it as it is not strictly specific to Rx adapter.
- In order to work with, below mentioned use cases, RX adapter ops needs
  to be constant and it will decided on the _adapter_create where "eth_port_id"
and "dev_id" specified.

1) Ethdev HW is not capable of injecting the packets and SW eventdev
driver(All existing ethdev PMD + drivers/event/sw PMD combination)
2) Ethdev HW is not capable of injecting the packets and not compatible
HW eventdev driver(All existing ethdev PMD + driver/event/octeontx PMD
combination)
3) Ethdev HW is capable of injecting the packet to compatible
HW eventdev driver.

- it will remove the below side effect(queue add/del API needs port_id)

Thoughts?

> 
> The side effect of is that the queue add/del APIs would need to specify the
> port_id as well.
> 
> > > > > int rte_event_eth_rx_adapter_get_info(uint8_t id, struct
> > > > rte_event_eth_rx_adap_info *info);
> > > > > int rte_event_eth_rx_adapter_configure(uint8_t id, struct
> > > > rte_event_eth_rx_adap_config *cfg);
> > > > > int rte_event_eth_rx_adapter_queue_add(uint8_t id, int32_t rx_queue_id,
> > > > const struct rte_eth_rx_event_adapter_queue_config *config);
> > > > > int rte_event_eth_rx_adapter_queue_del(uint8_t id, int32_t rx_queue_id)
> 
> > > > > int rte_event_eth_rx_adapter_run();
> 
> The adapter's run function is not part of the public interface, agree ?

Agree.

> > > > > int rte_event_eth_rx_adapter_free(uint8_t id);
> > > > > 
> > > > 
> > > > If I understood your idea, the function pointer struct would look something
> > > > like this.
> > > > 
> > > > struct rte_eventdev_rx_adapter_ops {
> > > > 	rx_adapter_create_t 	create,
> > > > 	rx_adapter_get_info_t	info,
> > > > 	rx_adapter_configure_t	configure,
> > > > 	rx_adapter_queue_add_t  queue_add,
> > > > 	rx_adapter_queue_del_t	queue_del,
> > > > 	rx_adapter_queue_free_t	queue_free,
> > > >        rx_adapter_free_t	free
> > > > };
> > > 
> > > Yes. But, I think, adapter create and free goes in struct rte_eventdev_op .
> > > See below.
> > > 
> > > 
> > > > 
> > > > struct rte_eventdev {
> > > >         ..
> > > >         const struct rte_eventdev_rx_adapter_ops *rx_adapter_ops;
> > > >         ..
> > > > };
> > > 
> > > An eventdev instance can have N adapters not just one. So this will be
> > > pointer to pointer, indexed through adapter_id.
> > > 
> > > In SW PMD case, the eventdev may have only one adapter.
> > > In HW PMD cases, There will more than one. example,
> > > - if octeontx eventdev dev_id + any external PCI n/w card like i40e or
> > > nicvf case an adapter with similar adapter ops as SW PMD
> > > - if octeontx eventdev dev_id + octeontx ethdev dev_id case another
> > > adapter that does not need service cores to inject event to octeontx
> > > eventdev
> > > 
> > > - Your generic Rx adapter we will make it as common code so that both SW PMD and
> > > octeontx PMD in service core mode as use the functions and register the
> > > ops.
> 
> OK.
> > > 
> > > 
> > > I was thinking like this
> > > 
> > > int rte_event_eth_rx_adapter_create(uint8_t id, uint8_t dev_id, uint8_t eth_port_id)
> > > {
> > > 	eventdev_ops = ... /* access through dev_id */
> > > 
> > > 	/* common code to get ops memory from adapter id */
> > > 	struct rte_eventdev_rx_adapter_ops* ops = rte_event_pmd_adapter_allocate(id);
> > > 
> > > 	/* Fill in adapter ops from driver */
> > > 	eventdev_ops->create_adapter(ops, dev_id, eth_port_id)
> 
> As an implementation detail, the function pointer setup needs to account for
> DPDK primary/secondary processes, i.e, function addresses could be different
> in the 2 processes.

Yes

> 
> > > 
> > > }
> > > 
> > > int rte_event_eth_rx_adapter_get_info(uint8_t id, struct rte_event_eth_rx_adap_info *info)
> > > {
> 
> Argument list is missing the dev_id (as are the
> configure/queue_add/queue_del) functions.

Yes


> > > 	eventdev_ops = ... /* access through dev_id */
> > > 
> > > 	struct rte_eventdev_rx_adapter_ops* ops = eventdev_ops->rx_adapter_ops[id];
> > > 
> > > 	/* adapter specific info ops)
> > > 	ops->info(ops,....);
> > > 
> > > }
> > 
> > IMO, the mapping with your current proposal may go like this.
> > 
> > - rte_event_eth_rx_adapter_create() // Will just fill the adapter ops from driver, probably
> >    your existing rte_event_eth_rx_adapter_init() can go here
> > - exiting rte_event_eth_rx_adapter_create() will be more of
> > proposed rte_event_eth_rx_adapter_configure() on the given adapter.
> > - existing rte_event_eth_rx_adapter_queue_add() maps 1:1. But you can
> >    remove eth_dev_id as it is provided proposed rte_event_eth_rx_adapter_create()
> 
> See above for the case where the application could call create() with 2
> different ports.
> 
> > - same applies to rte_event_eth_rx_adapter_queue_del()
> > - exiting rte_event_eth_rx_adapter_stats_get() can be changed to
> >    "xstat"(more like eventdev xstat scheme) so based on the adapter
> > capability application can get stats)
> > 
> > 
> > flow shall be:
> > 1) application calls rte_event_eth_rx_adapter_create(id, eventdev_id,
> > ethdev_id) to create the adapter.(Drivers fills the adapter ops based on
> > eventdev_id + ethdev_id capabilities)
> > 2) application calls rte_event_eth_rx_adapter_info_get() to get the
> > capability and other information of the adapter.
> > 3) application calls rte_event_eth_rx_adapter_configure() to configure
> > based on the application need and the adapter capability.
> > 4) application calls rte_event_eth_rx_adapter_queue_add() to link ethdev queues to
> > eventdev queues. On this call, The driver creates the service functions or programs the HW registers
> > based on the adapter capability to take the events from ethdev and inject to eventdev.
> > 
> 
> The flow looks good to me.

Great!!

> 
> > > 
> > > 
> > > > 
> > > > But from previous emails (see struct rte_event_dev_producer_conf below), it
> > > > appears that the rx queue id and event information would be needed to create
> > > > the adapter that enqueues from the ethdev queue id to the event pmd, however
> > > > that information is available only at queue add time - thoughts ?
> > > 
> > > Just eventdev_id and ethdev_id is enough to create adapter.
> 
> OK.