Re: [dpdk-dev] [PATCH 1/4] eventdev: add eth Tx adapter APIs

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Fri, 6 Jul 2018 12:12:06 +0530
> From: Nikhil Rao <nikhil.rao@intel.com>
> To: jerin.jacob@caviumnetworks.com, olivier.matz@6wind.com
> CC: nikhil.rao@intel.com, dev@dpdk.org
> Subject: [PATCH 1/4] eventdev: add eth Tx adapter APIs
> X-Mailer: git-send-email 1.8.3.1
> 
> 
> The ethernet Tx adapter abstracts the transmit stage of an
> event driven packet processing application. The transmit
> stage may be implemented with eventdev PMD support or use a
> rte_service function implemented in the adapter. These APIs
> provide a common configuration and control interface and
> an transmit API for the eventdev PMD implementation.
> 
> The transmit port is specified using mbuf::port. The transmit
> queue is specified using the rte_event_eth_tx_adapter_txq_set()
> function. The mbuf will specify a queue ID in the future
> (http://mails.dpdk.org/archives/dev/2018-February/090651.html)
> at which point this function will be replaced with a macro.
> 
> Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
> ---

1) Update doc/api/doxy-api-index.md
2) Update lib/librte_eventdev/Makefile
+SYMLINK-y-include += rte_event_eth_tx_adapter.h


I think, the following working is _pending_

1) Update app/test-eventdev/ for Tx adapter
2) Update examples/eventdev_pipeline/ for Tx adapter
3) Add Tx adapter documentation
4) Add Tx adapter ops for octeontx driver
5) Add Tx adapter ops for dpaa driver(if need)

Nikhil,
If you are OK then Cavium would like to take up (1), (2) and (4) activities.

Let me know your thoughts.

Since this patch set already crossed the RC1 deadline. We will complete
all the _pending_ work and push to next-eventdev tree in the very beginning of
v18.11 so that Anoob's adapter helper function work can be added v18.11.


> 
> This patch series adds the event ethernet Tx adapter which is
> based on a previous RFC
>  * RFCv1 - http://mails.dpdk.org/archives/dev/2018-May/102936.html
>  * RFCv2 - http://mails.dpdk.org/archives/dev/2018-June/104075.html
> 
> RFC -> V1:
> =========
> 
> * Move port and tx queue id to mbuf from mbuf private area. (Jerin Jacob)
> 
> * Support for PMD transmit function. (Jerin Jacob)
> 
> * mbuf change has been replaced with rte_event_eth_tx_adapter_txq_set().
> The goal is to align with the mbuf change for a qid field.
> (http://mails.dpdk.org/archives/dev/2018-February/090651.html). Once the mbuf
> change is available, the function can be replaced with a macro with no impact
> to applications.
> 
> * Various cleanups (Jerin Jacob)
> 
>  lib/librte_eventdev/rte_event_eth_tx_adapter.h | 497 +++++++++++++++++++++++++
>  lib/librte_mbuf/rte_mbuf.h                     |   4 +-
>  MAINTAINERS                                    |   5 +
>  3 files changed, 505 insertions(+), 1 deletion(-)
>  create mode 100644 lib/librte_eventdev/rte_event_eth_tx_adapter.h
> 
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * A structure used to retrieve statistics for an eth tx adapter instance.
> + */
> +struct rte_event_eth_tx_adapter_stats {
> +       uint64_t tx_retry;
> +       /**< Number of transmit retries */
> +       uint64_t tx_packets;
> +       /**< Number of packets transmitted */
> +       uint64_t tx_dropped;
> +       /**< Number of packets dropped */
> +};
> +
> +/** Event Eth Tx Adapter Structure */
> +struct rte_event_eth_tx_adapter {
> +       uint8_t id;
> +       /**< Adapter Identifier */
> +       uint8_t eventdev_id;
> +       /**< Max mbufs processed in any service function invocation */
> +       uint32_t max_nb_tx;
> +       /**< The adapter can return early if it has processed at least
> +        * max_nb_tx mbufs. This isn't treated as a requirement; batching may
> +        * cause the adapter to process more than max_nb_tx mbufs.
> +        */
> +       uint32_t nb_queues;
> +       /**< Number of Tx queues in adapter */
> +       int socket_id;
> +       /**< socket id */
> +       rte_spinlock_t tx_lock;
> +       /**<  Synchronization with data path */
> +       void *dev_private;
> +       /**< PMD private data */
> +       char mem_name[RTE_EVENT_ETH_TX_ADAPTER_SERVICE_NAME_LEN];
> +       /**< Memory allocation name */
> +       rte_event_eth_tx_adapter_conf_cb conf_cb;
> +       /** Configuration callback */
> +       void *conf_arg;
> +       /**< Configuration callback argument */
> +       uint16_t dev_count;
> +       /**< Highest port id supported + 1 */
> +       struct rte_event_eth_tx_adapter_ethdev *txa_ethdev;
> +       /**< Per ethernet device structure */
> +       struct rte_event_eth_tx_adapter_stats stats;
> +} __rte_cache_aligned;

Can you move this structure to .c file as implementation, Reasons are -
a) It should not be under ABI deprecation
b) INTERNAL_PORT based adapter may have different values.i.e the above
structure is implementation defined.

