From: "Wiles, Keith" <keith.wiles@intel.com>
To: "Morten Brørup" <mb@smartsharesystems.com>
Cc: "Thomas Monjalon" <thomas@monjalon.net>, DPDK <dev@dpdk.org>,
"Olivier Matz" <olivier.matz@6wind.com>,
"Wang, Zhihong" <zhihong.wang@intel.com>,
"Yuanhan Liu" <yuanhan.liu@linux.intel.com>,
"Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
"Richardson, Bruce" <bruce.richardson@intel.com>,
"Chilikin, Andrey" <andrey.chilikin@intel.com>,
"Jan Blunck" <jblunck@infradead.org>,
"Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>,
"arybchenko@solarflare.com" <arybchenko@solarflare.com>,
"jerin.jacob@caviumnetworks.com" <jerin.jacob@caviumnetworks.com>
Subject: Re: [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port and nbsegments
Date: Wed, 12 Jul 2017 15:34:11 +0000 [thread overview]
Message-ID: <33685046-2702-4AE4-826B-8FE65832127F@intel.com> (raw)
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC359EB28A@smartserver.smartshare.dk>
> On Jul 12, 2017, at 2:25 AM, Morten Brørup <mb@smartsharesystems.com> wrote:
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@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@solarflare.com;
>> jerin.jacob@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@smartsharesystems.com> wrote:
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
>>>> Sent: Tuesday, July 11, 2017 5:06 PM
>>>> To: Morten Brørup
>>>> Cc: dev@dpdk.org; Wiles, Keith; Olivier Matz; Wang, Zhihong; Yuanhan
>>>> Liu; Ananyev, Konstantin; Richardson, Bruce; Chilikin, Andrey; Jan
>>>> Blunck; nelio.laranjeiro@6wind.com; arybchenko@solarflare.com;
>>>> jerin.jacob@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).
I stated in the prototype not docs, which does pop up in some edits and so forth. Using the correct variable names it a given and not the subject of this discussion. Even Torvalds’ states that typedefs are reasonable in some cases, not sure this one falls in to that slot. Typedefs for structures were the main concern from Torvald is what I took from his email and we are not talking about a structure here.
>
>>
>> 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).
Even the Linux uint16_t is a typedef, so even Linux thought using a typedef here was reasonable as it changes from arch to arch. The real question here is does the port id fall into this category. Most likely not in this case unless we think the variable will change in the future or we decide to encode more information to the variable then just a simple port id 0-N.
If we can not determine it falls into this category then we use a scalar typedef like uint16_t, which seems to be the norm here. My point about typedefs mainly was around the name if we have a typedef not to make it 40 characters long or have redundant information in the typedef name.
So it appears the port ID value is not going to change any time soon and we do not need to make it opaque or encode information into the variable, which to me means we need to use the standard typedef from Linux ‘uint16_t’.
Is that agreeable with everyone or do you have a good reason to make it a typedef?
>
>
>>
>> Regards,
>> Keith
Regards,
Keith
next prev parent reply other threads:[~2017-07-12 15:34 UTC|newest]
Thread overview: 155+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-24 15:19 [dpdk-dev] [RFC 0/8] mbuf: structure reorganization Olivier Matz
2017-01-24 15:19 ` [dpdk-dev] [RFC 1/8] mbuf: make segment prefree function public Olivier Matz
2017-01-24 15:19 ` [dpdk-dev] [RFC 2/8] mbuf: make raw free " Olivier Matz
2017-01-24 15:19 ` [dpdk-dev] [RFC 3/8] mbuf: set mbuf fields while in pool Olivier Matz
2017-01-24 15:50 ` Bruce Richardson
2017-02-28 14:51 ` Olivier Matz
2017-01-24 15:19 ` [dpdk-dev] [RFC 4/8] net: don't touch mbuf next or nb segs on Rx Olivier Matz
2017-01-24 15:19 ` [dpdk-dev] [RFC 5/8] mbuf: make rearm data address naturally aligned Olivier Matz
2017-01-24 15:19 ` [dpdk-dev] [RFC 6/8] mbuf: use 2 bytes for port and nb segments Olivier Matz
2017-01-24 15:19 ` [dpdk-dev] [RFC 7/8] mbuf: move sequence number in second cache line Olivier Matz
2017-01-24 15:19 ` [dpdk-dev] [RFC 8/8] mbuf: add a timestamp field Olivier Matz
2017-01-24 15:59 ` [dpdk-dev] [RFC 0/8] mbuf: structure reorganization Bruce Richardson
2017-01-24 16:16 ` Olivier MATZ
2017-02-06 18:41 ` Ananyev, Konstantin
2017-02-09 16:20 ` Morten Brørup
2017-02-09 16:56 ` Ananyev, Konstantin
2017-02-16 13:48 ` Olivier Matz
2017-02-16 15:46 ` Bruce Richardson
2017-02-16 16:14 ` Olivier Matz
2017-02-21 14:20 ` Morten Brørup
2017-02-21 14:28 ` Bruce Richardson
2017-02-21 15:04 ` Olivier MATZ
2017-02-21 15:18 ` Bruce Richardson
2017-02-21 15:18 ` Morten Brørup
2017-02-19 19:04 ` Chilikin, Andrey
2017-02-21 9:53 ` Olivier MATZ
2017-02-16 17:26 ` Jan Blunck
2017-02-17 10:51 ` Olivier Matz
2017-02-17 12:49 ` Nélio Laranjeiro
2017-02-17 13:51 ` Jan Blunck
2017-02-18 5:48 ` Andrew Rybchenko
2017-02-17 13:38 ` Jan Blunck
2017-02-17 14:17 ` Olivier Matz
2017-02-17 18:42 ` Ananyev, Konstantin
2017-02-21 9:53 ` Olivier MATZ
2017-02-21 10:28 ` Ananyev, Konstantin
2017-02-20 9:27 ` Jan Blunck
2017-02-21 9:54 ` Olivier MATZ
2017-02-21 16:12 ` Jan Blunck
2017-02-21 16:38 ` Bruce Richardson
2017-02-21 17:04 ` Jan Blunck
2017-02-21 17:26 ` Ananyev, Konstantin
2017-02-21 19:17 ` Jan Blunck
2017-02-21 20:30 ` Ananyev, Konstantin
2017-02-21 21:51 ` Morten Brørup
2017-02-24 14:11 ` Olivier Matz
2017-02-24 14:00 ` Olivier Matz
2017-02-24 14:21 ` Bruce Richardson
2017-02-28 8:55 ` Jan Blunck
2017-02-28 9:05 ` Ananyev, Konstantin
2017-02-28 9:23 ` Olivier Matz
2017-02-28 9:33 ` Jan Blunck
2017-02-28 10:29 ` Ananyev, Konstantin
2017-02-28 10:50 ` Olivier Matz
2017-02-28 11:48 ` Ananyev, Konstantin
2017-02-28 12:28 ` Olivier Matz
2017-02-28 22:53 ` Ananyev, Konstantin
2017-03-02 16:46 ` Olivier Matz
2017-03-08 11:11 ` Ananyev, Konstantin
2017-03-20 9:00 ` Olivier Matz
2017-03-22 17:42 ` Ananyev, Konstantin
2017-03-24 8:35 ` Jerin Jacob
2017-03-24 13:35 ` Olivier Matz
2017-02-28 9:25 ` Jan Blunck
2017-02-19 23:45 ` Ananyev, Konstantin
2017-02-21 9:22 ` Morten Brørup
2017-02-21 9:54 ` Olivier MATZ
2017-03-08 9:41 ` [dpdk-dev] [PATCH 0/9] " Olivier Matz
2017-03-08 9:41 ` [dpdk-dev] [PATCH 1/9] mbuf: make segment prefree function public Olivier Matz
2017-03-08 9:41 ` [dpdk-dev] [PATCH 2/9] mbuf: make raw free " Olivier Matz
2017-03-08 9:41 ` [dpdk-dev] [PATCH 3/9] mbuf: set mbuf fields while in pool Olivier Matz
2017-03-31 11:21 ` Bruce Richardson
2017-03-31 11:51 ` Ananyev, Konstantin
2017-03-08 9:41 ` [dpdk-dev] [PATCH 4/9] drivers/net: don't touch mbuf next or nb segs on Rx Olivier Matz
2017-03-08 9:41 ` [dpdk-dev] [PATCH 5/9] mbuf: make rearm data address naturally aligned Olivier Matz
2017-03-08 9:41 ` [dpdk-dev] [PATCH 6/9] mbuf: use 2 bytes for port and nb segments Olivier Matz
2017-03-08 9:41 ` [dpdk-dev] [PATCH 7/9] mbuf: move sequence number in second cache line Olivier Matz
2017-03-08 9:42 ` [dpdk-dev] [PATCH 8/9] mbuf: add a timestamp field Olivier Matz
2017-04-04 10:29 ` [dpdk-dev] [PATCH 0/2] reduce writes to mbuf in ixgbe vRX Konstantin Ananyev
2017-04-07 15:13 ` Ferruh Yigit
2017-04-07 15:44 ` Ferruh Yigit
2017-04-09 22:56 ` Ananyev, Konstantin
2017-04-04 10:29 ` [dpdk-dev] [PATCH 1/2] net/ixgbe: eliminate mbuf write on rearm Konstantin Ananyev
2017-04-10 15:59 ` [dpdk-dev] [PATCH v2 0/2] reduce writes to mbuf in ixgbe vRX Konstantin Ananyev
2017-04-10 16:17 ` Ferruh Yigit
2017-04-10 15:59 ` [dpdk-dev] [PATCH v2 1/2] net/ixgbe: eliminate mbuf write on rearm Konstantin Ananyev
2017-04-10 15:59 ` [dpdk-dev] [PATCH v2 2/2] net/ixgbe: remove option to disable offload flags Konstantin Ananyev
2017-04-04 10:29 ` [dpdk-dev] [PATCH " Konstantin Ananyev
2017-03-08 9:42 ` [dpdk-dev] [PATCH 9/9] mbuf: reorder VLAN tci and buffer len fields Olivier Matz
2017-03-29 15:56 ` [dpdk-dev] [PATCH 0/9] mbuf: structure reorganization Olivier Matz
2017-03-29 16:03 ` Morten Brørup
2017-03-29 20:09 ` Bruce Richardson
2017-03-30 9:31 ` Bruce Richardson
2017-03-30 12:02 ` Olivier Matz
2017-03-30 12:23 ` Bruce Richardson
2017-03-30 16:45 ` Ananyev, Konstantin
2017-03-30 16:47 ` Ananyev, Konstantin
2017-03-30 18:06 ` Ananyev, Konstantin
2017-03-31 8:41 ` Olivier Matz
2017-03-31 9:58 ` Ananyev, Konstantin
2017-03-31 1:00 ` Ananyev, Konstantin
2017-03-31 7:21 ` Morten Brørup
2017-03-31 8:26 ` Olivier Matz
2017-03-31 8:41 ` Bruce Richardson
2017-03-31 8:59 ` Olivier Matz
2017-03-31 9:18 ` Ananyev, Konstantin
2017-03-31 9:36 ` Olivier Matz
2017-04-03 16:15 ` Thomas Monjalon
2017-04-04 7:58 ` Olivier MATZ
2017-04-04 8:53 ` Bruce Richardson
2017-03-31 9:23 ` Bruce Richardson
2017-03-31 11:18 ` Nélio Laranjeiro
2017-03-30 14:54 ` Andrew Rybchenko
2017-03-30 15:12 ` Jerin Jacob
2017-04-04 16:27 ` [dpdk-dev] [PATCH v2 0/8] " Olivier Matz
2017-04-04 16:28 ` [dpdk-dev] [PATCH v2 1/8] mbuf: make segment prefree function public Olivier Matz
2017-04-04 16:28 ` [dpdk-dev] [PATCH v2 2/8] mbuf: make raw free " Olivier Matz
2017-04-04 16:28 ` [dpdk-dev] [PATCH v2 3/8] mbuf: set mbuf fields while in pool Olivier Matz
2017-04-04 16:28 ` [dpdk-dev] [PATCH v2 4/8] drivers/net: don't touch mbuf next or nb segs on Rx Olivier Matz
2017-04-04 16:28 ` [dpdk-dev] [PATCH v2 5/8] mbuf: make rearm data address naturally aligned Olivier Matz
2017-04-04 16:28 ` [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port and nb segments Olivier Matz
2017-04-06 5:45 ` Yuanhan Liu
2017-04-18 13:03 ` Olivier MATZ
2017-07-04 7:54 ` Wang, Zhihong
2017-07-10 8:00 ` Olivier Matz
2017-07-10 8:15 ` Morten Brørup
2017-07-11 13:25 ` Wiles, Keith
2017-07-11 13:30 ` Morten Brørup
2017-07-11 15:05 ` Thomas Monjalon
2017-07-11 15:23 ` [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port and nbsegments Morten Brørup
2017-07-11 16:48 ` Wiles, Keith
2017-07-12 7:25 ` Morten Brørup
2017-07-12 9:02 ` Yang, Zhiyong
2017-07-12 9:50 ` [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port andnbsegments Morten Brørup
2017-07-12 15:35 ` Stephen Hemminger
2017-07-12 15:57 ` Morten Brørup
2017-07-12 16:23 ` Thomas Monjalon
2017-07-12 18:20 ` Wiles, Keith
2017-07-21 15:03 ` Bruce Richardson
2017-07-12 15:34 ` Wiles, Keith [this message]
2017-07-11 13:34 ` [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port and nb segments Wiles, Keith
2017-07-11 13:46 ` Olivier MATZ
2017-04-04 16:28 ` [dpdk-dev] [PATCH v2 7/8] mbuf: move sequence number in second cache line Olivier Matz
2017-04-04 16:28 ` [dpdk-dev] [PATCH v2 8/8] mbuf: add a timestamp field Olivier Matz
2017-04-05 9:37 ` [dpdk-dev] [PATCH v2 0/8] mbuf: structure reorganization Thomas Monjalon
2017-04-05 9:46 ` Olivier MATZ
2017-04-05 9:48 ` Richardson, Bruce
2017-04-05 12:06 ` Ferruh Yigit
2017-04-14 13:10 ` Ferruh Yigit
2017-04-18 13:04 ` Olivier MATZ
2017-04-19 9:39 ` Thomas Monjalon
2017-04-19 12:28 ` Olivier MATZ
2017-04-19 12:56 ` Thomas Monjalon
2017-04-19 13:03 ` Ferruh Yigit
2017-04-19 13:12 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=33685046-2702-4AE4-826B-8FE65832127F@intel.com \
--to=keith.wiles@intel.com \
--cc=andrey.chilikin@intel.com \
--cc=arybchenko@solarflare.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=jblunck@infradead.org \
--cc=jerin.jacob@caviumnetworks.com \
--cc=konstantin.ananyev@intel.com \
--cc=mb@smartsharesystems.com \
--cc=nelio.laranjeiro@6wind.com \
--cc=olivier.matz@6wind.com \
--cc=thomas@monjalon.net \
--cc=yuanhan.liu@linux.intel.com \
--cc=zhihong.wang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).