[dpdk-dev] [RFC PATCH 2/4] Add the new common device header and C file.
Wiles, Keith
keith.wiles at intel.com
Fri Apr 10 21:25:32 CEST 2015
On 4/9/15, 6:53 AM, "Neil Horman" <nhorman at tuxdriver.com> wrote:
>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.
No able to follow you here as this routine is called from the rte_ethdev.c
_rte_eth_dev_callback_process(). We could remove the
_rte_eth_dev_callback_process function and have the rest of the code call
this routine instead. No context is lost here and the callbacks are called
in the same location as before.
>
>> 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
The reason for these being specific to the device is the device may have
extra members private to the device. I could have added a private pointer
to the structure to make then generic to allow the device to have its own
private members.
>
>> + 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?
My goal was not to have everything common to a ethdev, but to move items
out of the ethdev we can have in common. I feel you are not understanding
something and I can not figure out what it is. If I remove the concat part
maybe this will get my point across better.
>
>> +/**
>> + * @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.
OK, will try to update the code to be non-specific to a given device.
>
>> + 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?
Not really the point was to have a routine able to return the correct
device structure pointer and the developer should understand which global
structure he is passing into the routine and what type is being returned.
>
>Neil
>
More information about the dev
mailing list