> +
> +struct rte_event_eth_tx_adapters {
> +       struct rte_event_eth_tx_adapter **data;
> +};
> +

same as above

> +/* Per eth device structure */
> +struct rte_event_eth_tx_adapter_ethdev {
> +       /* Pointer to ethernet device */
> +       struct rte_eth_dev *dev;
> +       /* Number of queues added */
> +       uint16_t nb_queues;
> +       /* PMD specific queue data */
> +       void *queues;
> +};

same as above

> +
> +extern struct rte_event_eth_tx_adapters rte_event_eth_tx_adapters;
> +

same as above

> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Create a new event ethernet Tx adapter with the specified identifier.
> + *
> + * @param id
> + *  The identifier of the event ethernet Tx adapter.
> + * @param dev_id
> + *  The event device identifier.
> + * @param port_config
> + *  Event port configuration, the adapter uses this configuration to
> + *  create an event port if needed.
> + * @return
> + *   - 0: Success
> + *   - <0: Error code on failure
> + */
> +int __rte_experimental
> +rte_event_eth_tx_adapter_create(uint8_t id, uint8_t dev_id,
> +                               struct rte_event_port_conf *port_config);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Create a new event ethernet Tx adapter with the specified identifier.
> + *
> + * @param id
> + *  The identifier of the event ethernet Tx adapter.
> + * @param dev_id
> + *  The event device identifier.
> + * @param conf_cb
> + *  Callback function that initalizes members of the

s/initalizes/initializes

> + *  struct rte_event_eth_tx_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_experimental
> +rte_event_eth_tx_adapter_create_ext(uint8_t id, uint8_t dev_id,
> +                               rte_event_eth_tx_adapter_conf_cb conf_cb,
> +                               void *conf_arg);
> +
> +/**
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Add a Tx queue to the adapter.
> + * A queue value of -1 is used to indicate all
> + * queues within the device.
> + *
> + * @param id
> + *  Adapter identifier.
> + * @param eth_dev_id
> + *  Ethernet Port Identifier.
> + * @param queue
> + *  Tx queue index.
> + * @return
> + *  - 0: Success, Queues added succcessfully.

s/succcessfully/successfully


> + *  - <0: Error code on failure.
> + */
> +int __rte_experimental
> +rte_event_eth_tx_adapter_queue_add(uint8_t id,
> +                               uint16_t eth_dev_id,
> +                               int32_t queue);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + *
> + * Set Tx queue in the mbuf.
> + *
> + * @param pkt
> + *  Pointer to the mbuf.
> + * @param queue
> + *  Tx queue index.
> + */
> +void __rte_experimental
> +rte_event_eth_tx_adapter_txq_set(struct rte_mbuf *pkt, uint16_t queue);

1) Can you make this as static inline for better performance(as it is just
a mbuf field access)?

2) Please add _get function, It will be useful for application and
Tx adapter op implementation.


> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Retrieve the adapter event port. The adapter creates an event port if
> + * the RTE_EVENT_ETH_TX_ADAPTER_CAP_INTERNAL_PORT is not set in the
> + * eth Tx capabilities of the event device.
> + *
> + * @param id
> + *  Adapter Identifier.
> + * @param[out] event_port_id
> + *  Event port pointer.
> + * @return
> + *   - 0: Success.
> + *   - <0: Error code on failure.
> + */
> +int __rte_experimental
> +rte_event_eth_tx_adapter_event_port_get(uint8_t id, uint8_t *event_port_id);
> +
> +static __rte_always_inline uint16_t __rte_experimental
> +__rte_event_eth_tx_adapter_enqueue(uint8_t id, uint8_t dev_id, uint8_t port_id,
> +                               struct rte_event ev[],
> +                               uint16_t nb_events,
> +                               const event_tx_adapter_enqueue fn)
> +{
> +       const struct rte_eventdev *dev = &rte_eventdevs[dev_id];

Access to *dev twice(see below rte_event_eth_tx_adapter_enqueue())

> +       struct rte_event_eth_tx_adapter *txa =
> +                                       rte_event_eth_tx_adapters.data[id];

Just like common Tx adapter implementation, We can manage  ethdev queue to adapter mapping
internally. So this deference is not required in fastpath.

Please simply call the following, just like other eventdev ops.
fn(dev->data->ports[port_id], ev, nb_events)


> +
> +#ifdef RTE_LIBRTE_EVENTDEV_DEBUG
> +       if (id >= RTE_EVENT_ETH_TX_ADAPTER_MAX_INSTANCE ||
> +               dev_id >= RTE_EVENT_MAX_DEVS ||
> +               !rte_eventdevs[dev_id].attached) {
> +               rte_errno = -EINVAL;
> +               return 0;
> +       }
> +
> +       if (port_id >= dev->data->nb_ports) {
> +               rte_errno = -EINVAL;
> +               return 0;
> +       }
> +#endif
> +       return fn((void *)txa, dev, dev->data->ports[port_id], ev, nb_events);
> +}
> +
> +/**
> + * Enqueue a burst of events objects or an event object supplied in *rte_event*
> + * structure on an  event device designated by its *dev_id* through the event
> + * port specified by *port_id*. This function is supported if the eventdev PMD
> + * has the RTE_EVENT_ETH_TX_ADAPTER_CAP_INTERNAL_PORT capability flag set.
> + *
> + * The *nb_events* parameter is the number of event objects to enqueue which are
> + * supplied in the *ev* array of *rte_event* structure.
> + *
> + * The rte_event_eth_tx_adapter_enqueue() function returns the number of
> + * events objects it actually enqueued. A return value equal to *nb_events*
> + * means that all event objects have been enqueued.
> + *
> + * @param id
> + *  The identifier of the tx adapter.
> + * @param dev_id
> + *  The identifier of the device.
> + * @param port_id
> + *  The identifier of the event port.
> + * @param ev
> + *  Points to an array of *nb_events* objects of type *rte_event* structure
> + *  which contain the event object enqueue operations to be processed.
> + * @param nb_events
> + *  The number of event objects to enqueue, typically number of
> + *  rte_event_port_enqueue_depth() available for this port.
> + *
> + * @return
> + *   The number of event objects actually enqueued on the event device. The
> + *   return value can be less than the value of the *nb_events* parameter when
> + *   the event devices queue is full or if invalid parameters are specified in a
> + *   *rte_event*. If the return value is less than *nb_events*, the remaining
> + *   events at the end of ev[] are not consumed and the caller has to take care
> + *   of them, and rte_errno is set accordingly. Possible errno values include:
> + *   - -EINVAL  The port ID is invalid, device ID is invalid, an event's queue
> + *              ID is invalid, or an event's sched type doesn't match the
> + *              capabilities of the destination queue.
> + *   - -ENOSPC  The event port was backpressured and unable to enqueue
> + *              one or more events. This error code is only applicable to
> + *              closed systems.
> + */
> +static inline uint16_t __rte_experimental
> +rte_event_eth_tx_adapter_enqueue(uint8_t id, uint8_t dev_id,
> +                               uint8_t port_id,
> +                               struct rte_event ev[],
> +                               uint16_t nb_events)
> +{
> +       const struct rte_eventdev *dev = &rte_eventdevs[dev_id];
> +
> +       return __rte_event_eth_tx_adapter_enqueue(id, dev_id, port_id, ev,
> +                                               nb_events,
> +                                               dev->txa_enqueue);

As per above, Since the function call logic is simplified you can add the
above function logic here.

> +}
> +
> index dabb12d..ab23503 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -388,6 +388,11 @@ F: lib/librte_eventdev/*crypto_adapter*
>  F: test/test/test_event_crypto_adapter.c
>  F: doc/guides/prog_guide/event_crypto_adapter.rst
> 
> +Eventdev Ethdev Tx Adapter API - EXPERIMENTAL
> +M: Nikhil Rao <nikhil.rao@intel.com>
> +T: git://dpdk.org/next/dpdk-next-eventdev
> +F: lib/librte_eventdev/*eth_tx_adapter*

Add the testcase also.

Overall it looks good. No more comments on specification.

> +
>  Raw device API - EXPERIMENTAL
>  M: Shreyansh Jain <shreyansh.jain@nxp.com>
>  M: Hemant Agrawal <hemant.agrawal@nxp.com>
> --
> 1.8.3.1
>