Re: [PATCH v1 01/15] eal: extract vdev infra

[Shreyansh jain <shreyansh.jain@nxp.com>]

Hi Jan,

Some comments.

On Saturday 09 July 2016 12:39 AM, Jan Viktorin wrote:
> Move all PMD_VDEV-specific code into a separate module and header
> file to not polute the generic code anymore. There is now a list
> of virtual devices available.
> 
> The rte_vdev_driver integrates the original rte_driver inside
> (C inheritance). The rte_driver will be however change in the
> future to serve as a common base for all other types of drivers.
> 
> The existing PMDs (PMD_VDEV) are to be modified later (there is
> no change for them at the moment).
> 
> There is however a inconsistency. The functions rte_eal_vdev_init
> and rte_eal_vdev_uninit are still placed in the rte_dev.h (instead
> of the rte_vdev.h).
> 
> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> ---
>  lib/librte_eal/bsdapp/eal/Makefile       |   1 +
>  lib/librte_eal/common/Makefile           |   2 +-
>  lib/librte_eal/common/eal_common_dev.c   |  54 +---------------
>  lib/librte_eal/common/eal_common_vdev.c  | 104 +++++++++++++++++++++++++++++++
>  lib/librte_eal/common/include/rte_dev.h  |   1 +
>  lib/librte_eal/common/include/rte_vdev.h |  83 ++++++++++++++++++++++++
>  lib/librte_eal/linuxapp/eal/Makefile     |   1 +
>  7 files changed, 192 insertions(+), 54 deletions(-)
>  create mode 100644 lib/librte_eal/common/eal_common_vdev.c
>  create mode 100644 lib/librte_eal/common/include/rte_vdev.h
> 
> diff --git a/lib/librte_eal/bsdapp/eal/Makefile b/lib/librte_eal/bsdapp/eal/Makefile
> index 698fa0a..b7e94a4 100644
> --- a/lib/librte_eal/bsdapp/eal/Makefile
> +++ b/lib/librte_eal/bsdapp/eal/Makefile

[...]

> diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
> new file mode 100644
> index 0000000..ea83c41
> --- /dev/null
> +++ b/lib/librte_eal/common/eal_common_vdev.c
> @@ -0,0 +1,104 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) 2016 RehiveTech. 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 RehiveTech 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 <string.h>
> +#include <sys/queue.h>
> +#include <rte_vdev.h>
> +
> +struct vdev_driver_list vdev_driver_list =
> +	TAILQ_HEAD_INITIALIZER(vdev_driver_list);
> +
> +/* register a driver */
> +void
> +rte_eal_vdrv_register(struct rte_vdev_driver *driver)
> +{
> +	TAILQ_INSERT_TAIL(&vdev_driver_list, driver, next);
> +}
> +
> +/* unregister a driver */
> +void
> +rte_eal_vdrv_unregister(struct rte_vdev_driver *driver)
> +{
> +	TAILQ_REMOVE(&vdev_driver_list, driver, next);
> +}
> +
> +int
> +rte_eal_vdev_init(const char *name, const char *args)
> +{
> +	struct rte_vdev_driver *driver;
> +
> +	if (name == NULL)
> +		return -EINVAL;
> +
> +	TAILQ_FOREACH(driver, &vdev_driver_list, next) {
> +		if (driver->driver.type != PMD_VDEV)
> +			continue;

Now that two separate lists for vdev and pdev exist, we don't need this check anymore.
In fact, PMD_VDEV might not even exist.

> +
> +		/*
> +		 * search a driver prefix in virtual device name.
> +		 * For example, if the driver is pcap PMD, driver->name
> +		 * will be "eth_pcap", but "name" will be "eth_pcapN".
> +		 * So use strncmp to compare.
> +		 */
> +		if (!strncmp(driver->driver.name, name, strlen(driver->driver.name)))
> +			return driver->driver.init(name, args);
> +	}
> +
> +	RTE_LOG(ERR, EAL, "no driver found for %s\n", name);
> +	return -EINVAL;
> +}
> +
> +int
> +rte_eal_vdev_uninit(const char *name)
> +{
> +	struct rte_vdev_driver *driver;
> +
> +	if (name == NULL)
> +		return -EINVAL;
> +
> +	TAILQ_FOREACH(driver, &vdev_driver_list, next) {
> +		if (driver->driver.type != PMD_VDEV)
> +			continue;

Same as above, redundant check.

> +
> +		/*
> +		 * search a driver prefix in virtual device name.
> +		 * For example, if the driver is pcap PMD, driver->name
> +		 * will be "eth_pcap", but "name" will be "eth_pcapN".
> +		 * So use strncmp to compare.
> +		 */
> +		if (!strncmp(driver->driver.name, name, strlen(driver->driver.name)))
> +			return driver->driver.uninit(name);
> +	}
> +
> +	RTE_LOG(ERR, EAL, "no driver found for %s\n", name);
> +	return -EINVAL;
> +}
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index b1c0520..2aeb752 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -210,6 +210,7 @@ static void devinitfn_ ##d(void)\
>  	rte_eal_driver_register(&d);\
>  }
>  
> +

Probably a stray newline.

>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/librte_eal/common/include/rte_vdev.h b/lib/librte_eal/common/include/rte_vdev.h
> new file mode 100644
> index 0000000..523bd92
> --- /dev/null
> +++ b/lib/librte_eal/common/include/rte_vdev.h
> @@ -0,0 +1,83 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) 2016 RehiveTech. 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 RehiveTech 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.
> + */
> +
> +#ifndef RTE_VDEV_H
> +#define RTE_VDEV_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <sys/queue.h>
> +#include <rte_dev.h>
> +
> +/** Double linked list of virtual device drivers. */
> +TAILQ_HEAD(vdev_driver_list, rte_vdev_driver);
> +
> +/**
> + * A virtual device driver abstraction.
> + */
> +struct rte_vdev_driver {
> +	TAILQ_ENTRY(rte_vdev_driver) next; /**< Next in list. */
> +	struct rte_driver driver;          /**< Inherited general driver. */
> +};
> +
> +/**
> + * Register a virtual device driver.
> + *
> + * @param driver
> + *   A pointer to a rte_vdev_driver structure describing the driver
> + *   to be registered.
> + */
> +void rte_eal_vdrv_register(struct rte_vdev_driver *driver);
> +
> +/**
> + * Unregister a virtual device driver.
> + *
> + * @param driver
> + *   A pointer to a rte_vdev_driver structure describing the driver
> + *   to be unregistered.
> + */
> +void rte_eal_vdrv_unregister(struct rte_vdev_driver *driver);
> +
> +#define RTE_EAL_VDRV_REGISTER(d)\

In the recent commits, I noticed that macros have taken the (name, driver) format.
PMD_REGISTER_DRIVER() (now redundant), DRIVER_REGISTER_PCI_TABLE() ... etc
It might be better to stick to the same format.

> +RTE_INIT(vdrvinitfn_ ##d);\
> +static void vdrvinitfn_ ##d(void)\
> +{\
> +	rte_eal_vdrv_register(&d);\
> +}
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
> diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
> index 30b30f3..9553e97 100644
> --- a/lib/librte_eal/linuxapp/eal/Makefile
> +++ b/lib/librte_eal/linuxapp/eal/Makefile
> @@ -85,6 +85,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_timer.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_memzone.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_log.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_launch.c
> +SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_vdev.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_pci.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_pci_uio.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_memory.c
>