[dpdk-dev] [RFC PATCH 2/4] Add the new common device header and C file.

Neil Horman nhorman at tuxdriver.com
Thu Apr 9 13:53:01 CEST 2015


On Wed, Apr 08, 2015 at 03:58:38PM -0500, Keith Wiles wrote:
> Move a number of device specific define, structures and functions
> into a generic device base set of files for all device not just Ethernet.
> 
> Signed-off-by: Keith Wiles <keith.wiles at intel.com>
> ---
>  lib/librte_eal/common/eal_common_device.c         | 185 +++++++
>  lib/librte_eal/common/include/rte_common_device.h | 617 ++++++++++++++++++++++
>  2 files changed, 802 insertions(+)
>  create mode 100644 lib/librte_eal/common/eal_common_device.c
>  create mode 100644 lib/librte_eal/common/include/rte_common_device.h
> 
> diff --git a/lib/librte_eal/common/eal_common_device.c b/lib/librte_eal/common/eal_common_device.c
> new file mode 100644
> index 0000000..a9ef925
> --- /dev/null
> +++ b/lib/librte_eal/common/eal_common_device.c
> @@ -0,0 +1,185 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
> + *   Copyright(c) 2014 6WIND S.A.
> + *   All rights reserved.
> + *
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in
> + *       the documentation and/or other materials provided with the
> + *       distribution.
> + *     * Neither the name of Intel Corporation nor the names of its
> + *       contributors may be used to endorse or promote products derived
> + *       from this software without specific prior written permission.
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <stdio.h>
> +#include <string.h>
> +#include <inttypes.h>
> +#include <sys/queue.h>
> +
> +#include <rte_dev.h>
> +#include <rte_devargs.h>
> +#include <rte_debug.h>
> +#include <rte_devargs.h>
> +#include <rte_log.h>
> +#include <rte_malloc.h>
> +#include <rte_errno.h>
> +
> +#include "rte_common_device.h"
> +
> +void *
> +rte_dev_add_callback(struct rte_dev_rxtx_callback ** cbp,
> +		void * fn, void *user_param)
> +{
> +	struct rte_dev_rxtx_callback *cb;
> +
> +	cb = rte_zmalloc(NULL, sizeof(*cb), 0);
> +
> +	if (cb == NULL) {
> +		rte_errno = ENOMEM;
> +		return NULL;
> +	}
> +
> +	cb->fn.vp = fn;
> +	cb->param = user_param;
> +	cb->next = *cbp;
> +	*cbp = cb;
> +	return cb;
> +}
> +
> +int
> +rte_dev_remove_callback(struct rte_dev_rxtx_callback ** cbp,
> +		struct rte_dev_rxtx_callback *user_cb)
> +{
> +	struct rte_dev_rxtx_callback *cb = *cbp;
> +	struct rte_dev_rxtx_callback *prev_cb;
> +
> +	/* Reset head pointer and remove user cb if first in the list. */
> +	if (cb == user_cb) {
> +		*cbp = user_cb->next;
> +		return 0;
> +	}
> +
> +	/* Remove the user cb from the callback list. */
> +	do {
> +		prev_cb = cb;
> +		cb = cb->next;
> +
> +		if (cb == user_cb) {
> +			prev_cb->next = user_cb->next;
> +			return 0;
> +		}
> +	} while (cb != NULL);
> +
> +	/* Callback wasn't found. */
> +	return (-EINVAL);
> +}
> +
> +int
> +rte_dev_callback_register(struct rte_dev_cb_list * cb_list,
> +		rte_spinlock_t * lock,
> +		enum rte_dev_event_type event,
> +		rte_dev_cb_fn cb_fn, void *cb_arg)
> +{
> +	struct rte_dev_callback *cb;
> +
> +	rte_spinlock_lock(lock);
> +
> +	TAILQ_FOREACH(cb, cb_list, next) {
> +		if (cb->cb_fn == cb_fn &&
> +			cb->cb_arg == cb_arg &&
> +			cb->event == event) {
> +			break;
> +		}
> +	}
> +
> +	/* create a new callback. */
> +	if (cb == NULL && (cb = rte_zmalloc("INTR_USER_CALLBACK",
> +			sizeof(struct rte_dev_callback), 0)) != NULL) {
> +		cb->cb_fn = cb_fn;
> +		cb->cb_arg = cb_arg;
> +		cb->event = event;
> +		TAILQ_INSERT_TAIL(cb_list, cb, next);
> +	}
> +
> +	rte_spinlock_unlock(lock);
> +	return ((cb == NULL) ? -ENOMEM : 0);
> +}
> +
> +int
> +rte_dev_callback_unregister(struct rte_dev_cb_list * cb_list,
> +		rte_spinlock_t * lock,
> +		enum rte_dev_event_type event,
> +		rte_dev_cb_fn cb_fn, void *cb_arg)
> +{
> +	int ret;
> +	struct rte_dev_callback *cb, *next;
> +
> +	rte_spinlock_lock(lock);
> +
> +	ret = 0;
> +	for (cb = TAILQ_FIRST(cb_list); cb != NULL; cb = next) {
> +
> +		next = TAILQ_NEXT(cb, next);
> +
> +		if (cb->cb_fn != cb_fn || cb->event != event ||
> +				(cb->cb_arg != (void *)-1 &&
> +				cb->cb_arg != cb_arg))
> +			continue;
> +
> +		/*
> +		 * if this callback is not executing right now,
> +		 * then remove it.
> +		 */
> +		if (cb->active == 0) {
> +			TAILQ_REMOVE(cb_list, cb, next);
> +			rte_free(cb);
> +		} else {
> +			ret = -EAGAIN;
> +		}
> +	}
> +
> +	rte_spinlock_unlock(lock);
> +	return (ret);
> +}
> +
> +void
> +rte_dev_callback_process(struct rte_dev_cb_list * cb_list, uint8_t port_id,
> +	enum rte_dev_event_type event, rte_spinlock_t *lock)
> +{
> +	struct rte_dev_callback *cb;
> +	struct rte_dev_callback dev_cb;
> +
> +	rte_spinlock_lock(lock);
> +	TAILQ_FOREACH(cb, cb_list, next) {
> +		if (cb->cb_fn == NULL || cb->event != event)
> +			continue;
> +		dev_cb = *cb;
> +		cb->active = 1;
> +		rte_spinlock_unlock(lock);
> +		dev_cb.cb_fn(port_id, dev_cb.event, dev_cb.cb_arg);
> +		rte_spinlock_lock(lock);
> +		cb->active = 0;
> +	}
> +	rte_spinlock_unlock(lock);
> +}


