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

[Wiles, Keith]

> On Jul 12, 2017, at 11:23 AM, Thomas Monjalon <thomas at monjalon.net> wrote:
> 
> 12/07/2017 17:57, Morten Brørup:
>> From: Stephen Hemminger
>>> Morten Brørup <mb at smartsharesystems.com> wrote:
>>>> From: Yang, Zhiyong [mailto:zhiyong.yang at 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.

As in my previous email I think we have settled on uint16_t for the port and not a new typedef. Unless someone can define a compelling reason to use a new typedef.

Regards,
Keith