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