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

[Wiles, Keith]

> 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.

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.

Regards,
Keith