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

["Rao, Nikhil" <nikhil.rao@intel.com>]

On 9/21/2017 9:13 PM, Pavan Nikhilesh Bhagavatula wrote:
> Hi Nikhil,
> 
> Few comments Inline
>
<snip>

>  + *  - 0: Success, statistics reset successfully.
> 
> Invalid description.

Thanks Pavan, for catching these, will fix.

> 
>> + *  - <0: Error code on failure, if the adapter doesn't use a rte_service
>> + * function, this function returns -ESRCH.
>> + */
>> +int rte_event_eth_rx_adapter_service_id_get(uint8_t id, uint32_t *service_id);
>> +
>> +#ifdef __cplusplus
>> +}
>> +#endif
>> +#endif	/* _RTE_EVENT_ETH_RX_ADAPTER_ */
>> diff --git a/lib/librte_eventdev/rte_event_eth_rx_adapter.c b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
>> new file mode 100644
>> index 000000000..d5b655dae
>> --- /dev/null
>> +++ b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
>> @@ -0,0 +1,1238 @@
> <snip>
>> +
>> +static int
>> +rx_adapter_ctrl(uint8_t id, int start)
>> +{
>> +	struct rte_event_eth_rx_adapter *rx_adapter;
>> +	struct rte_eventdev *dev;
>> +	struct eth_device_info *dev_info;
>> +	uint32_t i;
>> +	int use_service = 0;
>> +	int stop = !start;
>> +
>> +	RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL);
>> +	rx_adapter = id_to_rx_adapter(id);
>> +	if (!rx_adapter)
>> +		return -EINVAL;
>> +
>> +	dev = &rte_eventdevs[rx_adapter->eventdev_id];
>> +
>> +	for (i = 0; i < rte_eth_dev_count(); i++) {
>> +		dev_info = &rx_adapter->eth_devices[i];
>> +		/* if start  check for num dev queues */
>> +		if (start && !dev_info->nb_dev_queues)
>> +			continue;
>> +		/* if stop check if dev has been started */
>> +		if (stop && !dev_info->dev_rx_started)
>> +			continue;:1
>> +		use_service |= !dev_info->internal_event_port;
>> +		dev_info->dev_rx_started = start;
>> +		if (!dev_info->internal_event_port)
>> +			continue;
>> +		start ? (*dev->dev_ops->eth_rx_adapter_start)(dev,
>> +						&rte_eth_devices[i]) :
>> +			(*dev->dev_ops->eth_rx_adapter_stop)(dev,
>> +						&rte_eth_devices[i]);
>> +	}
>> +
>> +	if (use_service)
> 
> Here setting the service run state is not sufficient we need to enable the
> service on a service core calling rte_service_start_with_defaults() should be
> sufficient.
> 

Yes it is necessary but insufficient.

IMO,
If the application is controlling core masks, the application flow at 
startup looks like:

rte_event_eth_rx_adapter_create(id,..)
...
rte_event_eth_rx_adapter_start(id)
if (!rte_event_eth_rx_adapter_service_id_get(id, &service_id)) {
	rte_service_lcore_add(rx_lcore_id);
         rte_service_map_lcore_set(service_id, rx_lcore_id, 1);
	rte_service_lcore_start(rx_lcore_id)
}

Since rte_service_start_with_defaults() is invoked before the adapter is 
created, how would it get assigned a core etc ?


Nikhil
>> +		rte_service_runstate_set(rx_adapter->service_id, start);
>> +
>> +	return 0;
>> +}
>> +
> <snip>
> 
> Regards,
> Pavan
>