> -----Original Message----- > From: Jerin Jacob [mailto:jerin.ja...@caviumnetworks.com] > Sent: Tuesday, July 10, 2018 5:48 PM > To: Rao, Nikhil <nikhil....@intel.com> > Cc: olivier.m...@6wind.com; dev@dpdk.org; anoob.jos...@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....@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
