[dpdk-dev] [PATCH 04/12] vdev: move to drivers/bus

De Lara Guarch, Pablo pablo.de.lara.guarch at intel.com
Mon Sep 18 13:47:48 CEST 2017



> -----Original Message-----
> From: Tan, Jianfeng
> Sent: Tuesday, August 29, 2017 11:48 PM
> To: Gaëtan Rivet <gaetan.rivet at 6wind.com>
> Cc: dev at dpdk.org; Richardson, Bruce <bruce.richardson at intel.com>;
> Ananyev, Konstantin <konstantin.ananyev at intel.com>; De Lara Guarch,
> Pablo <pablo.de.lara.guarch at intel.com>; thomas at monjalon.net;
> yliu at fridaylinux.org; maxime.coquelin at redhat.com;
> mtetsuyah at gmail.com; Yigit, Ferruh <ferruh.yigit at intel.com>
> Subject: Re: [dpdk-dev] [PATCH 04/12] vdev: move to drivers/bus
> 
> Hi Gaetan,
> 
> 
> On 8/29/2017 6:04 AM, Gaëtan Rivet wrote:
> > On Fri, Aug 25, 2017 at 09:40:44AM +0000, Jianfeng Tan wrote:
> >> Move the vdev bus from lib/librte_eal to drivers/bus.
> >>
> >> As the crypto vdev helper function refers to data structure
> >> in rte_vdev.h, so we move those helper function into drivers/bus
> >> too.
> >>
> >> Signed-off-by: Jianfeng Tan <jianfeng.tan at intel.com>
> >> ---
> >>   config/common_base                        |   5 +
> >>   drivers/bus/Makefile                      |   2 +
> >>   drivers/bus/vdev/Makefile                 |  57 +++++
> >>   drivers/bus/vdev/rte_bus_vdev_version.map |  10 +
> >>   drivers/bus/vdev/rte_cryptodev_vdev.c     | 154 ++++++++++++++
> >>   drivers/bus/vdev/rte_cryptodev_vdev.h     | 100 +++++++++
> >>   drivers/bus/vdev/rte_vdev.h               | 153 +++++++++++++
> >>   drivers/bus/vdev/vdev.c                   | 342
> ++++++++++++++++++++++++++++++
> >>   lib/librte_cryptodev/Makefile             |   2 -
> >>   lib/librte_cryptodev/rte_cryptodev_vdev.c | 154 --------------
> >>   lib/librte_cryptodev/rte_cryptodev_vdev.h | 100 ---------
> >>   lib/librte_eal/bsdapp/eal/Makefile        |   1 -
> >>   lib/librte_eal/common/Makefile            |   2 +-
> >>   lib/librte_eal/common/eal_common_vdev.c   | 342 --------------------------
> ----
> >>   lib/librte_eal/common/include/rte_dev.h   |  24 +--
> >>   lib/librte_eal/common/include/rte_vdev.h  | 131 ------------
> >>   lib/librte_eal/linuxapp/eal/Makefile      |   1 -
> >>   mk/rte.app.mk                             |   1 +
> >>   18 files changed, 826 insertions(+), 755 deletions(-)
> >>   create mode 100644 drivers/bus/vdev/Makefile
> >>   create mode 100644 drivers/bus/vdev/rte_bus_vdev_version.map
> >>   create mode 100644 drivers/bus/vdev/rte_cryptodev_vdev.c
> >>   create mode 100644 drivers/bus/vdev/rte_cryptodev_vdev.h
> >>   create mode 100644 drivers/bus/vdev/rte_vdev.h
> >>   create mode 100644 drivers/bus/vdev/vdev.c
> >>   delete mode 100644 lib/librte_cryptodev/rte_cryptodev_vdev.c
> >>   delete mode 100644 lib/librte_cryptodev/rte_cryptodev_vdev.h
> >>   delete mode 100644 lib/librte_eal/common/eal_common_vdev.c
> >>   delete mode 100644 lib/librte_eal/common/include/rte_vdev.h
> >>
> >> diff --git a/config/common_base b/config/common_base
> >> index 5e97a08..aca0994 100644
> >> --- a/config/common_base
> >> +++ b/config/common_base
> >> @@ -750,3 +750,8 @@ CONFIG_RTE_APP_CRYPTO_PERF=y
> >>   # Compile the eventdev application
> >>   #
> >>   CONFIG_RTE_APP_EVENTDEV=y
> >> +
> >> +#
> >> +# Compile the vdev bus
> >> +#
> >> +CONFIG_RTE_LIBRTE_VDEV=y
> > Why not CONFIG_RTE_LIBRTE_VDEV_BUS?
> > It would seem more consistent.
> 
> Was trying to be consistent with the directory name, drivers/bus/vdev.
> Do you think that directory should also be renamed?
> 
> >
> >> diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
> >> index 0224214..9b6d45e 100644
> >> --- a/drivers/bus/Makefile
> >> +++ b/drivers/bus/Makefile
> >> @@ -35,4 +35,6 @@ core-libs := librte_eal librte_mbuf librte_mempool
> librte_ring librte_ether
> >>   DIRS-$(CONFIG_RTE_LIBRTE_FSLMC_BUS) += fslmc
> >>   DEPDIRS-fslmc = $(core-libs)
> >>
> >> +DIRS-$(CONFIG_RTE_LIBRTE_VDEV) += vdev
> >> +
> >>   include $(RTE_SDK)/mk/rte.subdir.mk
> >> diff --git a/drivers/bus/vdev/Makefile b/drivers/bus/vdev/Makefile
> >> new file mode 100644
> >> index 0000000..30c4813
> >> --- /dev/null
> >> +++ b/drivers/bus/vdev/Makefile
> >> @@ -0,0 +1,57 @@
> >> +#   BSD LICENSE
> >> +#
> >> +#   Copyright(c) 2017 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, 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 $(RTE_SDK)/mk/rte.vars.mk
> >> +
> >> +#
> >> +# library name
> >> +#
> >> +LIB = librte_bus_vdev.a
> >> +
> >> +CFLAGS += -O3
> >> +CFLAGS += $(WERROR_FLAGS)
> >> +
> >> +# versioning export map
> >> +EXPORT_MAP := rte_bus_vdev_version.map
> >> +
> >> +# library version
> >> +LIBABIVER := 1
> >> +
> >> +SRCS-y += vdev.c
> >> +SRCS-y += rte_cryptodev_vdev.c
> >> +
> >> +#
> >> +# Export include files
> >> +#
> >> +SYMLINK-y-include += rte_vdev.h
> >> +SYMLINK-y-include += rte_cryptodev_vdev.h
> >> +
> > Let's say the cryptodev lib must be updated.
> > I understand the need to move rte_cryptodev_vdev.h outside
> > librte_cryptodev, but I guess this exposes the vdev bus to ABI / API
> > instability due to a third-party subsystem?
> 
> Thank you for bringing up this question. I really don't want to move
> these crypto-specific files into bus/vdev/. It's just some helper
> functions to be called by crypto vdev drivers. And what's more, the only
> dependence on vdev is that the API rte_cryptodev_vdev_pmd_init() has a
> parameter of struct rte_vdev_device, which is totally not necessary, as
> it only needs a struct rte_device parameter.
> 
> In all, I'd prefer to change this specific API and move those
> crypto-specific files back to lib/librte_crypto (just like ether dev and
> eventdev); but it needs API change announcement.
> 
> Any thoughts?

I think we should keep this API in cryptodev. It looks strange to have some
Crypto specific functions in a file like this that should contain generic vdev
functions.

> 
> >
> > I did something somewhat similar for PCI:
> > http://dpdk.org/ml/archives/dev/2017-August/073525.html
> 
> I prefer your way to move those things to specific dev folder.
> 
> >
> > I don't know which solution is best, but something certainly needs to be
> > done.
> >
> > ---
> >
> > Beside the `why`, about the `how`: shouldn't this file compilation and
> > symlink be conditioned to CONFIG_RTE_LIBRTE_CRYPTODEV=y?
> >
> > i.e.: SYMLINK-$(CONFIG_RTE_LIBRTE_CRYPTODEV)-include +=
> rte_cryptodev_vdev.h
> 
> Yes, make sense.
> 
> Thanks,
> Jianfeng
> 
> 

Btw, for future times, strip out all the text where there are no comments,
so it is easier to review by others.

Thanks,
Pablo


More information about the dev mailing list