Re: [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port andnbsegments

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 12 Jul 2017 11:50:38 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > -----Original Message-----
> > From: Yang, Zhiyong [mailto:zhiyong.yang@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; arybchenko@solarflare.com;
> > jerin.jacob@caviumnetworks.com
> > Subject: RE: [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port
> > andnbsegments
> > 
> > 
> >   
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Morten Brørup
> > > Sent: Wednesday, July 12, 2017 3:25 PM
> > > To: Wiles, Keith <keith.wiles@intel.com>
> > > Cc: Thomas Monjalon <thomas@monjalon.net>; DPDK <dev@dpdk.org>;
> > > Olivier Matz <olivier.matz@6wind.com>; Wang, Zhihong
> > > <zhihong.wang@intel.com>; Yuanhan Liu <yuanhan.liu@linux.intel.com>;
> > > Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce
> > > <bruce.richardson@intel.com>; Chilikin, Andrey
> > > <andrey.chilikin@intel.com>; Jan Blunck <jblunck@infradead.org>;  
> > Nélio  
> > > Laranjeiro <nelio.laranjeiro@6wind.com>; arybchenko@solarflare.com;
> > > jerin.jacob@caviumnetworks.com
> > > Subject: Re: [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port and
> > > nbsegments
> > >  
> > > > -----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ørup
> > > > Cc: Thomas Monjalon; DPDK; Olivier Matz; Wang, Zhihong; Yuanhan  
> > Liu;  
> > > > Ananyev, Konstantin; Richardson, Bruce; Chilikin, Andrey; Jan
> > > > Blunck; Nélio Laranjeiro; arybchenko@solarflare.com;
> > > > jerin.jacob@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  
> > > > <mb@smartsharesystems.com> wrote:  
> > > > >  
> > > > >> -----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ørup
> > > > >> 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  
> > 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.