This is a bit...odd.   The rte_eth callbacks had some context because you knew
when the callback was going to be issued (at the end of an rx and tx operation).
Here, by making the process routine generic, you're removed that context, and so
the caller has no guarantee as to when their callbacks will be called.  If its
to be done at the end of the generic transmit and routine functions, that would
be fine, but I don't see that happening here.
 
> diff --git a/lib/librte_eal/common/include/rte_common_device.h b/lib/librte_eal/common/include/rte_common_device.h
> new file mode 100644
> index 0000000..5baefb6
> --- /dev/null
> +++ b/lib/librte_eal/common/include/rte_common_device.h
> @@ -0,0 +1,617 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
> + *   All rights reserved.
> + *
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in
> + *       the documentation and/or other materials provided with the
> + *       distribution.
> + *     * Neither the name of Intel Corporation nor the names of its
> + *       contributors may be used to endorse or promote products derived
> + *       from this software without specific prior written permission.
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHPDF IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef _RTE_COMMON_DEVICE_H_
> +#define _RTE_COMMON_DEVICE_H_
> +
> +/**
> + * @file
> + *
> + * Common Device Helpers in RTE
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <rte_spinlock.h>
> +
> +/* Macros for checking for restricting functions to primary instance only */
> +#define PROC_PRIMARY_OR_ERR_RET(retval) do { \
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY) { \
> +		PMD_DEBUG_TRACE("Cannot run in secondary processes\n"); \
> +		return (retval); \
> +	} \
> +} while(0)
> +#define PROC_PRIMARY_OR_RET() do { \
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY) { \
> +		PMD_DEBUG_TRACE("Cannot run in secondary processes\n"); \
> +		return; \
> +	} \
> +} while(0)
> +
> +/* Macros to check for invalid function pointers in dev_ops structure */
> +#define FUNC_PTR_OR_ERR_RET(func, retval) do { \
> +	if ((func) == NULL) { \
> +		PMD_DEBUG_TRACE("Function not supported\n"); \
> +		return (retval); \
> +	} \
> +} while(0)
> +#define FUNC_PTR_OR_RET(func) do { \
> +	if ((func) == NULL) { \
> +		PMD_DEBUG_TRACE("Function not supported\n"); \
> +		return; \
> +	} \
> +} while(0)
> +
> +enum {
> +	DEV_DETACHED = 0,
> +	DEV_ATTACHED
> +};
> +
> +/*
> + * The device type
> + */
> +enum rte_dev_type {
> +	RTE_DEV_UNKNOWN,	/**< unknown device type */
> +	RTE_DEV_PCI,
> +		/**< Physical function and Virtual function of PCI devices */
> +	RTE_DEV_VIRTUAL,	/**< non hardware device */
> +	RTE_DEV_MAX			/**< max value of this enum */
> +};
> +
> +/**
> + * The device event type for interrupt, and maybe others in the future.
> + */
> +enum rte_dev_event_type {
> +	RTE_DEV_EVENT_UNKNOWN,  /**< unknown event type */
> +	RTE_DEV_EVENT_INTR_LSC, /**< lsc interrupt event */
> +	RTE_DEV_EVENT_MAX       /**< max value of this enum */
> +};
> +
> +struct rte_dev_callback;
> +
> +/** @internal Structure to keep track of registered callback */
> +TAILQ_HEAD(rte_dev_cb_list, rte_dev_callback);
> +
> +struct rte_mbuf;
> +
> +typedef uint16_t (*dev_rx_burst_t)(void *rxq,
> +				   struct rte_mbuf **rx_pkts,
> +				   uint16_t nb_pkts);
> +/**< @internal Retrieve input packets from a receive queue of a device. */
> +
> +typedef uint16_t (*dev_tx_burst_t)(void *txq,
> +				   struct rte_mbuf **tx_pkts,
> +				   uint16_t nb_pkts);
> +/**< @internal Send output packets on a transmit queue of a device. */
> +
> +typedef void (*rte_dev_cb_fn)(uint8_t port_id, \
> +		enum rte_dev_event_type event, void *cb_arg);
> +/**< user application callback to be registered for interrupts */
> +
> +#define RTE_DEV_NAME_MAX_LEN (32)
> +
> +/**
> + * @internal
> + * Common set of members for all devices included at the top of 'struct rte_xyz_dev'
> + */
> +#define RTE_COMMON_DEV(_t)                                                              	\
> +    dev_rx_burst_t              rx_pkt_burst;   /**< Pointer to PMD receive function. */    \
> +    dev_tx_burst_t              tx_pkt_burst;   /**< Pointer to PMD transmit function. */   \
> +    struct rte_##_t##dev_data   *data;          /**< Pointer to device data */              \
> +    struct rte_##_t##global     *gbl_data;      /**< Pointer to device global data */       \
> +    const struct _t##driver     *driver;        /**< Driver for this device */              \
> +    struct _t##dev_ops          *dev_ops;       /**< Functions exported by PMD */           \

