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

[Thomas Monjalon <thomas@monjalon.net>]

12/07/2017 17:57, Morten Brørup:
> From: Stephen Hemminger
> > Morten Brørup <mb@smartsharesystems.com> wrote:
> > > From: Yang, Zhiyong [mailto:zhiyong.yang@intel.com]
> > > > From: Morten Brørup
> > > > > From: Wiles, Keith
> > > > > > > On Jul 11, 2017, at 10:23 AM, Morten Brørup wrote:
> > > > > > > From: Thomas Monjalon
> > > > > > >> 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.
> > > > > >
> > > > > > > My preference: Follow convention and change it to uint16_t
> > > > > > > everywhere.
> > > > > >
> > > > > > 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).
> > > > >
> > > > > 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?
> > >
> > > 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.
> 
> We are using a non-contiguous ifindex in our firmware, for virtual
> interfaces as you mention, so I get your point here!
> But until DPDK gets there, I suppose the DPDK port id is considered
> more or less contiguous.
> 
> You clearly have a longer track record working with Linus than me,
> so if you interpret the coding style like that, I will not object
> anymore - as my objection was based on coding style. And will someone
> please update the DPDK Coding Style document accordingly...
> 
> rte_portid_t is fine with me, but why not just rte_port_t?

One problem with opaque typedef is that we don't know how to print them,
except if we have a PRIx macro.

So I suggest to keep with uint16_t (my preference),
or to add a printf format macro.