Re: [dpdk-dev] [PATCH v5 2/3] ethdev: add optional rxtx callback support

[Thomas Monjalon <thomas.monjalon@6wind.com>]

Hi John,

2015-02-20 17:03, John McNamara:
> From: Richardson, Bruce <bruce.richardson@intel.com>
> 
> Add optional support for inline processing of packets inside the RX
> or TX call. For an RX callback, what happens is that we get a set of
> packets from the NIC and then pass them to a callback function, if
> configured, to allow additional processing to be done on them, e.g.
> filling in more mbuf fields, before passing back to the application.
> On TX, the packets are similarly post-processed before being handed
> to the NIC for transmission.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: John McNamara <john.mcnamara@intel.com>
[...]
> +#ifdef RTE_ETHDEV_RXTX_CALLBACKS
> +void *
> +rte_eth_add_rx_callback(uint8_t port_id, uint16_t queue_id,
> +		rte_rxtx_callback_fn fn, void *user_param)
> +{
> +	/* check input parameters */
> +	if (port_id >= nb_ports || fn == NULL ||
> +		    queue_id >= rte_eth_devices[port_id].data->nb_rx_queues) {
> +		rte_errno = EINVAL;
> +		return NULL;
> +	}

Why not putting #ifdef only here and return an error ENOTSUP?

> +	struct rte_eth_rxtx_callback *cb = rte_zmalloc(NULL, sizeof(*cb), 0);
> +
> +	if (cb == NULL) {
> +		rte_errno = ENOMEM;
> +		return NULL;
> +	}
> +
> +	cb->fn = fn;
> +	cb->param = user_param;
> +	cb->next = rte_eth_devices[port_id].post_rx_burst_cbs[queue_id];
> +	rte_eth_devices[port_id].post_rx_burst_cbs[queue_id] = cb;
> +	return cb;
> +}
[...]
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -1522,6 +1522,49 @@ struct eth_dev_ops {
>  	eth_filter_ctrl_t              filter_ctrl;          /**< common filter control*/
>  };
>  
> +#ifdef RTE_ETHDEV_RXTX_CALLBACKS
> +/**
> + * Function type used for callbacks for processing packets on RX and TX
> + *
> + * If configured for RX, it is called with a burst of packets that have just
> + * been received on the given port and queue. On TX, it is called with a burst
> + * of packets immediately before those packets are put onto the hardware queue
> + * for transmission.
> + *
> + * @param port
> + *   The ethernet port on which rx or tx is being performed
> + * @param queue
> + *   The queue on the ethernet port which is being used to receive or transmit
> + *   the packets.
> + * @param pkts
> + *   The burst of packets on which processing is to be done. On RX, these
> + *   packets have just been received. On TX, they are about to be transmitted.
> + * @param nb_pkts
> + *   The number of packets in the burst pointed to by "pkts"
> + * @param user_param
> + *   The arbitrary user parameter passed in by the application when the callback
> + *   was originally configured.
> + * @return
> + *   The number of packets remaining in pkts are processing.
> + *	* On RX, this will be returned to the user as the return value from
> + *	  rte_eth_rx_burst.
> + *	* On TX, this will be the number of packets actually written to the NIC.
> + */
> +typedef uint16_t (*rte_rxtx_callback_fn)(uint8_t port, uint16_t queue,
> +	struct rte_mbuf *pkts[], uint16_t nb_pkts, void *user_param);
> +
> +/**
> + * @internal
> + * Structure used to hold information about the callbacks to be called for a
> + * queue on RX and TX.
> + */
> +struct rte_eth_rxtx_callback {
> +	struct rte_eth_rxtx_callback *next;
> +	rte_rxtx_callback_fn fn;
> +	void *param;
> +};
> +#endif
> +
>  /**
>   * @internal
>   * The generic data structure associated with each ethernet device.
> @@ -1539,7 +1582,22 @@ struct rte_eth_dev {
>  	const struct eth_driver *driver;/**< Driver for this device */
>  	struct eth_dev_ops *dev_ops;    /**< Functions exported by PMD */
>  	struct rte_pci_device *pci_dev; /**< PCI info. supplied by probing */
> -	struct rte_eth_dev_cb_list link_intr_cbs; /**< User application callbacks on interrupt*/
> +	/** User application callbacks for NIC interrupts */
> +	struct rte_eth_dev_cb_list link_intr_cbs;
> +
> +#ifdef RTE_ETHDEV_RXTX_CALLBACKS
> +	/**
> +	 * User-supplied functions called from rx_burst to post-process
> +	 * received packets before passing them to the user
> +	 */
> +	struct rte_eth_rxtx_callback **post_rx_burst_cbs;
> +
> +	/**
> +	 * User-supplied functions called from tx_burst to pre-process
> +	 * received packets before passing them to the driver for transmission.
> +	 */
> +	struct rte_eth_rxtx_callback **pre_tx_burst_cbs;
> +#endif
>  };

Generally, I think it's a bad idea to put #ifdef in API (structs or functions).

>  struct rte_eth_dev_sriov {
> @@ -2393,7 +2451,23 @@ rte_eth_rx_burst(uint8_t port_id, uint16_t queue_id,
>  	struct rte_eth_dev *dev;
>  
>  	dev = &rte_eth_devices[port_id];
> +
> +#ifndef RTE_ETHDEV_RXTX_CALLBACKS
>  	return (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id], rx_pkts, nb_pkts);
> +#else
> +	nb_pkts = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id], rx_pkts,
> +			nb_pkts);

Why not #ifdef only from here...

> +	struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
> +
> +	if (unlikely(cb != NULL)) {
> +		do {
> +			nb_pkts = cb->fn(port_id, queue_id, rx_pkts, nb_pkts,
> +					cb->param);
> +			cb = cb->next;
> +		} while (cb != NULL);
> +	}