This doesn't make any sense to me.  You've made a generic macro that creates
what is ostensibly supposed to be common code point, but then you include some
string concatenation so that the common parts are actually unique to a given
device class.  Why would you do that?  At that point the common parts aren't
common by definition, and should go in a structure specific to that device class

> +    struct rte_pci_device       *pci_dev;       /**< PCI info. supplied by probing */       \
> +    /** User application callback for interrupts if present */                              \
> +    struct rte_dev_cb_list   link_intr_cbs;                                          		\
> +    /**                                                                                     \
> +     * User-supplied functions called from rx_burst to post-process                         \
> +     * received packets before passing them to the user                                     \
> +     */                                                                                     \
> +    struct rte_dev_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_dev_rxtx_callback **pre_tx_burst_cbs;                                     	\
> +    enum rte_dev_type       	dev_type;       /**< Flag indicating the device type */     \
> +    uint8_t                     attached        /**< Flag indicating the port is attached */
> +

I'm also confused here.  Looking at this, this is effectively a complete
renaming of the rte_eth_dev device structure.  Once again, why not just leave
rte_eth_dev alone, and develop the crypto device from scratch?  If your intent
is for it to have so much in common with an ethernet device that you can
effectively just rename the existing structure, why not just call it an ethernet
device?

> +/**
> + * @internal
> + * Common set of members for all devices included at the top of 'struct rte_xyz_dev_data'
> + */
> +#define RTE_COMMON_DEV_DATA                                                 	\
> +    char name[RTE_DEV_NAME_MAX_LEN]; /**< Unique identifier name */             \
> +                                                                                \
> +    void **rx_queues;               /**< Array of pointers to RX queues. */     \
> +    void **tx_queues;               /**< Array of pointers to TX queues. */     \
> +    uint16_t nb_rx_queues;          /**< Number of RX queues. */                \
> +    uint16_t nb_tx_queues;          /**< Number of TX queues. */                \
> +                                                                                \
> +    uint16_t mtu;                   /**< Maximum Transmission Unit. */          \
> +    uint8_t unit_id;                /**< Unit/Port ID for this instance */      \
> +    uint8_t _pad0;             		/* alignment filler */                      \
> +                                                                                \
> +    /* 64bit alignment starts here */                                           \
> +    void    *dev_private;           /**< PMD-specific private data */           \
> +    uint64_t rx_mbuf_alloc_failed;  /**< RX ring mbuf allocation failures. */   \
> +    uint32_t min_rx_buf_size;       /**< Common rx buffer size handled by all queues */ \
> +    uint32_t _pad1					/**< Align to a 64bit boundary */
> +
> +/**
> + * @internal
> + * Common set of members for all devices included at the top of 'struct rte_xyz_dev_info'
> + */
> +#define RTE_COMMON_DEV_INFO                                                 	\
> +    struct rte_pci_device   *pci_dev;       /**< Device PCI information. */     \
> +    const char              *driver_name;   /**< Device Driver name. */         \
> +    unsigned int            if_index;       /**< Index to bound host interface, or 0 if none. */ \
> +        /* Use if_indextoname() to translate into an interface name. */         \
> +    uint32_t _pad0
> +
> +#define port_id		unit_id
> +
> +/**
> + * @internal
> + * Common set of members for all devices included at the top of 'struct rte_xyz_global'
> + */
> +#define RTE_COMMON_GLOBAL(_t)                                           \
> +    struct rte_##_t##dev * devs;/**< Device information array */       \
> +    struct rte_##_t##dev_data * data;  /**< Device private data */     \
Again, if you're going to make something common, It doesn't make sense to then
give it a name that is going to be unique to the device class instantiating the
macro.

