From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 08A5F7D4E for ; Mon, 18 Sep 2017 13:47:52 +0200 (CEST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Sep 2017 04:47:51 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,412,1500966000"; d="scan'208";a="152534794" Received: from irsmsx105.ger.corp.intel.com ([163.33.3.28]) by fmsmga005.fm.intel.com with ESMTP; 18 Sep 2017 04:47:50 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.167]) by irsmsx105.ger.corp.intel.com ([169.254.7.75]) with mapi id 14.03.0319.002; Mon, 18 Sep 2017 12:47:49 +0100 From: "De Lara Guarch, Pablo" To: "Tan, Jianfeng" , =?iso-8859-1?Q?Ga=EBtan_Rivet?= CC: "dev@dpdk.org" , "Richardson, Bruce" , "Ananyev, Konstantin" , "thomas@monjalon.net" , "yliu@fridaylinux.org" , "maxime.coquelin@redhat.com" , "mtetsuyah@gmail.com" , "Yigit, Ferruh" Thread-Topic: [dpdk-dev] [PATCH 04/12] vdev: move to drivers/bus Thread-Index: AQHTHYYPRnqcEbZR00OSbpZeLmmEr6KbQuMAgACi6oCAHsYvwA== Date: Mon, 18 Sep 2017 11:47:48 +0000 Message-ID: References: <1503654052-84730-1-git-send-email-jianfeng.tan@intel.com> <1503654052-84730-5-git-send-email-jianfeng.tan@intel.com> <20170829130430.GO8124@bidouze.vm.6wind.com> <8d8e053d-28c3-7cf5-f906-8fa1159187ca@intel.com> In-Reply-To: <8d8e053d-28c3-7cf5-f906-8fa1159187ca@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTIyYmQ4Y2ItOGM3MS00NzcwLThkYTItZGUxYWRlNDg0MDQzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6InAyQ3Rvbmd5K3JlTFJMbms0VmxoT2lNUHdUMWJJZ1lDVHVGS2taOGJOWDg9In0= x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 04/12] vdev: move to drivers/bus X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Sep 2017 11:47:53 -0000 > -----Original Message----- > From: Tan, Jianfeng > Sent: Tuesday, August 29, 2017 11:48 PM > To: Ga=EBtan Rivet > Cc: dev@dpdk.org; Richardson, Bruce ; > Ananyev, Konstantin ; De Lara Guarch, > Pablo ; thomas@monjalon.net; > yliu@fridaylinux.org; maxime.coquelin@redhat.com; > mtetsuyah@gmail.com; Yigit, Ferruh > Subject: Re: [dpdk-dev] [PATCH 04/12] vdev: move to drivers/bus >=20 > Hi Gaetan, >=20 >=20 > On 8/29/2017 6:04 AM, Ga=EBtan 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 > >> --- > >> 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=3Dy > >> # Compile the eventdev application > >> # > >> CONFIG_RTE_APP_EVENTDEV=3Dy > >> + > >> +# > >> +# Compile the vdev bus > >> +# > >> +CONFIG_RTE_LIBRTE_VDEV=3Dy > > Why not CONFIG_RTE_LIBRTE_VDEV_BUS? > > It would seem more consistent. >=20 > Was trying to be consistent with the directory name, drivers/bus/vdev. > Do you think that directory should also be renamed? >=20 > > > >> 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 :=3D librte_eal librte_mbuf librte_mempool > librte_ring librte_ether > >> DIRS-$(CONFIG_RTE_LIBRTE_FSLMC_BUS) +=3D fslmc > >> DEPDIRS-fslmc =3D $(core-libs) > >> > >> +DIRS-$(CONFIG_RTE_LIBRTE_VDEV) +=3D 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 withou= t > >> +# modification, are permitted provided that the following condition= s > >> +# are met: > >> +# > >> +# * Redistributions of source code must retain the above copyrigh= t > >> +# 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 =3D librte_bus_vdev.a > >> + > >> +CFLAGS +=3D -O3 > >> +CFLAGS +=3D $(WERROR_FLAGS) > >> + > >> +# versioning export map > >> +EXPORT_MAP :=3D rte_bus_vdev_version.map > >> + > >> +# library version > >> +LIBABIVER :=3D 1 > >> + > >> +SRCS-y +=3D vdev.c > >> +SRCS-y +=3D rte_cryptodev_vdev.c > >> + > >> +# > >> +# Export include files > >> +# > >> +SYMLINK-y-include +=3D rte_vdev.h > >> +SYMLINK-y-include +=3D 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? >=20 > 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. >=20 > 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. >=20 > 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 v= dev functions. >=20 > > > > I did something somewhat similar for PCI: > > http://dpdk.org/ml/archives/dev/2017-August/073525.html >=20 > I prefer your way to move those things to specific dev folder. >=20 > > > > I don't know which solution is best, but something certainly needs to b= e > > done. > > > > --- > > > > Beside the `why`, about the `how`: shouldn't this file compilation and > > symlink be conditioned to CONFIG_RTE_LIBRTE_CRYPTODEV=3Dy? > > > > i.e.: SYMLINK-$(CONFIG_RTE_LIBRTE_CRYPTODEV)-include +=3D > rte_cryptodev_vdev.h >=20 > Yes, make sense. >=20 > Thanks, > Jianfeng >=20 >=20 Btw, for future times, strip out all the text where there are no comments, so it is easier to review by others. Thanks, Pablo