... to here?

> +	return nb_pkts;
> +#endif
>  }
>  #endif
>  
> @@ -2520,6 +2594,17 @@ rte_eth_tx_burst(uint8_t port_id, uint16_t queue_id,
>  	struct rte_eth_dev *dev;
>  
>  	dev = &rte_eth_devices[port_id];
> +#ifdef RTE_ETHDEV_RXTX_CALLBACKS
> +	struct rte_eth_rxtx_callback *cb = dev->pre_tx_burst_cbs[queue_id];
> +
> +	if (unlikely(cb != NULL)) {
> +		do {
> +			nb_pkts = cb->fn(port_id, queue_id, tx_pkts, nb_pkts,
> +					cb->param);
> +			cb = cb->next;
> +		} while (cb != NULL);
> +	}
> +#endif
>  	return (*dev->tx_pkt_burst)(dev->data->tx_queues[queue_id], tx_pkts, nb_pkts);
>  }
>  #endif
> @@ -3667,6 +3752,123 @@ int rte_eth_dev_filter_supported(uint8_t port_id, enum rte_filter_type filter_ty
>  int rte_eth_dev_filter_ctrl(uint8_t port_id, enum rte_filter_type filter_type,
>  			enum rte_filter_op filter_op, void *arg);
>  
> +#ifdef RTE_ETHDEV_RXTX_CALLBACKS
> +/**
> + * Add a callback to be called on packet RX on a given port and queue.
> + *
> + * This API configures a function to be called for each burst of
> + * packets received on a given NIC port queue. The return value is a pointer
> + * that can be used to later remove the callback using
> + * rte_eth_remove_rx_callback().
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param queue_id
> + *   The queue on the Ethernet device on which the callback is to be added.
> + * @param fn
> + *   The callback function
> + * @param user_param
> + *   A generic pointer parameter which will be passed to each invocation of the
> + *   callback function on this port and queue.
> + *
> + * @return
> + *   NULL on error.
> + *   On success, a pointer value which can later be used to remove the callback.
> + */
> +void *rte_eth_add_rx_callback(uint8_t port_id, uint16_t queue_id,
> +		rte_rxtx_callback_fn fn, void *user_param);
> +
> +/**
> + * Add a callback to be called on packet TX on a given port and queue.
> + *
> + * This API configures a function to be called for each burst of
> + * packets sent on a given NIC port queue. The return value is a pointer
> + * that can be used to later remove the callback using
> + * rte_eth_remove_tx_callback().
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param queue_id
> + *   The queue on the Ethernet device on which the callback is to be added.
> + * @param fn
> + *   The callback function
> + * @param user_param
> + *   A generic pointer parameter which will be passed to each invocation of the
> + *   callback function on this port and queue.
> + *
> + * @return
> + *   NULL on error.
> + *   On success, a pointer value which can later be used to remove the callback.
> + */
> +void *rte_eth_add_tx_callback(uint8_t port_id, uint16_t queue_id,
> +		rte_rxtx_callback_fn fn, void *user_param);
> +
> +/**
> + * Remove an RX packet callback from a given port and queue.
> + *
> + * This function is used to removed callbacks that were added to a NIC port
> + * queue using rte_eth_add_rx_callback().
> + *
> + * Note: the callback is removed from the callback list but it isn't freed
> + * since the it may still be in use. The memory for the callback can be
> + * subsequently freed back by the application by calling rte_free():
> + *
> + *  - Immediately - if the port is stopped, or the user knows that no
> + *    callbacks are in flight e.g. if called from the thread doing RX/TX
> + *    on that queue.
> + *
> + * - After a short delay - where the delay is sufficient to allow any
> + *   in-flight callbacks to complete.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param queue_id
> + *   The queue on the Ethernet device from which the callback is to be removed.
> + * @param user_cb
> + *   User supplied callback created via rte_eth_add_rx_callback().
> + *
> + * @return
> + *   - 0: Success. Callback was removed.
> + *   - -EINVAL: The port_id or the queue_id is out of range, or the callback is
> + *              NULL or not found for the port/queue.
> + */
> +int rte_eth_remove_rx_callback(uint8_t port_id, uint16_t queue_id,
> +		struct rte_eth_rxtx_callback *user_cb);
> +
> +/**
> + * Remove a TX packet callback from a given port and queue.
> + *
> + * This function is used to removed callbacks that were added to a NIC port
> + * queue using rte_eth_add_tx_callback().
> + *
> + * Note: the callback is removed from the callback list but it isn't freed
> + * since the it may still be in use. The memory for the callback can be
> + * subsequently freed back by the application by calling rte_free():
> + *
> + *  - Immediately - if the port is stopped, or the user knows that no
> + *    callbacks are in flight e.g. if called from the thread doing RX/TX
> + *    on that queue.
> + *
> + * - After a short delay - where the delay is sufficient to allow any
> + *   in-flight callbacks to complete.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param queue_id
> + *   The queue on the Ethernet device from which the callback is to be removed.
> + * @param user_cb
> + *   User supplied callback created via rte_eth_add_tx_callback().
> + *
> + * @return
> + *   - 0: Success. Callback was removed.
> + *   - -EINVAL: The port_id or the queue_id is out of range, or the callback is
> + *              NULL or not found for the port/queue.
> + */
> +int rte_eth_remove_tx_callback(uint8_t port_id, uint16_t queue_id,
> +		struct rte_eth_rxtx_callback *user_cb);
> +
> +#endif /* RTE_ETHDEV_RXTX_CALLBACKS */

Please avoid #ifdef around function declarations.

Thanks