Re: [dpdk-dev] [RFC PATCH 1/4 v2] Adding the common device files for multiple device support

["Wiles, Keith" <keith.wiles@intel.com>]

Hi Marc

On 5/4/15, 6:13 AM, "Marc Sune" <marc.sune@bisdn.de> wrote:

>
>
>On 13/04/15 21:44, Keith Wiles wrote:
>> Add the eal_common_device.c and rte_common_device.h and include the
>> build support changes.
>>
>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
>> ---
>>   lib/librte_eal/bsdapp/eal/Makefile                |   1 +
>>   lib/librte_eal/common/Makefile                    |   1 +
>>   lib/librte_eal/common/eal_common_device.c         | 185 ++++++
>>   lib/librte_eal/common/include/rte_common_device.h | 674
>>++++++++++++++++++++++
>>   lib/librte_eal/common/include/rte_log.h           |   1 +
>>   lib/librte_eal/linuxapp/eal/Makefile              |   1 +
>>   lib/librte_kni/rte_kni.c                          |   4 +-
>>   7 files changed, 865 insertions(+), 2 deletions(-)
>>   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/bsdapp/eal/Makefile
>>b/lib/librte_eal/bsdapp/eal/Makefile
>> index 2357cfa..7bb2689 100644
>> --- a/lib/librte_eal/bsdapp/eal/Makefile
>> +++ b/lib/librte_eal/bsdapp/eal/Makefile
>> @@ -78,6 +78,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) +=
>>eal_common_devargs.c
>>   SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) += eal_common_dev.c
>>   SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) += eal_common_options.c
>>   SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) += eal_common_thread.c
>> +SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) += eal_common_device.c
>>   
>>   CFLAGS_eal.o := -D_GNU_SOURCE
>>   #CFLAGS_eal_thread.o := -D_GNU_SOURCE
>> diff --git a/lib/librte_eal/common/Makefile
>>b/lib/librte_eal/common/Makefile
>> index 3ea3bbf..c4bf805 100644
>> --- a/lib/librte_eal/common/Makefile
>> +++ b/lib/librte_eal/common/Makefile
>> @@ -40,6 +40,7 @@ INC += rte_string_fns.h rte_version.h
>>   INC += rte_eal_memconfig.h rte_malloc_heap.h
>>   INC += rte_hexdump.h rte_devargs.h rte_dev.h
>>   INC += rte_pci_dev_feature_defs.h rte_pci_dev_features.h
>> +INC += rte_common_device.h
>>   
>>   ifeq ($(CONFIG_RTE_INSECURE_FUNCTION_WARNING),y)
>>   INC += rte_warnings.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);
>> +}
>> 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..de48ad7
>> --- /dev/null
>> +++ b/lib/librte_eal/common/include/rte_common_device.h
>> @@ -0,0 +1,674 @@
>> +/*-
>> + *   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)
>> +
>> +#ifndef NULL
>> +#define NULL	(void *)0
>> +#endif
>> +
>> +/* 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)
>> +
>> +struct rte_dev;
>> +/**
>> + * @internal
>> + * Initialization function of an Ethernet driver invoked for each
>>matching
>> + * Ethernet PCI device detected during the PCI probing phase.
>> + *
>> + * @param dev
>> + *   The *dev* pointer is the address of the *rte_xyz_dev* structure
>> + *   associated with the matching device and which have been
>>[automatically]
>> + *   allocated in the *rte_eth_devices* array.
>> + *   The *eth_dev* structure is supplied to the driver initialization
>>function
>> + *   with the following fields already initialized:
>> + *
>> + *   - *pci_dev*: Holds the pointers to the *rte_pci_device* structure
>>which
>> + *     contains the generic PCI information of the matching device.
>> + *
>> + *   - *driver*: Holds the pointer to the *eth_driver* structure.
>> + *
>> + *   - *dev_private*: Holds a pointer to the device private data
>>structure.
>> + *
>> + *   - *mtu*: Contains the default Ethernet maximum frame length
>>(1500).
>> + *
>> + *   - *port_id*: Contains the port index of the device (actually the
>>index
>> + *     of the *eth_dev* structure in the *rte_eth_devices* array).
>> + *
>> + * @return
>> + *   - 0: Success, the device is properly initialized by the driver.
>> + *        In particular, the driver MUST have set up the *dev_ops*
>>pointer
>> + *        of the *dev* structure.
>> + *   - <0: Error code of the device initialization failure.
>> + */
>> +typedef int (*dev_init_t)(struct rte_dev *dev);
>> +
>> +/**
>> + * @internal
>> + * Finalization function of an Ethernet driver invoked for each
>>matching
>> + * Ethernet PCI device detected during the PCI closing phase.
>> + *
>> + * @param dev
>> + *   The *dev* pointer is the address of the *rte_eth_dev* structure
>> + *   associated with the matching device and which have been
>>[automatically]
>> + *   allocated in the *rte_eth_devices* array.
>> + * @return
>> + *   - 0: Success, the device is properly finalized by the driver.
>> + *        In particular, the driver MUST free the *dev_ops* pointer
>> + *        of the *eth_dev* structure.
>> + *   - <0: Error code of the device initialization failure.
>> + */
>> +typedef int (*dev_uninit_t)(struct rte_dev *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 {
>> +    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 */
>
>Besides port_id, what is the other usage of unit_id so that it has to be
>in the common place?

The unit_id seemed more generic to me then port_id as not all devices will
have a port, but could have more then one unit with in the device.

>
>> +    uint8_t  _pad0;             	/* alignment filler */
>> +
>> +    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 */
>
>The same way; is this really common for all the abstracted devices?
>
>> +};
>> +
>> +/**
>> + * @internal
>> + * Cast the rte_xyz_dev_data pointer into rte_dev_data to retrieve the
>>value.
>> + */
>> +#define _DD(dev, m)			((struct rte_dev_data *)(dev)->data)->m
>> +#define _DD_PRIVATE(dev)	_DD(dev, dev_private)
>> +#define _DD_PORT(dev)		_DD(dev, unit_id)
>> +#define _DD_UNIT(dev)		_DD(dev, unit_id)
>
>I don't like the approach of using these MACROs, because it makes the
>code less readable. Also because _DD name is pretty generic (although
>afterwards I realised it is the acronym of Device Data).
>
>Does it really provide that much of a benefit?

The Device Data macro helped hide some of the messy stuff and help convert
the code quicker then editing ever instance. We use macros all of the time
in DPDK and this one is no exception IMO.
>
>> +
>> +/**
>> + * @internal
>> + * Common set of members for all devices'
>> + */
>> +struct rte_dev_global {
>> +    struct rte_dev 		*devs;	/**< Device information array */
>> +    struct rte_dev_data *data;	/**< Device private data */
>> +    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
>>*/
>> +	void		*gbl_private;	/**< Global private data for device to use */
>> +};
>> +
>> +/**
>> + * @internal
>> + * The structure associated with a PMD Ethernet driver.
>> + *
>> + * Each Ethernet driver acts as a PCI driver and is represented by a
>>generic
>> + * *eth_driver* structure that holds:
>> + *
>> + * - An *rte_pci_driver* structure (which must be the first field).
>> + *
>> + * - The *eth_dev_init* function invoked for each matching PCI device.
>> + *
>> + * - The *eth_dev_uninit* function invoked for each matching PCI
>>device.
>> + *
>> + * - The size of the private data to allocate for each matching device.
>> + */
>> +struct rte_dev_drv {
>> +	struct rte_pci_driver pci_drv;  /**< The PMD is also a PCI driver. */
>> +	dev_init_t dev_init;      		/**< Device init function. */
>> +	dev_uninit_t dev_uninit;  		/**< Device uninit function. */
>> +	unsigned int dev_private_size;  /**< Size of device private data. */
>> +};
>> +
>> +/**
>> + * @internal
>> + * Common set of members for all devices included at the top of
>>'struct rte_xyz_dev_info'
>> + */
>> +struct rte_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. */
>> +    unsigned int	_pad0;
>> +};
>> +
>> +/**
>> + * @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 {
>> +    dev_rx_burst_t          rx_pkt_burst;   /**< Pointer to PMD
>>receive function. */
>> +    dev_tx_burst_t          tx_pkt_burst;   /**< Pointer to PMD
>>transmit function. */
>> +    void				  	*data;      	/**< Pointer to device data */
>> +    struct rte_dev_global	*gbl_data;      /**< Pointer to device
>>global data */
>
>Why is this gbl_data really necessary?
>
>I think I am missing something fundamental here...

