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

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

> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Tuesday, July 10, 2018 5:48 PM
> To: Rao, Nikhil <nikhil.rao@intel.com>
> Cc: olivier.matz@6wind.com; dev@dpdk.org; anoob.joseph@cavium.com
> Subject: Re: [PATCH 1/4] eventdev: add eth Tx adapter APIs
> 
> ---
> 
> 1) Update doc/api/doxy-api-index.md
OK.

> 2) Update lib/librte_eventdev/Makefile
> +SYMLINK-y-include += rte_event_eth_tx_adapter.h
> 
This is done in patch 3 of this series.

> 
> 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.
> 
Fine with me.

> 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
>
OK, if these fields are not going to be used within the other adapter, I will move these to the .c file.
 
> > +/**
> > + * @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)?
OK.
This would also move the private definition of   struct txa_mbuf_txq_id  to the adapter header file, which would be needed to deprecated once the field is
available in rte_mbuf.h.

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

> > +
> > +/**
> > + * @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)
> 
> 
OK.

> > +
> > +#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.
> 
OK, I will also delete the id parameter.

> > +}
> > +
> > 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.
> 
I have made that update in patch 4 of this series.

> Overall it looks good. No more comments on specification.
> 

Thanks for the review,
Nikhil