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

Morten Brørup mb at smartsharesystems.com
Wed Jul 12 09:25:21 CEST 2017


> -----Original Message-----
> From: dev [mailto:dev-bounces at 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 at solarflare.com;
> jerin.jacob at 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 at smartsharesystems.com> wrote:
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon
> >> Sent: Tuesday, July 11, 2017 5:06 PM
> >> To: Morten Brørup
> >> Cc: dev at dpdk.org; Wiles, Keith; Olivier Matz; Wang, Zhihong; Yuanhan
> >> Liu; Ananyev, Konstantin; Richardson, Bruce; Chilikin, Andrey; Jan
> >> Blunck; nelio.laranjeiro at 6wind.com; arybchenko at solarflare.com;
> >> jerin.jacob at 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).


> 
> Regards,
> Keith



More information about the dev mailing list