> +    uint8_t     nb_ports;        /**< Number of ports/units found */    \
> +    uint8_t     max_ports;       /**< Max number of ports or units */   \
> +    uint16_t    dflt_mtu;        /**< Default MTU if needed by device */\
> +    uint16_t    dev_size;        /**< Internal size of device struct */	\
> +    uint16_t    data_size;       /**< Internal size of data structure */\
> +    const char * mz_dev_data     /**< String constant for device data */
> +
> +/**
> + * @internal
> + * Common overlay type structures with all devices.
> + */
> +struct rte_dev_info {
> +    RTE_COMMON_DEV_INFO;
> +};
> +
> +/**
> + * @internal
> + * The generic data structure associated with each device.
> + *
> + * Pointers to burst-oriented packet receive and transmit functions are
> + * located at the beginning of the structure, along with the pointer to
> + * where all the data elements for the particular device are stored in shared
> + * memory. This split allows the function pointer and driver data to be per-
> + * process, while the actual configuration data for the device is shared.
> + */
> +struct rte_dev {
> +    RTE_COMMON_DEV();
> +};
> +
> +/**
> + * @internal
> + * The data part, with no function pointers, associated with each device.
> + *
> + * This structure is safe to place in shared memory to be common among different
> + * processes in a multi-process configuration.
> + */
> +struct rte_dev_data {
> +    RTE_COMMON_DEV_DATA;
> +};
> +
> +/**
> + * @internal
> + * This structure is attempting to mirror the rte_xyz_global structures.
> + *
> + * Be careful as this structure is over layered on top of the xyzdev structures.
> + */
> +struct rte_dev_global {
> +    RTE_COMMON_GLOBAL();
> +};
> +
I don't understand, you created macros that allow for naming specifics, then
don't use it?  If your intent was for that data to be generically named (which
would make sense), then you should remove the parameter from the macro so that
can't change.

> +/**
> + * Get the rte_pkt_dev structure device pointer for the device.
> + *
> + * @param   gbl     pointer to device specific global structure.
> + * @param   port_id Port ID value to select the device structure.
> + *
> + * @return
> + *   - The rte_pkt_dev structure pointer for the given port ID.
> + */
> +static inline void *
> +rte_get_dev(void * _gbl, uint8_t port_id)
> +{
> +    struct rte_dev_global * gbl = (struct rte_dev_global *)_gbl;
> +
> +    return (void *)((uint8_t *)gbl->devs + (port_id * gbl->dev_size));
> +}
> +
How will this be helpful?  the caller will need to know what type of struct to
cast this to (since it can be named different for different dev classes).
doesn't that sort of thing undo the notion of commonality?

Neil



More information about the dev mailing list