From: Stephen Hemminger <stephen@networkplumber.org>
To: "Morten Brørup" <mb@smartsharesystems.com>
Cc: "Yang, Zhiyong" <zhiyong.yang@intel.com>,
"Wiles, Keith" <keith.wiles@intel.com>,
"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, jerin.jacob@caviumnetworks.com
Subject: Re: [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port andnbsegments
Date: Wed, 12 Jul 2017 08:35:50 -0700 [thread overview]
Message-ID: <20170712083550.62bd9442@xeon-e3> (raw)
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC359EB28F@smartserver.smartshare.dk>
On Wed, 12 Jul 2017 11:50:38 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:
> > -----Original Message-----
> > From: Yang, Zhiyong [mailto:zhiyong.yang@intel.com]
> > Sent: Wednesday, July 12, 2017 11:02 AM
> > To: Morten Brørup; Wiles, Keith
> > 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
> > andnbsegments
> >
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Morten Brørup
> > > Sent: Wednesday, July 12, 2017 3:25 PM
> > > To: Wiles, Keith <keith.wiles@intel.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;
> > > jerin.jacob@caviumnetworks.com
> > > Subject: Re: [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port and
> > > nbsegments
> > >
> > > > -----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).
> > >
> > > >
> > > > 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).
> > >
> >
> > How about rte_dev_id_t?
> >
> > Thanks
> > Zhiyong
> >
> > >
> > > >
> > > > Regards,
> > > > Keith
>
> 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.
next prev parent reply other threads:[~2017-07-12 15:36 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 [this message]
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 ` [dpdk-dev] [PATCH v2 6/8] mbuf: use 2 bytes for port and nbsegments Wiles, Keith
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=20170712083550.62bd9442@xeon-e3 \
--to=stephen@networkplumber.org \
--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=keith.wiles@intel.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 \
--cc=zhiyong.yang@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).