12/07/2017 17:57, Morten Brørup:

From: Stephen Hemminger

quoted

Morten Brørup [off-list ref] wrote:

quoted

From: Yang, Zhiyong [mailto:zhiyong.yang@intel.com]

quoted

From: Morten Brørup

quoted

From: Wiles, Keith

quoted

quoted

On Jul 11, 2017, at 10:23 AM, Morten Brørup wrote:
From: Thomas Monjalon

quoted

11/07/2017 15:30, Morten Brørup:

quoted

Morten Brørup wrote:

quoted

Olivier Matz wrote:

quoted

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.

[...]

quoted

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.

quoted

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.