From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Rao, Nikhil" <nikhil.rao@intel.com>
Subject: Re: [PATCH v4 3/4] eventdev: Add eventdev ethernet Rx
	adapter
Date: Sat, 23 Sep 2017 17:05:21 +0530
Message-ID: <0327e0b4-d632-a542-1b63-d48174401e29@intel.com>
References: <1506028634-22998-1-git-send-email-nikhil.rao@intel.com>
 <1506028634-22998-4-git-send-email-nikhil.rao@intel.com>
 <20170921154352.GA5744@PBHAGAVATULA-LT>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
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
To: Pavan Nikhilesh Bhagavatula <pbhagavatula@caviumnetworks.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mga06.intel.com (mga06.intel.com [134.134.136.31])
 by dpdk.org (Postfix) with ESMTP id F3063276C
 for <dev@dpdk.org>; Sat, 23 Sep 2017 13:35:26 +0200 (CEST)
In-Reply-To: <20170921154352.GA5744@PBHAGAVATULA-LT>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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
>