[dpdk-dev] [PATCH 3/4] eventdev: Add eventdev ethernet Rx adapter

[Rao, Nikhil]

On 9/15/2017 11:37 AM, Nipun Gupta wrote:
> 
> 
>> -----Original Message-----
>> From: Nikhil Rao [mailto:nikhil.rao at intel.com]
<snip>
>> +static inline void
>> +fill_event_buffer(struct rte_event_eth_rx_adapter *rx_adapter,
>> +	uint8_t dev_id,
>> +	uint16_t rx_queue_id,
>> +	struct rte_mbuf **mbufs,
>> +	uint16_t num)
>> +{
>> +	uint32_t i;
>> +	struct eth_device_info *eth_device_info =
>> +					&rx_adapter->eth_devices[dev_id];
>> +	struct eth_rx_queue_info *eth_rx_queue_info =
>> +					&eth_device_info-
>>> rx_queue[rx_queue_id];
>> +
>> +	int32_t qid = eth_rx_queue_info->event_queue_id;
>> +	uint8_t sched_type = eth_rx_queue_info->sched_type;
>> +	uint8_t priority = eth_rx_queue_info->priority;
>> +	uint32_t flow_id;
>> +	struct rte_event events[BATCH_SIZE];
>> +	struct rte_mbuf *m = mbufs[0];
>> +	uint32_t rss_mask;
>> +	uint32_t rss;
>> +	int do_rss;
>> +
>> +	/* 0xffff ffff if PKT_RX_RSS_HASH is set, otherwise 0 */
>> +	rss_mask = ~(((m->ol_flags & PKT_RX_RSS_HASH) != 0) - 1);
>> +	do_rss = !rss_mask && !eth_rx_queue_info->flow_id_mask;
>> +
>> +	for (i = 0; i < num; i++) {
>> +		m = mbufs[i];
>> +		struct rte_event *ev = &events[i];
>> +
>> +		rss = do_rss ? do_softrss(m) : m->hash.rss;
>> +		flow_id =
>> +		    eth_rx_queue_info->flow_id &
>> +				eth_rx_queue_info->flow_id_mask;
>> +		flow_id |= rss & ~eth_rx_queue_info->flow_id_mask;
>> +
>> +		ev->flow_id = flow_id;
>> +		ev->op = RTE_EVENT_OP_NEW;
>> +		ev->sched_type = sched_type;
>> +		ev->queue_id = qid;
>> +		ev->event_type = RTE_EVENT_TYPE_ETHDEV;
>> +		ev->sub_event_type = 0;
>> +		ev->priority = priority;
>> +		ev->mbuf = m;
> 
> How will the application get to know about the queue and eth_port from where the
> packet is received by the eventdev? Is it the user responsibility to have that information
> in the flow_id?
queue: If the application needs to know the queue ID, it would have to 
set the flow ID to the
queue ID at the time of adding the queue.

eth port: input port contained in in the mbuf.

>> +
>> +		buf_event_enqueue(rx_adapter, ev);
>> +	}
>> +}
>> +
> 
> <snip>
> 
>> +int
>> +rte_event_eth_rx_adapter_queue_add(uint8_t id,
>> +		uint8_t eth_dev_id,
>> +		int32_t rx_queue_id,
>> +		const struct rte_event_eth_rx_adapter_queue_conf
>> *queue_conf)
>> +{
>> +	int ret;
>> +	uint32_t rx_adapter_cap;
>> +	struct rte_event_eth_rx_adapter *rx_adapter;
>> +	struct rte_eventdev *dev;
>> +	struct eth_device_info *dev_info;
>> +	int start_service = 0;
>> +
>> +	RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL);
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(eth_dev_id, -EINVAL);
>> +
>> +	rx_adapter = id_to_rx_adapter(id);
>> +	if (!rx_adapter || !queue_conf)
>> +		return -EINVAL;
>> +
>> +	dev = &rte_eventdevs[rx_adapter->eventdev_id];
>> +	ret = (*dev->dev_ops->eth_rx_adapter_caps_get)(dev, eth_dev_id,
>> +						&rx_adapter_cap);
>> +	if (ret) {
>> +		RTE_EDEV_LOG_ERR("Failed to get adapter caps edev %" PRIu8
>> +			"eth port %" PRIu8, id, eth_dev_id);
>> +		return ret;
>> +	}
>> +
>> +	if (!(rx_adapter_cap & RTE_EVENT_ETH_RX_ADAPTER_CAP_FLOW_ID)
>> &&
>> +		!(queue_conf->rx_queue_flags &
>> +
>> 	RTE_EVENT_ETH_RX_ADAPTER_QUEUE_FLOW_ID_VALID)) {
>> +		RTE_EDEV_LOG_ERR("Flow ID required for configuration,"
>> +				" eth port: %" PRIu8 " adapter id: %" PRIu8,
>> +				eth_dev_id, id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if ((rx_adapter_cap &
>> RTE_EVENT_ETH_RX_ADAPTER_CAP_SINGLE_EVENTQ) &&
>> +		(rx_queue_id != -1)) {
>> +		RTE_EDEV_LOG_ERR("Rx queues can only be connected to
>> single "
>> +			"event queue id %u eth port %u", id, eth_dev_id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (rx_queue_id != -1 && (uint16_t)rx_queue_id >=
>> +			rte_eth_devices[eth_dev_id].data->nb_rx_queues) {
>> +		RTE_EDEV_LOG_ERR("Invalid rx queue_id %" PRIu16,
>> +			 (uint16_t)rx_queue_id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	start_service = 0;
>> +	dev_info = &rx_adapter->eth_devices[eth_dev_id];
>> +
>> +	if (rx_adapter_cap &
>> RTE_EVENT_ETH_RX_ADAPTER_CAP_INTERNAL_PORT) {
>> +		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
>>> eth_rx_adapter_queue_add,
>> +					-ENOTSUP);
>> +		if (!dev_info->rx_queue) {
>> +			dev_info->rx_queue =
>> +			    rte_zmalloc_socket(rx_adapter->mem_name,
>> +					dev_info->dev->data->nb_rx_queues *
>> +					sizeof(struct eth_rx_queue_info), 0,
>> +					rx_adapter->socket_id);
>> +			if (!dev_info->rx_queue)
>> +				return -ENOMEM;
>> +		}
>> +
>> +		ret = (*dev->dev_ops->eth_rx_adapter_queue_add)(dev,
>> eth_dev_id,
>> +				rx_queue_id, queue_conf);
> 
> IMO, Instead of eth_dev_id rte_eth_dev parameter should be passed to the underlying driver.
> Otherwise the underlying driver needs to use rte_eth_devices[] global variable to fetch the device.
> This does not seems very right to me.

Agree, passing struct rte_eth_dev to the PMD is a cleaner approach.

> 
>> +		if (!ret) {
>> +			update_queue_info(rx_adapter,
>> +					&rx_adapter-
>>> eth_devices[eth_dev_id],
>> +					rx_queue_id,
>> +					1);
>> +		}
>> +	} else {
>> +		rte_spinlock_lock(&rx_adapter->rx_lock);
>> +		ret = init_service(rx_adapter, id);
>> +		if (!ret)
>> +			ret = add_rx_queue(rx_adapter, eth_dev_id,
>> rx_queue_id,
>> +					queue_conf);
>> +		rte_spinlock_unlock(&rx_adapter->rx_lock);
>> +		if (!ret)
>> +			start_service =
>> !!sw_rx_adapter_queue_count(rx_adapter);
>> +	}
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (start_service)
>> +		rte_service_component_runstate_set(rx_adapter->service_id,
>> 1);
>> +
>> +	return 0;
>> +}
>> +
>> +int
>> +rte_event_eth_rx_adapter_queue_del(uint8_t id, uint8_t eth_dev_id,
>> +				int32_t rx_queue_id)
>> +{
>> +	int ret = 0;
>> +	struct rte_eventdev *dev;
>> +	struct rte_event_eth_rx_adapter *rx_adapter;
>> +	struct eth_device_info *dev_info;
>> +	uint32_t rx_adapter_cap;
>> +	uint16_t i;
>> +
>> +	RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL);
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(eth_dev_id, -EINVAL);
>> +
>> +	rx_adapter = id_to_rx_adapter(id);
>> +	if (!rx_adapter)
>> +		return -EINVAL;
>> +
>> +	dev = &rte_eventdevs[rx_adapter->eventdev_id];
>> +	ret = dev->dev_ops->eth_rx_adapter_caps_get(dev, eth_dev_id,
>> +						&rx_adapter_cap);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (rx_queue_id != -1 && (uint16_t)rx_queue_id >=
>> +		rte_eth_devices[eth_dev_id].data->nb_rx_queues) {
>> +		RTE_EDEV_LOG_ERR("Invalid rx queue_id %" PRIu16,
>> +			 (uint16_t)rx_queue_id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	dev_info = &rx_adapter->eth_devices[eth_dev_id];
>> +
>> +	if (rx_adapter_cap &
>> RTE_EVENT_ETH_RX_ADAPTER_CAP_INTERNAL_PORT) {
>> +		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
>>> eth_rx_adapter_queue_del,
>> +				 -ENOTSUP);
>> +		ret = (*dev->dev_ops->eth_rx_adapter_queue_del)(dev,
>> eth_dev_id,
>> +							rx_queue_id);
> 
> Here too rte_eth_device shall be passed.
OK.

> 
>> +		if (!ret) {
>> +			update_queue_info(rx_adapter,
>> +					&rx_adapter-
>>> eth_devices[eth_dev_id],
>> +					rx_queue_id,
>> +					0);
>> +			if (!dev_info->nb_dev_queues) {
>> +				rte_free(dev_info->rx_queue);
>> +				dev_info->rx_queue = NULL;
>> +			}
>> +		}
>> +	} else {
>> +		int rc;
>> +		rte_spinlock_lock(&rx_adapter->rx_lock);
>> +		if (rx_queue_id == -1) {
>> +			for (i = 0; i < dev_info->dev->data->nb_rx_queues; i++)
>> +
>> 	_rte_event_eth_rx_adapter_queue_del(rx_adapter,
>> +								dev_info,
>> +								i);
>> +		} else {
>> +			_rte_event_eth_rx_adapter_queue_del(rx_adapter,
>> +							dev_info,
>> +
>> 	(uint16_t)rx_queue_id);
>> +		}
>> +
>> +		rc = eth_poll_wrr_calc(rx_adapter);
>> +		if (rc)
>> +			RTE_EDEV_LOG_ERR("WRR recalculation failed %"
>> PRId32,
>> +					rc);
>> +
>> +		if (!dev_info->nb_dev_queues) {
>> +			rte_free(dev_info->rx_queue);
>> +			dev_info->rx_queue = NULL;
>> +		}
>> +
>> +		rte_spinlock_unlock(&rx_adapter->rx_lock);
>> +		rte_service_component_runstate_set(rx_adapter->service_id,
>> +				sw_rx_adapter_queue_count(rx_adapter));
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +
>> +int
>> +rte_event_eth_rx_adapter_start(uint8_t id)
>> +{
>> +	return rx_adapter_ctrl(id, 1);
>> +}
>> +
>> +int
>> +rte_event_eth_rx_adapter_stop(uint8_t id)
>> +{
>> +	return rx_adapter_ctrl(id, 0);
>> +}
>> +
>> +int
>> +rte_event_eth_rx_adapter_stats_get(uint8_t id,
>> +			       struct rte_event_eth_rx_adapter_stats *stats)
>> +{
>> +	struct rte_event_eth_rx_adapter *rx_adapter;
>> +	struct rte_event_eth_rx_adapter_stats dev_stats_sum = { 0 };
>> +	struct rte_event_eth_rx_adapter_stats dev_stats;
>> +	struct rte_eventdev *dev;
>> +	struct eth_device_info *dev_info;
>> +	uint32_t i;
>> +	int ret;
>> +
>> +	RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL);
>> +
>> +	rx_adapter = id_to_rx_adapter(id);
>> +	if (!rx_adapter || !stats)
>> +		return -EINVAL;
>> +
>> +	dev = &rte_eventdevs[rx_adapter->eventdev_id];
>> +	memset(stats, 0, sizeof(*stats));
>> +	for (i = 0; i < rte_eth_dev_count(); i++) {
>> +		dev_info = &rx_adapter->eth_devices[i];
>> +		if (!dev_info->internal_event_port)
>> +			continue;
>> +		ret = (*dev->dev_ops->eth_rx_adapter_stats_get)(dev, i,
>> +						&dev_stats);
> 
> Preferable to check if the adapter stats get and stats set API are registered from the driver.
>

OK.

Thanks for the review, I will make the changes and post a v4.

Nikhil

> Regards,
> Nipun
>