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

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

-----Original Message-----
> Date: Fri, 17 Aug 2018 09:50:49 +0530
> From: Nikhil Rao <nikhil.rao@intel.com>
> To: jerin.jacob@caviumnetworks.com, olivier.matz@6wind.com
> CC: dev@dpdk.org, Nikhil Rao <nikhil.rao@intel.com>
> Subject: [PATCH v2 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.
> 
> Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>

Overall it looks good to me.

Could you please add programmers guide in the next revision?

Some minor comments below.

> ---
> 
> +/**
> + * @file
> + *
> + * RTE Event Ethernet Tx Adapter
> + *
> + * The event ethernet Tx adapter provides configuration and data path APIs
> + * for the ethernet transmit stage of an event driven packet processing
> + * application. These APIs abstract the implementation of the transmit stage
> + * and allow the application to use eventdev PMD support or a common
> + * implementation.
> + *
> + * In the common implementation, the application enqueues mbufs to the adapter
> + * which runs as a rte_service function. The service function dequeues events
> + * from its event port and transmits the mbufs referenced by these events.
> + *
> + * The ethernet Tx event adapter APIs are:
> + *
> + *  - rte_event_eth_tx_adapter_create()
> + *  - rte_event_eth_tx_adapter_create_ext()
> + *  - rte_event_eth_tx_adapter_free()
> + *  - rte_event_eth_tx_adapter_start()
> + *  - rte_event_eth_tx_adapter_stop()
> + *  - rte_event_eth_tx_adapter_queue_add()
> + *  - rte_event_eth_tx_adapter_queue_del()
> + *  - rte_event_eth_tx_adapter_stats_get()
> + *  - rte_event_eth_tx_adapter_stats_reset()
> + *  - rte_event_eth_tx_adapter_enqueue()
> + *  - rte_event_eth_tx_adapter_event_port_get()
> + *  - rte_event_eth_tx_adapter_service_id_get()
> + *
> + * The application creates the adapter using
> + * rte_event_eth_tx_adapter_create() or rte_event_eth_tx_adapter_create_ext().
> + *
> + * The adapter will use the common implementation when the eventdev PMD
> + * does not have the RTE_EVENT_ETH_TX_ADAPTER_CAP_INTERNAL_PORT capability.

Due to some reason, the generated doxygen file, does not show
RTE_EVENT_ETH_TX_ADAPTER_CAP_INTERNAL_PORT as hyperlink.


> + * The common implementation uses an event port that is created using the port
> + * configuration parameter passed to rte_event_eth_tx_adapter_create(). The
> + * application can get the port identifier using
> + * rte_event_eth_tx_adapter_event_port_get() and must link an event queue to
> + * this port.
> + *
> + * If the eventdev PMD has the RTE_EVENT_ETH_TX_ADAPTER_CAP_INTERNAL_PORT
> + * flags set, Tx adapter events should be enqueued using the
> + * rte_event_eth_tx_adapter_enqueue() function, else the application should
> + * use rte_event_enqueue_burst().
> + *
> + * Transmit queues can be added and deleted from the adapter using
> + * rte_event_eth_tx_adapter_queue_add()/del() APIs respectively.
> + *
> + * The application can start and stop the adapter using the
> + * rte_event_eth_tx_adapter_start/stop() calls.
> + *
> + * The common adapter implementation uses an EAL service function as described
> + * before and its execution is controlled using the rte_service APIs. The
> + * rte_event_eth_tx_adapter_service_id_get()
> + * function can be used to retrieve the adapter's service function ID.
> + *
> + * The ethernet port and transmit queue index to transmit the mbuf on are
> + * specified using the mbuf port and the struct rte_event_tx_adapter_mbuf_queue
> + * (overlaid on mbuf::hash). The application should use the
> + * rte_event_eth_tx_adapter_txq_set() and rte_event_eth_tx_adapter_txq_get()
> + * functions to access the transmit queue index since it is expected that the
> + * transmit queue will be eventually defined within struct rte_mbuf and using
> + * these macros will help with minimizing application impact due to
> + * a change in how the transmit queue index is specified.
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <stdint.h>
> +
> +#include <rte_mbuf.h>
> +
> +#include "rte_eventdev.h"
> +
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Set Tx queue in the mbuf. This queue is used by the  adapter
> + * to transmit the mbuf.
> + *
> + * @param pkt
> + *  Pointer to the mbuf.
> + * @param queue
> + *  Tx queue index.
> + */
> +static __rte_always_inline void __rte_experimental
> +rte_event_eth_tx_adapter_txq_set(struct rte_mbuf *pkt, uint16_t queue)
> +{
> +       struct rte_event_tx_adapter_mbuf_queue *mbuf_queue =
> +               (struct rte_event_tx_adapter_mbuf_queue *)(&pkt->hash);

It make sense to have inline function to set and get txq so that mbuf
change wont have any visible impact.

But, Can we get ride of struct rte_event_tx_adapter_mbuf_queue
typecasting/indirection?

I prefer to use just pkt->hi and add comment in rte_mbuf.h, see rte_event_eth_tx_adapter_txq_set()
or add following without breaking anything on exiting scheme.

+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -529,6 +529,11 @@ struct rte_mbuf {
                        /**< First 4 flexible bytes or FD ID, dependent
 * on
                             PKT_RX_FDIR_* flag in ol_flags. */
                } fdir;           /**< Filter identifier if FDIR enabled
*/
+               struct {
+                       uint32_t resvd1;
+                       uint16_t resvd2;
+                       uint16_t txq_id;
+               } txadapter;

Reasons:
1) Additional indirection may result in additional instruction(s) in fastpath.
2) It is kind of hiding the mbuf usage on specific variable. So consumers
may get confused on the usage and it may hide problem when mbuf gets
changed as changes are not in one place.

With above changes:

Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>