From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f180.google.com (mail-pf0-f180.google.com [209.85.192.180]) by dpdk.org (Postfix) with ESMTP id 4A08B2BBE for ; Wed, 12 Jul 2017 17:36:02 +0200 (CEST) Received: by mail-pf0-f180.google.com with SMTP id q86so14735463pfl.3 for ; Wed, 12 Jul 2017 08:36:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=ZqkMzrU5eMg0O8I8LLlvM3e6VGJ0Gyn+dzTV1n8A4ls=; b=SIuvUQ92rWsrP3lNdD7vj5Nn3Rw23NCyl8vV1BhVAI1kCoJmCsPjgkrJdFOsbpc0EX vOcvAPHMsWagk1A3tsFiIjQSBgrzB8TGLl8zxT56/8817qaFJpIGhzHj2Y6DWUFJS4ZT R9+U+kIQ+PkaitDKzcuDUGbPDR06//M9kxi+f7Lwao2jywt7U9eOtGc8z4oFh6KLN6eH HAGwOomnAeoGA1IkOrc5vG5wuCB/g4bL7B33jjBVQ3CDDio4LO7Lpo2rGgik0j9AtNxn Npuk0AiorY2m2y7QMQEH7CkpT5TZx+o3Bwo1zeer+qzX+IfgV8iqR80kcRob763Fo/RT l93A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=ZqkMzrU5eMg0O8I8LLlvM3e6VGJ0Gyn+dzTV1n8A4ls=; b=ZS5XHbk24LUNGb97rdy2dxGgXNLr4dtnDnG5S3Yg3t9o+lo/ZqoJuQFO+lx6DOs+Op vFIPJqreu9/Z5KNDLcH8Qg78tobOJrd2T81ADPB54gLIvj5xIbjYGAZ+RdfFmDwq7e9N ajYZUQntS1WnZbAbUZ7Dn9N+RWtHtpQLunRgvNHr9PJ+QOkPpEVFbDNBtH2tyS83NqFo SvHJggcZUxZXESOStwalETWyuZ4foxmuQ4xz5kl2fSl17UjqrqaSBhYrl5UgM2cFjXBO ptG0Qielo8U+8ctvdEBafmayirVZj2JNC4K0xlnf0CHG14+NO+BTEss1rJyVVhJFo14C GM/g== X-Gm-Message-State: AIVw111iTh8e//D/niMVaOkJdEsjkadG/mid24J35XSL7v2Ug6HVw4D3 gJl9p0kjpfnLYhsn X-Received: by 10.84.193.101 with SMTP id e92mr4679145pld.209.1499873761524; Wed, 12 Jul 2017 08:36:01 -0700 (PDT) Received: from xeon-e3 (76-14-207-240.or.wavecable.com. [76.14.207.240]) by smtp.gmail.com with ESMTPSA id n74sm6903736pfh.118.2017.07.12.08.36.00 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 12 Jul 2017 08:36:01 -0700 (PDT) Date: Wed, 12 Jul 2017 08:35:50 -0700 From: Stephen Hemminger To: Morten =?UTF-8?B?QnLDuHJ1cA==?= Cc: "Yang, Zhiyong" , "Wiles, Keith" , "Thomas Monjalon" , "DPDK" , "Olivier Matz" , "Wang, Zhihong" , "Yuanhan Liu" , "Ananyev, Konstantin" , "Richardson, Bruce" , "Chilikin, Andrey" , "Jan Blunck" , =?UTF-8?B?TsOpbGlv?= Laranjeiro , , Message-ID: <20170712083550.62bd9442@xeon-e3> In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC359EB28F@smartserver.smartshare.dk> References: <1488966121-22853-1-git-send-email-olivier.matz@6wind.com> <130C769A-180F-4C74-A974-EA51E7FAD1EC@intel.com> <98CBD80474FA8B44BF855DF32C47DC359EB27E@smartserver.smartshare.dk> <53564501.0VCaTE2lYi@xps> <98CBD80474FA8B44BF855DF32C47DC359EB285@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC359EB28A@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC359EB28F@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 15:36:02 -0000 On Wed, 12 Jul 2017 11:50:38 +0200 Morten Br=C3=B8rup wrote: > > -----Original Message----- > > From: Yang, Zhiyong [mailto:zhiyong.yang@intel.com] > > Sent: Wednesday, July 12, 2017 11:02 AM > > To: Morten Br=C3=B8rup; Wiles, Keith > > Cc: Thomas Monjalon; DPDK; Olivier Matz; Wang, Zhihong; Yuanhan Liu; > > Ananyev, Konstantin; Richardson, Bruce; Chilikin, Andrey; Jan Blunck; > > N=C3=A9lio Laranjeiro; arybchenko@solarflare.com; > > jerin.jacob@caviumnetworks.com > > Subject: RE: [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port > > andnbsegments > >=20 > >=20 > > =20 > > > -----Original Message----- > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Morten Br=C3=B8r= up > > > Sent: Wednesday, July 12, 2017 3:25 PM > > > To: Wiles, Keith > > > Cc: Thomas Monjalon ; DPDK ; > > > Olivier Matz ; Wang, Zhihong > > > ; Yuanhan Liu ; > > > Ananyev, Konstantin ; Richardson, Bruce > > > ; Chilikin, Andrey > > > ; Jan Blunck ; =20 > > N=C3=A9lio =20 > > > Laranjeiro ; arybchenko@solarflare.com; > > > jerin.jacob@caviumnetworks.com > > > Subject: Re: [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port and > > > nbsegments > > > =20 > > > > -----Original Message----- > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wiles, Keith > > > > Sent: Tuesday, July 11, 2017 6:48 PM > > > > To: Morten Br=C3=B8rup > > > > Cc: Thomas Monjalon; DPDK; Olivier Matz; Wang, Zhihong; Yuanhan =20 > > Liu; =20 > > > > Ananyev, Konstantin; Richardson, Bruce; Chilikin, Andrey; Jan > > > > Blunck; N=C3=A9lio Laranjeiro; arybchenko@solarflare.com; > > > > jerin.jacob@caviumnetworks.com > > > > Subject: Re: [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port > > > > and nbsegments > > > > > > > > =20 > > > > > On Jul 11, 2017, at 10:23 AM, Morten Br=C3=B8rup =20 > > > > wrote: =20 > > > > > =20 > > > > >> -----Original Message----- > > > > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas > > > > >> Monjalon > > > > >> Sent: Tuesday, July 11, 2017 5:06 PM > > > > >> To: Morten Br=C3=B8rup > > > > >> Cc: dev@dpdk.org; Wiles, Keith; Olivier Matz; Wang, Zhihong; > > > > >> Yuanhan Liu; Ananyev, Konstantin; Richardson, Bruce; Chilikin, > > > > >> Andrey; Jan Blunck; nelio.laranjeiro@6wind.com; > > > > >> arybchenko@solarflare.com; jerin.jacob@caviumnetworks.com > > > > >> Subject: Re: [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for =20 > > port =20 > > > > and =20 > > > > >> nbsegments > > > > >> > > > > >> 11/07/2017 15:30, Morten Br=C3=B8rup: =20 > > > > >>> Morten Br=C3=B8rup wrote: =20 > > > > >>>> Olivier Matz wrote: =20 > > > > >>>>> As I said in a previous message, I think a good first step > > > > >>>>> would be to introduce a typedef for the port number: =20 > > > > >> rte_eth_port_num_t. =20 > > > > >>>>> It can still be uint8_t for now, and can be switched to 16 > > > > >>>>> bits =20 > > > > >> in =20 > > > > >>>>> one step when everyone uses this new type. =20 > > > > >>>> > > > > >>>> I think that DPDK follows the Linux tradition of exposing the > > > > >>>> variable types, as opposed to hiding them behind typedefs. =20 > > This =20 > > > > has =20 > > > > >>>> the unfortunate consequence that when a variable type changes, > > > > >>>> it has to be changed everywhere. > > > > >>>> > > > > >>>> Introducing a rte_eth_port_num_t will require changing the =20 > > same =20 > > > > >>>> files at the same locations everywhere, so not even as a > > > > >>>> temporary solution will it be beneficial. =20 > > > > >> [...] =20 > > > > >>> What I was trying to communicate with my long argument about > > > > >>> type =20 > > > > >> definitions was: When the type changed from 8 bit to 16 bit, the= =20 > > > > type =20 > > > > >> needs to change from uint8_t to uint16_t everywhere too, > > > > >> including =20 > > > > in =20 > > > > >> the ethdev APIs. =20 > > > > >>> > > > > >>> Don't start breaking coding conventions here by hiding the type > > > > >>> of =20 > > > > >> 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. =20 > > > > > > > > > > I'm against the typedef because it would break convention, and =20 > > I'm =20 > > > > > a =20 > > > > 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. =20 > > > > > > > > > > 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.) =20 > > > > > > > > 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 =20 > > more =20 > > > > then > > > > 2 bytes would be to encode something into the port id value, which = =20 > > I =20 > > > > could see, but a very slim chance IMHO. > > > > =20 > > > > > > > > > > However, if we change the convention and start hiding simple > > > > > types, =20 > > > > they still need the rte_ prefix regardless if they are popular or > > > > obscure types. Even struct rte_mbuf has the rte_ prefix, and I > > > > consider that a very popular type. If so, rte_port_t would be a =20 > > good =20 > > > > name for this type. =20 > > > > > > > > > > My preference: Follow convention and change it to uint16_t =20 > > > > everywhere. =20 > > > > > > > > > > Med venlig hilsen / kind regards > > > > > - Morten Br=C3=B8rup > > > > > =20 > > > > > > > > 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 = =20 > > to =20 > > > > 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 =20 > > doing =20 > > > > here for that reason. =20 > > > > > > That is not a very good reason: When used as a function parameter, =20 > > the =20 > > > 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). > > > =20 > > > > > > > > As for Olivier=E2=80=99s statement about the typedef name I do not = see the > > > > need for =E2=80=98_eth_' to be part of the typedef as it conveys no= extra > > > > information in the name. Everything port related in DPDK is a > > > > ethernet type device or port. If we do add something like fiber > > > > channel maybe rte_pid_t is reason to that too, but if it contains > > > > =E2=80=98_eth_=E2=80=99 it would not. > > > > > > > > I would like to see names that are just short enough to convey the > > > > information and not be redundant. IMHO rte_pid_t is fine, but if we > > > > use some something similar to the length of uint8_t (7) or uint16_t > > > > (8) characters then we would not have to also reformat the line =20 > > more =20 > > > > then needed. Using rte_pid_t (pid =3D=3D port_id) we only extend the > > > > length by one (or two) characters and most likely the added byte(s) > > > > will not cause more format problems in the code. =20 > > > > > > I still don't support typedefs for scalar types. I consider it =20 > > against =20 > > > the coding style, although after reviewing the official DPDK Coding > > > Style documentation > > > (http://dpdk.org/doc/guides/contributing/coding_style.html), I can =20 > > see =20 > > > 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 =20 > > should =20 > > > 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). > > > =20 > >=20 > > How about rte_dev_id_t? > >=20 > > Thanks > > Zhiyong > > =20 > > > =20 > > > > > > > > Regards, > > > > Keith =20 >=20 > 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' o= pinions about it: http://yarchive.net/comp/linux/typedefs.html >=20 > Perhaps the DPDK Coding Style should be updated to clarify this. (Or if w= e decide otherwise, to explicitly mention this deviation from the Linux cod= ing style.) It is logical to use typedef's for this kind of scalar type that may need t= o 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 Longer term, probably rte_eth_devices[] needs to go. Change port id into s= omething 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.