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

[Rao, Nikhil]

Hi Jerin,

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 at 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.

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 ?
>>>> 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.

>>
>> }
>>
>> 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.

>> 	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.

>>
>>
>>>
>>> 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.