On Wed, 12 Jul 2017 11:50:38 +0200 Morten Brørup <m...@smartsharesystems.com> wrote:
> > -----Original Message----- > > From: Yang, Zhiyong [mailto:zhiyong.y...@intel.com] > > Sent: Wednesday, July 12, 2017 11:02 AM > > To: Morten Brørup; Wiles, Keith > > Cc: Thomas Monjalon; DPDK; Olivier Matz; Wang, Zhihong; Yuanhan Liu; > > Ananyev, Konstantin; Richardson, Bruce; Chilikin, Andrey; Jan Blunck; > > Nélio Laranjeiro; arybche...@solarflare.com; > > jerin.ja...@caviumnetworks.com > > Subject: RE: [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port > > andnbsegments > > > > > > > > > -----Original Message----- > > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Morten Brørup > > > Sent: Wednesday, July 12, 2017 3:25 PM > > > To: Wiles, Keith <keith.wi...@intel.com> > > > Cc: Thomas Monjalon <tho...@monjalon.net>; DPDK <dev@dpdk.org>; > > > Olivier Matz <olivier.m...@6wind.com>; Wang, Zhihong > > > <zhihong.w...@intel.com>; Yuanhan Liu <yuanhan....@linux.intel.com>; > > > Ananyev, Konstantin <konstantin.anan...@intel.com>; Richardson, Bruce > > > <bruce.richard...@intel.com>; Chilikin, Andrey > > > <andrey.chili...@intel.com>; Jan Blunck <jblu...@infradead.org>; > > Nélio > > > Laranjeiro <nelio.laranje...@6wind.com>; arybche...@solarflare.com; > > > jerin.ja...@caviumnetworks.com > > > Subject: Re: [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port and > > > nbsegments > > > > > > > -----Original Message----- > > > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Wiles, Keith > > > > Sent: Tuesday, July 11, 2017 6:48 PM > > > > To: Morten Brørup > > > > Cc: Thomas Monjalon; DPDK; Olivier Matz; Wang, Zhihong; Yuanhan > > Liu; > > > > Ananyev, Konstantin; Richardson, Bruce; Chilikin, Andrey; Jan > > > > Blunck; Nélio Laranjeiro; arybche...@solarflare.com; > > > > jerin.ja...@caviumnetworks.com > > > > Subject: Re: [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port > > > > and nbsegments > > > > > > > > > > > > > On Jul 11, 2017, at 10:23 AM, Morten Brørup > > > > <m...@smartsharesystems.com> wrote: > > > > > > > > > >> -----Original Message----- > > > > >> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Thomas > > > > >> Monjalon > > > > >> Sent: Tuesday, July 11, 2017 5:06 PM > > > > >> To: Morten Brørup > > > > >> Cc: dev@dpdk.org; Wiles, Keith; Olivier Matz; Wang, Zhihong; > > > > >> Yuanhan Liu; Ananyev, Konstantin; Richardson, Bruce; Chilikin, > > > > >> Andrey; Jan Blunck; nelio.laranje...@6wind.com; > > > > >> arybche...@solarflare.com; jerin.ja...@caviumnetworks.com > > > > >> Subject: Re: [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for > > port > > > > and > > > > >> nbsegments > > > > >> > > > > >> 11/07/2017 15:30, Morten Brørup: > > > > >>> Morten Brørup 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 everywhere. > > > > >>>> > > > > >>>> Introducing a rte_eth_port_num_t will require changing the > > same > > > > >>>> files at the same locations everywhere, so not even as 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. > > > > > > > > > > > > > > However, if we change the convention and start hiding simple > > > > > types, > > > > 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 > > good > > > > name for this type. > > > > > > > > > > My preference: Follow convention and change it to uint16_t > > > > everywhere. > > > > > > > > > > Med venlig hilsen / kind regards > > > > > - Morten Brørup > > > > > > > > > > > > > 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). > > > > > > > > > > > As for Olivier’s statement about the typedef name I do not see the > > > > need for ‘_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 > > > > ‘_eth_’ 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 > > more > > > > then needed. Using rte_pid_t (pid == 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. > > > > > > 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? > > > > Thanks > > Zhiyong > > > > > > > > > > > > > Regards, > > > > Keith > > 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.) 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 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.
