> -----Original Message----- > From: Tan, Jianfeng > Sent: Tuesday, August 29, 2017 11:48 PM > To: Gaëtan Rivet <gaetan.ri...@6wind.com> > Cc: dev@dpdk.org; Richardson, Bruce <bruce.richard...@intel.com>; > Ananyev, Konstantin <konstantin.anan...@intel.com>; De Lara Guarch, > Pablo <pablo.de.lara.gua...@intel.com>; tho...@monjalon.net; > y...@fridaylinux.org; maxime.coque...@redhat.com; > mtetsu...@gmail.com; Yigit, Ferruh <ferruh.yi...@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....@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