As we add more and more devices you will start to see the global variables
in the device will grow, so I decided to move all of the globals into a
device specific structure to hide the device data. This is nothing new,
just some data hiding.

>
>> +    const struct rte_dev_drv *driver;       /**< Driver for this
>>device */
>> +    void					*dev_ops;       /**< Functions exported by PMD */
>> +    struct rte_pci_device   *pci_dev;       /**< PCI info. supplied by
>>probing */
>Why not in (private) data?
>> +    struct rte_dev_info		*dev_info;		/**< Pointer to the device info
>>structure */
>
>Idem.
>
>> +
>> +    /** 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 */
>> +};
>> +
>> +#define port_id		unit_id
>
>This is very dangerous and will mess user's code; why is it necessary?

We can remove it and replace every instance with unit_id, but as for
dangerous it is not. We do these types of things all of the time. Making
everyone device use port when they are not a port device seemed messy to
me.

++Keith
>
>marc
>
>> +
>> +/**
>> + * 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));
>> +}
>> +
>> +/**
>> + * Get the number of device ports in the system.
>> + *
>> + * @param gbl
>> + *  The pointer to the current global data structure for xyzdev.
>> + *
>> + * @return
>> + *   - Number of ports found in the system.
>> + */
>> +static inline uint8_t
>> +rte_pkt_dev_count(void * _gbl)
>> +{
>> +    struct rte_dev_global * gbl = (struct rte_dev_global *)_gbl;
>> +    return gbl->nb_ports;
>> +}
>> +
>> +/**
>> + * Validate if the port number is valid
>> + *
>> + * @param   gbl The pointer to the current global data structure for
>>xyzdev.
>> + * @param   port_id Port ID value to select the device.
>> + *
>> + * @return
>> + *   - Number of ports found in the system.
>> + */
>> +static inline int
>> +rte_dev_is_valid_port(void * _gbl, uint8_t port_id)
>> +{
>> +    struct rte_dev_global * gbl = (struct rte_dev_global *)_gbl;
>> +    struct rte_dev * dev;
>> +
>> +    if (port_id >= gbl->nb_ports)
>> +        return 0;
>> +
>> +    dev = rte_get_dev(gbl, port_id);
>> +    if (dev->attached != DEV_ATTACHED)
>> +        return 0;
>> +    else
>> +        return 1;
>> +}
>> +
>> +/**
>> + * Get the number of device ports in the system.
>> + *
>> + * @param gbl
>> + *  The pointer to the current global data structure for xyzdev.
>> + *
>> + * @return
>> + *   - Number of ports found in the system.
>> + */
>> +static inline uint8_t
>> +rte_dev_count(void * _gbl)
>> +{
>> +    struct rte_dev_global * gbl = (struct rte_dev_global *)_gbl;
>> +    return gbl->nb_ports;
>> +}
>> +
>> +/**
>> + * Get the rte_pkt_dev structure device pointer for the device by name.
>> + *
>> + * @param   gbl     pointer to device specific global structure.
>> + * @param   name    Name of device.
>> + *
>> + * @return
>> + *   - The rte_dev structure pointer for the given name.
>> + */
>> +static inline struct rte_dev *
>> +rte_get_named_dev(void * _gbl, const char *name)
>> +{
>> +    struct rte_dev_global * gbl = (struct rte_dev_global *)_gbl;
>> +    struct rte_dev *dev;
>> +    int i;
>> +
>> +    if (name == NULL)
>> +        return NULL;
>> +
>> +    for (i = 0; i < gbl->nb_ports; i++) {
>> +        dev = (struct rte_dev *)((uint8_t *)gbl->devs +
>> +                (i * gbl->dev_size));
>> +
>> +        if (strcmp(_DD(dev, name), name) == 0)
>> +            return dev;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +/**
>> + * Get the rte_pkt_dev structure device pointer for the PCI name.
>> + *
>> + * @param   gbl     pointer to device specific global structure.
>> + * @param   name    Name of PCI device.
>> + *
>> + * @return
>> + *   - The rte_dev structure pointer for the given PCIname.
>> + */
>> +static inline struct rte_dev *
>> +rte_get_pci_named_dev(void * _gbl, struct rte_pci_addr *pci_addr)
>> +{
>> +    struct rte_dev_global * gbl = (struct rte_dev_global *)_gbl;
>> +    struct rte_dev *dev;
>> +    int i;
>> +
>> +    if (pci_addr == NULL)
>> +        return NULL;
>> +
>> +    for (i = 0; i < gbl->nb_ports; i++) {
>> +        dev = (struct rte_dev *)((uint8_t *)gbl->devs +
>> +                (i * gbl->dev_size));
>> +
>> +        if (rte_eal_compare_pci_addr(&dev->pci_dev->addr, pci_addr) ==
>>0)
>> +            return dev;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +/*
>> + * Return the NUMA socket to which a device is connected
>> + *
>> + * @param gbl
>> + *   The global structure pointer for a given xyzdev.
>> + * @param port_id
>> + *   The port identifier of the device
>> + * @return
>> + *   The NUMA socket id to which the device is connected or
>> + *   a default of zero if the socket could not be determined.
>> + *   -1 is returned is the port_id value is out of range.
>> + */
>> +static inline int
>> +rte_dev_socket_id(void * _gbl, uint8_t port_id)
>> +{
>> +    struct rte_dev_global * gbl = (struct rte_dev_global *)_gbl;
>> +    struct rte_dev * dev;
>> +
>> +    if (!rte_dev_is_valid_port(gbl, port_id))
>> +        return -1;
>> +
>> +    dev = rte_get_dev(gbl, port_id);
>> +
>> +    if ( dev->pci_dev )
>> +        return dev->pci_dev->numa_node;
>> +    else
>> +        return 0;
>> +}
>> +/**
>> + * Function type used for RX packet processing packet a callback.
>> + *
>> + * The callback function is called on RX with a burst of packets that
>>have
>> + * been received on the given port and queue.
>> + *
>> + * @param port
>> + *   The Ethernet port on which RX is being performed.
>> + * @param queue
>> + *   The queue on the Ethernet port which is being used to receive the
>>packets.
>> + * @param pkts
>> + *   The burst of packets that have just been received.
>> + * @param nb_pkts
>> + *   The number of packets in the burst pointed to by "pkts".
>> + * @param max_pkts
>> + *   The max number of packets that can be stored in the "pkts" array.
>> + * @param user_param
>> + *   The arbitrary user parameter passed in by the application when
>>the callback
>> + *   was originally configured.
>> + * @return
>> + *   The number of packets returned to the user.
>> + */
>> +typedef uint16_t (*rte_rx_callback_fn)(uint8_t port, uint16_t queue,
>> +	struct rte_mbuf *pkts[], uint16_t nb_pkts, uint16_t max_pkts,
>> +	void *user_param);
>> +
>> +/**
>> + * Function type used for TX packet processing packet a callback.
>> + *
>> + * The callback function is called on TX with a burst of packets
>>immediately
>> + * before the packets are put onto the hardware queue for transmission.
>> + *
>> + * @param port
>> + *   The Ethernet port on which TX is being performed.
>> + * @param queue
>> + *   The queue on the Ethernet port which is being used to transmit
>>the packets.
>> + * @param pkts
>> + *   The burst of packets that 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 to be written to the NIC.
>> + */
>> +typedef uint16_t (*rte_tx_callback_fn)(uint8_t port, uint16_t queue,
>> +	struct rte_mbuf *pkts[], uint16_t nb_pkts, void *user_param);
>> +
>> +/**
>> + * @internal
>> + * Union used to hold the callback function pointers for RX and TX
>>callback.
>> + */
>> +union rte_dev_callback_fn {
>> +	void * vp;						/**< Allow for a generic function pointer to avoid
>>casting in common routines */
>> +	rte_rx_callback_fn rx;			/**< Ethernet or packet based callback
>>function for RX packets */
>> +	rte_tx_callback_fn tx;			/**< Ethernet or packet based callback
>>function for TX packets */
>> +	/**< Add other device callback prototypes here */
>> +};
>> +
>> +/**
>> + * @internal
>> + * Structure used to hold information about a callback to be called
>>for a
>> + * queue on RX and TX.
>> + */
>> +struct rte_dev_rxtx_callback {
>> +	struct rte_dev_rxtx_callback *next;
>> +	union rte_dev_callback_fn fn;
>> +	void *param;
>> +};
>> +
>> +/**
>> + * The user application callback description.
>> + *
>> + * It contains callback address to be registered by user application,
>> + * the pointer to the parameters for callback, and the event type.
>> + */
>> +struct rte_dev_callback {
>> +	TAILQ_ENTRY(rte_dev_callback) next;		/**< Callback list */
>> +	rte_dev_cb_fn cb_fn;                	/**< Callback address */
>> +	void *cb_arg;                           /**< Parameter for callback */
>> +	enum rte_dev_event_type event;          /**< Interrupt event type */
>> +	uint32_t active;                        /**< Callback is executing */
>> +};
>> +
>> +/**
>> + * Register a callback function for specific port id.
>> + *
>> + * @param cbp
>> + *  Pointer to the ret_dev_cb_list structure.
>> + * @param lock
>> + *  Pointer to the rte_spinlock_t structure.
>> + * @param event
>> + *  Event interested.
>> + * @param cb_fn
>> + *  User supplied callback function to be called.
>> + * @param cb_arg
>> + *  Pointer to the parameters for the registered callback.
>> + *
>> + * @return
>> + *  - On success, zero.
>> + *  - On failure, a negative value.
>> + */
>> +int rte_dev_callback_register(struct rte_dev_cb_list * cbp,
>>rte_spinlock_t * lock,
>> +		enum rte_dev_event_type event,
>> +		rte_dev_cb_fn cb_fn, void *cb_arg);
>> +
>> +/**
>> + * Unregister a callback function for specific port id.
>> + *
>> + * @param cb_list
>> + *  Pointer to the ret_dev_cb_list structure.
>> + * @param lock
>> + *  Pointer to the rte_spinlock_t structure.
>> + * @param event
>> + *  Event interested.
>> + * @param cb_fn
>> + *  User supplied callback function to be called.
>> + * @param cb_arg
>> + *  Pointer to the parameters for the registered callback. -1 means to
>> + *  remove all for the same callback address and same event.
>> + *
>> + * @return
>> + *  - On success, zero.
>> + *  - On failure, a negative value.
>> + */
>> +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);
>> +
>> +/**
>> + * @internal Executes all the user application registered callbacks for
>> + * the specific device. It is for DPDK internal user only. User
>> + * application should not call it directly.
>> + *
>> + * @param cb_list
>> + *  Pointer to struct rte_dev_cb_list.
>> + * @param port_id
>> + *  Port_id or unit_id value.
>> + * @param event
>> + *  Eth device interrupt event type.
>> + * @param lock
>> + *  Pointer to the lock structure.
>> + *
>> + * @return
>> + *  void
>> + */
>> +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);
>> +
>> +/**
>> + * Add a callback to be called on packet RX or TX 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_dev_remove_callback().
>> + *
>> + * @param cbp
>> + *  Pointer to a pointer of the ret_dev_cb_list structure.
>> + * @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_dev_add_callback(struct rte_dev_rxtx_callback ** cbp,
>> +		void * fn, void *user_param);
>> +
>> +/**
>> + * Remove an RX or TX packet callback from a given port and queue.
>> + *
>> + * This function is used to removed a callback that were added to a 
>>NIC port
>> + * queue using rte_dev_add_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
>> + *    callback's 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 callback's to complete.
>> + *
>> + * @param cbp
>> + *   Pointer to a pointer to use as the head of the list.
>> + * @param user_cb
>> + *   User supplied callback created via rte_dev_add_callback().
>> + *
>> + * @return
>> + *   - 0: Success. Callback was removed.
>> + *   - -ENOTSUP: Callback support is not available.
>> + *   - -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_dev_remove_callback(struct rte_dev_rxtx_callback ** cbp,
>> +		struct rte_dev_rxtx_callback *user_cb);
>> +
>> +#ifdef __cplusplus
>> +}
>> +#endif
>> +
>> +#endif /* _RTE_COMMON_DEVICE_H_ */
>> diff --git a/lib/librte_eal/common/include/rte_log.h 
>>b/lib/librte_eal/common/include/rte_log.h
>> index f83a0d9..543deb7 100644
>> --- a/lib/librte_eal/common/include/rte_log.h
>> +++ b/lib/librte_eal/common/include/rte_log.h
>> @@ -77,6 +77,7 @@ extern struct rte_logs rte_logs;
>>   #define RTE_LOGTYPE_PORT    0x00002000 /**< Log related to port. */
>>   #define RTE_LOGTYPE_TABLE   0x00004000 /**< Log related to table. */
>>   #define RTE_LOGTYPE_PIPELINE 0x00008000 /**< Log related to pipeline. 
>>*/
>> +#define RTE_LOGTYPE_DEV     0x00010000 /**< Log related to device. */
>>   
>>   /* these log types can be used in an application */
>>   #define RTE_LOGTYPE_USER1   0x01000000 /**< User-defined log type 1. 
>>*/
>> diff --git a/lib/librte_eal/linuxapp/eal/Makefile 
>>b/lib/librte_eal/linuxapp/eal/Makefile
>> index 01f7b70..935720a 100644
>> --- a/lib/librte_eal/linuxapp/eal/Makefile
>> +++ b/lib/librte_eal/linuxapp/eal/Makefile
>> @@ -90,6 +90,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += 
>>eal_common_devargs.c
>>   SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_dev.c
>>   SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_options.c
>>   SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_thread.c
>> +SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_device.c
>>   
>>   CFLAGS_eal.o := -D_GNU_SOURCE
>>   CFLAGS_eal_interrupts.o := -D_GNU_SOURCE
>> diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
>> index 4e70fa0..87ee7a3 100644
>> --- a/lib/librte_kni/rte_kni.c
>> +++ b/lib/librte_kni/rte_kni.c
>> @@ -320,8 +320,8 @@ rte_kni_create(uint8_t port_id,
>>   	rte_eth_dev_info_get(port_id, &info);
>>   
>>   	snprintf(conf.name, sizeof(conf.name), "vEth%u", port_id);
>> -	conf.addr = info.pci_dev->addr;
>> -	conf.id = info.pci_dev->id;
>> +	conf.addr = info.di.pci_dev->addr;
>> +	conf.id = info.di.pci_dev->id;
>>   	conf.group_id = (uint16_t)port_id;
>>   	conf.mbuf_size = mbuf_size;
>>   
>