From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id 495652BBE for ; Wed, 12 Jul 2017 18:23:39 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 95D7C20A96; Wed, 12 Jul 2017 12:23:39 -0400 (EDT) Received: from frontend1 ([10.202.2.160]) by compute1.internal (MEProxy); Wed, 12 Jul 2017 12:23:39 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-sender :x-me-sender:x-sasl-enc:x-sasl-enc; s=mesmtp; bh=dIij/ZsprH2jF9O Kkdp0W/dymI+uuxrctNfGriAzOt4=; b=DxBPK7G/Uv1joKhvm5vhVPXlFKUOXEk accEVCCyU2aFnt98eT+4o5SMR1BMPinIxktPkTCxb6dm7B4iytZ/C4cWJl8JSf9U ZEFwjU9pspfEIF8KSrzrN6BkPAtFK9hAttMIBw/ogX2ckZXpIUaANh7cCqGvpcjT q9FkqBO8V8kE= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-sender:x-me-sender:x-sasl-enc:x-sasl-enc; s= fm1; bh=dIij/ZsprH2jF9OKkdp0W/dymI+uuxrctNfGriAzOt4=; b=ppSTx/sI C5sKsliGONlu8AVa1JxM+6MbK1yDJ8v0y7CSciPbcXlTKDIEOSZ085PZ0KyoMoEO ayvJWrqYhE+3dlGn7LnaTYtsxHVfPkPzqLnuEYxLZY+Co74Sj3jazo2y4EPd2VB+ aNxkMRt7jIbPhWV+rzKrYW6zzGeARYZd04v+7s/av0B+n14ZzS1acb5L7SfP8ijr 55coasDiKAsn5LPCmHiuC1zD25WiXuYQMDDPlgVws4Hqk2zlnWEYR6umazz9zNJ7 p7ftOuwQi+w/NLBXP3oxb0b3mTNmuWgxN0q4Pu5wweC87cfLDDa6cG2X0Jm39cj/ X+8hYYMDgeku+Q== X-ME-Sender: X-Sasl-enc: /ftr29YmnWt6v8RvFuqY6NsJyWFWYrw6dfBQz1gf9FV8 1499876619 Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 39BBF7E21C; Wed, 12 Jul 2017 12:23:39 -0400 (EDT) From: Thomas Monjalon To: Morten =?ISO-8859-1?Q?Br=F8rup?= , Stephen Hemminger , "Wiles, Keith" , Olivier Matz Cc: "Yang, Zhiyong" , dev@dpdk.org, "Wang, Zhihong" , Yuanhan Liu , "Ananyev, Konstantin" , "Richardson, Bruce" , "Chilikin, Andrey" , Jan Blunck , =?ISO-8859-1?Q?N=E9lio?= Laranjeiro , arybchenko@solarflare.com, jerin.jacob@caviumnetworks.com Date: Wed, 12 Jul 2017 18:23:38 +0200 Message-ID: <1911402.Nq572ABAkn@xps> In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC359EB297@smartserver.smartshare.dk> References: <1488966121-22853-1-git-send-email-olivier.matz@6wind.com> <20170712083550.62bd9442@xeon-e3> <98CBD80474FA8B44BF855DF32C47DC359EB297@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" Subject: Re: [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port andnbsegments 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: Wed, 12 Jul 2017 16:23:40 -0000 12/07/2017 17:57, Morten Br=F8rup: > From: Stephen Hemminger > > Morten Br=F8rup wrote: > > > From: Yang, Zhiyong [mailto:zhiyong.yang@intel.com] > > > > From: Morten Br=F8rup > > > > > From: Wiles, Keith > > > > > > > On Jul 11, 2017, at 10:23 AM, Morten Br=F8rup wrote: > > > > > > > From: Thomas Monjalon > > > > > > >> 11/07/2017 15:30, Morten Br=F8rup: > > > > > > >>> Morten Br=F8rup wrote: > > > > > > >>>> Olivier Matz wrote: > > > > > > >>>>> As I said in a previous message, I think a good first > > > > > > >>>>> step would be to introduce a typedef for the port > > > > > > >>>>> number: rte_eth_port_num_t. > > > > > > >>>>> It can still be uint8_t for now, and can be switched > > > > > > >>>>> to 16 bits in one step when everyone uses this new type. > > > > > > >>>> > > > > > > >>>> I think that DPDK follows the Linux tradition of exposing > > > > > > >>>> the variable types, as opposed to hiding them behind > > > > > > >>>> typedefs. This has the unfortunate consequence that > > > > > > >>>> when a variable type changes, it has to be changed everywh= ere. > > > > > > >>>> > > > > > > >>>> Introducing a rte_eth_port_num_t will require changing the > > > > > > >>>> same files at the same locations everywhere, so not even a= s a > > > > > > >>>> temporary solution will it be beneficial. > > > > > > >> [...] > > > > > > >>> What I was trying to communicate with my long argument > > > > > > >>> about type definitions was: > > > > > > >>> When the type changed from 8 bit to 16 bit, the type > > > > > > >>> needs to change from uint8_t to uint16_t everywhere too, > > > > > > >>> including in the ethdev APIs. > > > > > > >>> > > > > > > >>> Don't start breaking coding conventions here by hiding the > > > > > > >>> type of this variable. > > > > > > >> > > > > > > >> So, Morten, you are against the typedef, right? > > > > > > >> Because we need to change it everywhere anyway, right? > > > > > > >> > > > > > > >> Note: I have no strong opinion. > > > > > > > > > > > > > > I'm against the typedef because it would break convention, > > > > > > > and I'm a strong proponent of conventions. > > > > > > > In other projects, I'm all for typedefs, virtual classes, > > > > > > > inheritance etc., but DPDK follows the Linux convention > > > > > > > of not hiding simple types. > > > > > > > > > > > > > > We need to change it from uint8_t everywhere, regardless what > > > > > > > we change it to. (But if we need to change it again sometime > > > > > > > in the future, then a typedef will save us next time.) > > > > > > > > > > > > If the number of ports go beyond 64K then I will be the first > > > > > > one (if still alive) to eat this email. :-) The only reason to > > > > > > have more then 2 bytes would be to encode something into the > > > > > > port id value, which I could see, but a very slim chance IMHO. > > > > > > > > > > > > > My preference: Follow convention and change it to uint16_t > > > > > > > everywhere. > > > > > > > > > > > > As we must change the uint8_t to uint16_t, then I would like it > > > > > > to be more descriptive via a typedef. I really do not see us > > > > > > needing to change it again in the near future. > > > > > > The only reason to make it a typedef is to be able to identify > > > > > > from just the prototype of the function that it takes a port > > > > > > ID value, which I am in favor of doing here for that reason. > > > > > > > > > > That is not a very good reason: When used as a function > > > > > parameter, the type is only shown in the function declaration, > > > > > whereas the variable name is shown every time it is used inside > > > > > the function. > > > > > So remember to always use meaningful variable names, such as > > > > > "port" (like in the mbuf structure) or "port_id" (used in other > > > > > places). > > > > > > > > > > I still don't support typedefs for scalar types. I consider it > > > > > against the coding style, although after reviewing the official > > > > > DPDK Coding Style documentation > > > > > (http://dpdk.org/doc/guides/contributing/coding_style.html), > > > > > I can see that it is not explicitly stated. Please also note > > > > > that section 1.5.7 of the DPDK Coding Style documentation says > > > > > that the _t postfix should be avoided. Anyway, if we end up > > > > > with a typedef, please don't use something resembling pid_t > > > > > known from POSIX > > > > > (https://www.gnu.org/software/libc/manual/html_node/Process- > > > > > Identification.html). > > > > > > > > How about rte_dev_id_t? > > > > > > If the DPDK Coding Style is based on Linux Coding Style, we should > > > avoid typedefs in general, not just for structures. Please read Linus > > > Torvalds' opinions about it: > > > http://yarchive.net/comp/linux/typedefs.html > > > > > > Perhaps the DPDK Coding Style should be updated to clarify this. (Or > > > if we decide otherwise, to explicitly mention this deviation from the > > > Linux coding style.) > >=20 > > It is logical to use typedef's for this kind of scalar type that may > > need to change. > > Names matter, please avoid pid (confuse with posix) and dev (confuse > > with device id). > > I would prefer: rte_portid_t and rte_queueid_t > >=20 > > Longer term, probably rte_eth_devices[] needs to go. Change port id > > into something more like ifindex. And use sparse data structure to > > allow very large number of devices and non-contiguous lookup. Think of > > a VPN server where each VPN connection looks like a DPDK device. >=20 > We are using a non-contiguous ifindex in our firmware, for virtual > interfaces as you mention, so I get your point here! > But until DPDK gets there, I suppose the DPDK port id is considered > more or less contiguous. >=20 > You clearly have a longer track record working with Linus than me, > so if you interpret the coding style like that, I will not object > anymore - as my objection was based on coding style. And will someone > please update the DPDK Coding Style document accordingly... >=20 > rte_portid_t is fine with me, but why not just rte_port_t? One problem with opaque typedef is that we don't know how to print them, except if we have a PRIx macro. So I suggest to keep with uint16_t (my preference), or to add a printf format macro.