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

[Pavan Nikhilesh Bhagavatula <pbhagavatula@caviumnetworks.com>]

Hi Nikhil,

Few comments Inline

On Fri, Sep 22, 2017 at 02:47:13AM +0530, Nikhil Rao wrote:
> Add common APIs for configuring packet transfer from ethernet Rx
> queues to event devices across HW & SW packet transfer mechanisms.
> A detailed description of the adapter is contained in the header's
> comments.
>
> The adapter implementation uses eventdev PMDs to configure the packet
> transfer if HW support is available and if not, it uses an EAL service
> function that reads packets from ethernet Rx queues and injects these
> as events into the event device.
>
> Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
> Signed-off-by: Gage Eads <gage.eads@intel.com>
> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> ---
>  lib/librte_eventdev/rte_event_eth_rx_adapter.h |  384 ++++++++
>  lib/librte_eventdev/rte_event_eth_rx_adapter.c | 1238 ++++++++++++++++++++++++
>  lib/Makefile                                   |    2 +-
>  lib/librte_eventdev/Makefile                   |    2 +
>  lib/librte_eventdev/rte_eventdev_version.map   |   11 +-
>  5 files changed, 1635 insertions(+), 2 deletions(-)
>  create mode 100644 lib/librte_eventdev/rte_event_eth_rx_adapter.h
>  create mode 100644 lib/librte_eventdev/rte_event_eth_rx_adapter.c
>
> diff --git a/lib/librte_eventdev/rte_event_eth_rx_adapter.h b/lib/librte_eventdev/rte_event_eth_rx_adapter.h
> new file mode 100644
> index 000000000..c3849ec31
> --- /dev/null
> +++ b/lib/librte_eventdev/rte_event_eth_rx_adapter.h
> @@ -0,0 +1,384 @@
> +/*
> +

<snip>

> +/**
> + * Create a new ethernet Rx event adapter with the specified identifier.
> + *
> + * @param id
> + *  The identifier of the ethernet Rx event adapter.
> + *
> + * @dev_id
> + *  The identifier of the device to configure.
> + *
> + * @eth_port_id
> + *  The identifier of the ethernet device.
> + *
Invalid param
> + * @param conf_cb
> + *  Callback function that fills in members of a
> + *  struct rte_event_eth_rx_adapter_conf struct passed into
> + *  it.
> + *
> + * @param conf_arg
> + *  Argument that is passed to the conf_cb function.
> + *
> + * @return
> + *   - 0: Success
> + *   - <0: Error code on failure
> + */
> +int rte_event_eth_rx_adapter_create_ext(uint8_t id, uint8_t dev_id,
> +					rx_adapter_conf_cb conf_cb,
> +					void *conf_arg);
> +
> +/**
> + * Create a new ethernet Rx event adapter with the specified identifier.
> + * This function uses an internal configuration function that creates an event
> + * port. This default function reconfigures the event device with an
> + * additional event port and setups up the event port using the port_config
> + * parameter passed into this function. In case the application needs more
> + * control in configuration of the service, it should use the
> + * rte_event_eth_rx_adapter_create_ext() version.
> + *
> + * @param id
> + *  The identifier of the ethernet Rx event adapter.
> + *
> + * @dev_id
> + *  The identifier of the device to configure.
> + *
> + * @eth_port_id
> + *  The identifier of the ethernet device.
> + *
> + * @param conf_cb
> + *  Callback function that fills in members of a
> + *  struct rte_event_eth_rx_adapter_conf struct passed into
> + *  it.
> + *
> + * @param conf_arg
> + *  Argument of type *rte_event_port_conf* that is passed to the conf_cb
> + *  function.
> + *
Invalid param
> + * @return
> + *   - 0: Success
> + *   - <0: Error code on failure
> + */
> +int rte_event_eth_rx_adapter_create(uint8_t id, uint8_t dev_id,
> +				struct rte_event_port_conf *port_config);
> +
> +/**
> + * Free an event adapter
> + *
> + * @param id
> + *  Adapter identifier.
> + *
> + * @return
> + *   - 0: Success
> + *   - <0: Error code on failure, If the adapter still has Rx queues
> + *      added to it, the function returns -EBUSY.
> + */
> +int rte_event_eth_rx_adapter_free(uint8_t id);
> +
<snip>
> +/**
> + * Reset statistics for an adapter
> + *
> + * @param id
> + *  Adapter identifier.
> + *
> + * @return
> + *  - 0: Success, statistics reset successfully.
> + *  - <0: Error code on failure.
> + */
> +int rte_event_eth_rx_adapter_stats_reset(uint8_t id);
> +
> +/**
> + * Retrieve the service ID of an adapter. If the adapter doesn't use
> + * a rte_service function, this function returns -ESRCH
> + *
> + * @param id
> + *  Adapter identifier.

Param missing

> + *
> + * @return
> + *  - 0: Success, statistics reset successfully.

Invalid description.

> + *  - <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;
> +		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.

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

Regards,
Pavan