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

Yang, Zhiyong zhiyong.yang at intel.com
Wed Jul 12 11:02:28 CEST 2017



> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Morten Brørup
> Sent: Wednesday, July 12, 2017 3:25 PM
> To: Wiles, Keith <keith.wiles at intel.com>
> Cc: Thomas Monjalon <thomas at monjalon.net>; DPDK <dev at dpdk.org>; Olivier
> Matz <olivier.matz at 6wind.com>; Wang, Zhihong <zhihong.wang at intel.com>;
> Yuanhan Liu <yuanhan.liu at linux.intel.com>; Ananyev, Konstantin
> <konstantin.ananyev at intel.com>; Richardson, Bruce
> <bruce.richardson at intel.com>; Chilikin, Andrey <andrey.chilikin at intel.com>;
> Jan Blunck <jblunck at infradead.org>; Nélio Laranjeiro
> <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
> 
> > -----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).
> 

How about rte_dev_id_t?

Thanks
Zhiyong

> 
> >
> > Regards,
> > Keith



More information about the dev mailing list