From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 9244EA0096 for ; Thu, 6 Jun 2019 17:14:50 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 63F7A1B993; Thu, 6 Jun 2019 17:14:50 +0200 (CEST) Received: from mx0b-0016f401.pphosted.com (mx0a-0016f401.pphosted.com [67.231.148.174]) by dpdk.org (Postfix) with ESMTP id 46E6A1B974 for ; Thu, 6 Jun 2019 17:14:49 +0200 (CEST) Received: from pps.filterd (m0045849.ppops.net [127.0.0.1]) by mx0a-0016f401.pphosted.com (8.16.0.27/8.16.0.27) with SMTP id x56FBSdV016711; Thu, 6 Jun 2019 08:14:48 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=marvell.com; h=from : to : cc : subject : date : message-id : references : in-reply-to : content-type : content-transfer-encoding : mime-version; s=pfpt0818; bh=eRYse1Eh0RKd1ooyygPfPjxWqb1F3+86HwCw+ts1LSY=; b=rL7yPBGU8apAXYhvY6nozR0xobDkLGw0QFKrjdEAc2DSut3lSrdxTvSz/nNyyQL+qoec rN2gZ6ZF/O6V+bUJfRUlHzzCXYGhuLINGUf/ZFKUw/iODSRidO6WoUNWQ83s2RRifJxc 6r+ubAUCIJdf2IbCvL6dLRL4ZH/T3wjJX+Lu5dzghPXRF0f0et78WK6+44J10L+H/Iaq GjRShcE/Ae7VONZGLQnijVry3SgYj6aJL9x2hwfKGf7bkIsQuk2UUjZdUpEvrlsG9Vay xLtcM1kwzmBkglJtGFnScLxp4FzHrSSXZdus90SZY7o+zIgz+Hp9mP3p8ms1qUmQLd2W pQ== Received: from sc-exch04.marvell.com ([199.233.58.184]) by mx0a-0016f401.pphosted.com with ESMTP id 2sxwgnsxyj-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-SHA384 bits=256 verify=NOT); Thu, 06 Jun 2019 08:14:48 -0700 Received: from SC-EXCH04.marvell.com (10.93.176.84) by SC-EXCH04.marvell.com (10.93.176.84) with Microsoft SMTP Server (TLS) id 15.0.1367.3; Thu, 6 Jun 2019 08:14:47 -0700 Received: from NAM05-CO1-obe.outbound.protection.outlook.com (104.47.48.51) by SC-EXCH04.marvell.com (10.93.176.84) with Microsoft SMTP Server (TLS) id 15.0.1367.3 via Frontend Transport; Thu, 6 Jun 2019 08:14:47 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=marvell.onmicrosoft.com; s=selector2-marvell-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=eRYse1Eh0RKd1ooyygPfPjxWqb1F3+86HwCw+ts1LSY=; b=ouWK/pk7hQnkEUXELNgPSXhSJvQW1h5AfXzfiiyYHMx2PjSfx+2KrKy/2srDrlFWQI849LbjXyf3bCQbSshJTlrvdl0WTx+/ygybhgFoDv0NU4UGBMTdJFXJ4C78GbHfrqMVb2QEs1FDkVxQw/P8y7MjvCfF2B2aIPc1c2zf6x8= Received: from BYAPR18MB2424.namprd18.prod.outlook.com (20.179.91.149) by BYAPR18MB2648.namprd18.prod.outlook.com (20.179.94.83) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1965.12; Thu, 6 Jun 2019 15:14:43 +0000 Received: from BYAPR18MB2424.namprd18.prod.outlook.com ([fe80::1ce4:557d:eeb8:843c]) by BYAPR18MB2424.namprd18.prod.outlook.com ([fe80::1ce4:557d:eeb8:843c%7]) with mapi id 15.20.1965.011; Thu, 6 Jun 2019 15:14:43 +0000 From: Jerin Jacob Kollanukkaran To: Neil Horman CC: Bruce Richardson , "dev@dpdk.org" , Thomas Monjalon Thread-Topic: [EXT] [RFC PATCH 0/2] introduce __rte_internal tag Thread-Index: AQHVEynnIvy0R3R0lkaWrQuIVgPnUKaNTWtQgAAIeYCAABfgAIABAvyAgAAgfgCAAAaRkIAAGyeAgAAFlPCAABM/AIAAAWqg Date: Thu, 6 Jun 2019 15:14:42 +0000 Message-ID: References: <20190525184346.27932-1-nhorman@tuxdriver.com> <20190605164541.GH1550@bricha3-MOBL.ger.corp.intel.com> <20190605181108.GC554@hmswarspite.think-freely.org> <20190606113422.GA29521@hmswarspite.think-freely.org> <20190606133503.GB29521@hmswarspite.think-freely.org> <20190606150354.GF29521@hmswarspite.think-freely.org> In-Reply-To: <20190606150354.GF29521@hmswarspite.think-freely.org> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [122.178.234.223] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 2dd6804a-40b1-4e46-8aec-08d6ea91b3fd x-microsoft-antispam: BCL:0; PCL:0; RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600148)(711020)(4605104)(1401327)(2017052603328)(7193020); SRVR:BYAPR18MB2648; x-ms-traffictypediagnostic: BYAPR18MB2648: x-ms-exchange-purlcount: 2 x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:8882; x-forefront-prvs: 00603B7EEF x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(39860400002)(376002)(346002)(136003)(366004)(396003)(43544003)(174874002)(13464003)(199004)(189003)(73956011)(81166006)(66946007)(14454004)(4326008)(5660300002)(6916009)(8676002)(66066001)(3846002)(7736002)(66556008)(229853002)(446003)(30864003)(74316002)(66476007)(256004)(64756008)(33656002)(305945005)(7696005)(76176011)(99286004)(68736007)(6436002)(14444005)(53546011)(71190400001)(54906003)(8936002)(9686003)(102836004)(6306002)(6506007)(81156014)(186003)(86362001)(6116002)(26005)(52536014)(55016002)(561944003)(66446008)(76116006)(11346002)(476003)(478600001)(2906002)(6246003)(486006)(25786009)(316002)(53936002)(966005)(71200400001); DIR:OUT; SFP:1101; SCL:1; SRVR:BYAPR18MB2648; H:BYAPR18MB2424.namprd18.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: marvell.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam-message-info: m9oXAJBBDHioxFUAiD9FORRJfm/WZIfKVfPnjJlJ1iae68w58RbaFQH9t1rez/XkP5j7yFObjoWYaSVk3d5ZwLJC9nQak4m2ARCRaEY2s5YGaVoj3WkNGVyHPQMNKVZBaZTPtdWJXnxyzEYscwdxxnXQ6lSM/gbp6bsumXofeJE6s+93nqZtff6mUQ3X3n71Xs75G5HdrkxQ0V0b8RQh05Zhmn6wW4X8epDQPhyVfkUI8/r78ONPpP/szj5h+4rUV3kEBqrCe9QtnLlHR5GPh6pcAllReXrqFANEdZ8pb2UZAQbcdxLbTye3Q2olBU7vwo7UDEqsfQoroRsmYcZpxa9el9OUyrsLNcAYfm6bSGOh/lSgLDgohkK+CLKyvbdyTnqJht4hekDZulttmagFNITU4OBBjfzxLY1w/JPn+eE= Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 2dd6804a-40b1-4e46-8aec-08d6ea91b3fd X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Jun 2019 15:14:42.8290 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: jerinj@marvell.com X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR18MB2648 X-OriginatorOrg: marvell.com X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:, , definitions=2019-06-06_11:, , signatures=0 Subject: Re: [dpdk-dev] [EXT] [RFC PATCH 0/2] introduce __rte_internal tag 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Neil Horman > Sent: Thursday, June 6, 2019 8:34 PM > To: Jerin Jacob Kollanukkaran > Cc: Bruce Richardson ; dev@dpdk.org; > Thomas Monjalon > Subject: Re: [EXT] [RFC PATCH 0/2] introduce __rte_internal tag >=20 > On Thu, Jun 06, 2019 at 02:02:03PM +0000, Jerin Jacob Kollanukkaran wrote= : > > > -----Original Message----- > > > From: Neil Horman > > > Sent: Thursday, June 6, 2019 7:05 PM > > > To: Jerin Jacob Kollanukkaran > > > Cc: Bruce Richardson ; dev@dpdk.org; > > > Thomas Monjalon > > > Subject: Re: [EXT] [RFC PATCH 0/2] introduce __rte_internal tag > > > > > > On Thu, Jun 06, 2019 at 12:04:57PM +0000, Jerin Jacob Kollanukkaran > wrote: > > > > > -----Original Message----- > > > > > From: Neil Horman > > > > > Sent: Thursday, June 6, 2019 5:04 PM > > > > > To: Jerin Jacob Kollanukkaran > > > > > Cc: Bruce Richardson ; dev@dpdk.org; > > > > > Thomas Monjalon > > > > > Subject: Re: [EXT] [RFC PATCH 0/2] introduce __rte_internal tag > > > > > > > > > > On Thu, Jun 06, 2019 at 09:44:52AM +0000, Jerin Jacob > > > > > Kollanukkaran > > > wrote: > > > > > > > -----Original Message----- > > > > > > > From: Neil Horman > > > > > > > Sent: Wednesday, June 5, 2019 11:41 PM > > > > > > > To: Bruce Richardson > > > > > > > Cc: Jerin Jacob Kollanukkaran ; > > > > > > > dev@dpdk.org; Thomas Monjalon > > > > > > > Subject: Re: [EXT] [RFC PATCH 0/2] introduce __rte_internal > > > > > > > tag > > > > > > > > > > > > > > On Wed, Jun 05, 2019 at 05:45:41PM +0100, Bruce Richardson > wrote: > > > > > > > > On Wed, Jun 05, 2019 at 04:24:09PM +0000, Jerin Jacob > > > > > > > > Kollanukkaran > > > > > > > wrote: > > > > > > > > > > -----Original Message----- > > > > > > > > > > From: Neil Horman > > > > > > > > > > Sent: Sunday, May 26, 2019 12:14 AM > > > > > > > > > > To: dev@dpdk.org > > > > > > > > > > Cc: Neil Horman ; Jerin Jacob > > > > > > > > > > Kollanukkaran ; Bruce Richardson > > > > > > > > > > ; Thomas Monjalon > > > > > > > > > > > > > > > > > > > > Subject: [EXT] [RFC PATCH 0/2] introduce > > > > > > > > > > __rte_internal tag > > > > > > > > > > > > > > > > > > > > Hey- > > > > > > > > > > Based on our recent conversations regarding the use > > > > > > > > > > of symbols only meant for internal dpdk consumption > > > > > > > > > > (between dpdk libraries), this is an idea that I've > > > > > > > > > > come up with that I'd like to get some feedback on > > > > > > > > > > > > > > > > > > > > Summary: > > > > > > > > > > 1) We have symbols in the DPDK that are meant to be > > > > > > > > > > used between DPDK libraries, but not by applications > > > > > > > > > > linking to them > > > > > > > > > > 2) We would like to document those symbols in the > > > > > > > > > > code, so as to note them clearly as for being meant > > > > > > > > > > for internal use only > > > > > > > > > > 3) Linker symbol visibility is a very coarse grained > > > > > > > > > > tool, and so there is no good way in a single library > > > > > > > > > > to mark items as being meant for use only by other > > > > > > > > > > DPDK libraries, at least not without some extensive > > > > > > > > > > runtime checking > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Proposal: > > > > > > > > > > I'm proposing that we introduce the __rte_internal tag. > > > > > > > > > > From a coding standpoint it works a great deal like > > > > > > > > > > the __rte_experimental tag in that it expempts the > > > > > > > > > > tagged symbol from ABI constraints (as the only users > > > > > > > > > > should be represented in the DPDK build environment). > > > > > > > > > > Additionally, the __rte_internal macro resolves > > > > > > > > > > differently based on the definition of the > > > > > > > > > > BUILDING_RTE_SDK flag (working under the assumption > > > > > > > > > > that said flag should only ever be set if we are > > > > > > > > > > actually building DPDK libraries which will make use > > > > > > > > > > of internal calls). If the BUILDING_RTE_SDK flag is > > > > > > > > > > set __rte_internal resolves to __attribute__((section > > > > > > > > > > "text.internal)), placing it in a special text section > > > > > > > > > > which is then used to validate that the the symbol > > > > > > > > > > appears in the INTERNAL section of the corresponding > > > > > > > > > > library version > > > map). > > > > > > > > > > If BUILDING_RTE_SDK is not set, then __rte_internal > > > > > > > > > > resolves to > > > > > > > __attribute__((error("..."))), which causes any caller of > > > > > > > the tagged function to throw an error at compile time, > > > > > > > indicating that the symbol is not available for external use. > > > > > > > > > > > > > > > > > > > > This isn't a perfect solution, as applications can > > > > > > > > > > still hack around it of course, > > > > > > > > > > > > > > > > > > I think, one way to, avoid, hack around could be to, > > > > > > > > > > > > > > > > > > 1) at config stage, create a random number for the > > > > > > > > > build > > > > > > > > > 2) introduce RTE_CALL_INTERNAL macro for calling > > > > > > > > > internal function, compare the generated random number > > > > > > > > > for allowing the calls to make within the library. i.e > > > > > > > > > leverage the fact that external library would never know > > > > > > > > > the random number generated for the DPDK build > > > > > > > and internal driver code does. > > > > > > > > > > > > > > > > > Do we really need to care about this. If have some > > > > > > > > determined enough to hack around our limitations, then > > > > > > > > they surely know that they have an unsupported > > > > > > > > configuration. We just need to protect against inadvertent = use > of internals, IMHO. > > > > > > > > > > > > > > > I agree, I too had thought about doing some sort of internal > > > > > > > runtime checking to match internal only symbols, such that > > > > > > > they were only accessable by internally approved users, but > > > > > > > it started to feel like a great > > > > > deal of overhead. > > > > > > > Its a good idea for a general mechanism I think, but I > > > > > > > believe the value here is more to internally document which > > > > > > > apis we want to mark as being for internal use only, and > > > > > > > create a lightweight roadblock at build time to catch users > inadvertently using them. > > > > > > > Determined users will get around anything, and theres not > > > > > > > much we can do to stop > > > > > them. > > > > > > > > > > > > I agree too. IMHO, Simply having following items would be > > > > > > enough > > > > > > > > > > > > 1) Avoid exposing the internal function prototype through > > > > > > public header files > > > > > > 2) Add @internal to API documentation > > > > > > 3) Just decide the name space for internal API for tooling(i.e > > > > > > not start with rte_ or so) Using objdump scheme to detect > > > > > > internal functions > > > > > requires the the library to build prior to run the checkpatch. > > > > > > > > > > > > > > > > No, I'm not comfortable with that approach, and I've stated why: > > > > > 1) Not exposing the functions via header files is a fine start > > > > > > > > > > 2) Adding internal documentation is also fine, but does nothing > > > > > to correlate the code implementing those functions to the > > > > > documentation. Its valuable to have a tag on a function > > > > > identifying it as > > > internal only. > > > > > > > > > > 3) Using naming conventions to separate internal only from > > > > > non-internal functions is a vague approach, requiring future > > > > > developers to be cogniscent of the convention and make the > > > > > appropriate naming choices. It also implicitly restricts the > > > > > abliity for future developers to make naming changes in conflict > > > > > with that convention > > > > > > > > Enforcing the naming convention can be achieved through tooling as > well. > > > > > > > Sure, but why enforce any function naming at all, when you don't have > to. > > > > May I ask, why to enforce __rte_internal, when you don't have to > > >=20 > Because its more clear. Implicitly deciding that any function not prefix= ed with > rte_ is internal only does nothing to prevent a developer from accidental= ly > naming a function incorrectly, exporting it, and allowing a user to call = it. We > can move headers all you want, but we provide an ABI guarantee to end > users, and developers should have a way to clearly record that without > having to check the documentation for each function that an application > developer wants to use. >=20 > The long and the short of it for me is that I want a way for developers t= o opt > their code into an internal only condition, not to just document it as su= ch and > hope its up to date. If they tag a function as __rte_internal then its c= learly > marked as internal only, they have checks to ensure that its in the INTER= NAL > section of the version map, and should that header somehow get externally > exported (see rte_mempool_check_cookies for an example of how thats > happened), users are prevented from using them at build time, rather than > having to ask questions on the list, or read documentation after an error= to > find out "oops, shouldn't have done that". >=20 > I think you'll find that going through all the header files, and bifurcat= ing them > to public and private headers is a much larger undertaking than just tagg= ing > those functions accordingly. a quick scan of all our header file for the > @internal tag shows about 260 instances of such functions, almost all of > which are published to applications. All of those functions would have t= o be > moved to private headers, and their requisite C files would need to be > updated to include the new header. with the use of __rte_internal, we ju= st > have tag the functions as such, which can be handled with a cocinelle or = awk > script. I don't have any strong opinion on name prefix vs marking as __rte_internal= . Or combination of both. I am fine any approach. I have only strong option on not to induce objdump dependency for checkpat= ch.=20 For the reason mentioned in http://mails.dpdk.org/archives/dev/2019-June/13= 4160.html. >=20 > Neil >=20 >=20 > > > > > > > > > > > > > 4) Adding a tag like __rte_internal creates an interlock > > > > > whereby, not only are internal functions excused from ABI > > > > > constraints, but forces developers to intentionally mark their > > > > > internal functions as being internal in the code, which is > > > > > beneficial to clarlity of understanding > > > during the development process. > > > > > > > > No issues in adding __rte_internal. But, I am against current > > > > implementaion, Ie. adding objdump dependency > > > That dependency already exists for the __rte_external flag > > > > Sorry, I could not see the dependency. > > > > [master][dpdk.org] $ grep -ri "objdump" devtools/ [master][dpdk.org] $ > > grep -ri "objdump" usertools/ [master][dpdk.org] $ grep -ri > > "__rte_external" * > > > > > > > > > to checkpatch i.e developer has to build the library first so > > > > that checkpatch can can know, Is it belongs to internal section or = not? > > > > > > > What developer is running checkpatch/posting patches without first > > > building their changes? > > > > # it is not developer, The CI/CD tools can quicky check the sanity of > > patches before the build itself. Why to add unnecessary dependency? > > # If some PMD is not building if the requirements are not meet(say AES > > NI PMD for crypto) then how do take care of the dependency. > > > > > > > > > > > > > > > > > > > > 5) Adding a tag like __rte_internal is explicit, and allows > > > > > developers to use a single header file instead of multiple > > > > > header files if they so choose > > > > > > > > > > We went through this with experimental symbols as well[1], and > > > > > it just makes more sense to me to clearly document in the code > > > > > what constitutes an internal symbol rather than relying on > > > > > naming conventions and hoping that developers read the > > > > > documentation before exporting a symbol publically. > > > > > > > > > > > > > > > [1] > > > > > https://mails.dpdk.org/archives/dev/2017-December/083828.html > > > > > > > > > > > > > > If we really wanted to go down that road, we could use a > > > > > > > mechainsm simmilar to the EXPORT_SYMBOL / > EXPORT_SYMBOL_GPL > > > > > > > infrastructure that the kernel uses, but that would required > > > > > > > building our own custom linker script, which seems like overk= ill > here. > > > > > > > > > > > > > > Best > > > > > > > Neil > > > > > > > > > > > > > > > /Bruce > > > > > > > > > > > > > > > > > > > >