Re: [dpdk-dev] [PATCH v2 1/4] ethdev: increase port_id range

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

Hi Zhiyong,

On Mon, Sep 04, 2017 at 09:47:10AM +0000, Yang, Zhiyong wrote:
> Hi,  Ferruh, Bruce:
> 
> > -----Original Message-----
> > From: Yigit, Ferruh
> > Sent: Monday, September 4, 2017 5:30 PM
> > To: Richardson, Bruce <bruce.richardson@intel.com>; Yang, Zhiyong
> > <zhiyong.yang@intel.com>
> > Cc: dev@dpdk.org; thomas@monjalon.net; Wiles, Keith
> > <keith.wiles@intel.com>; stephen@networkplumber.org
> > Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: increase port_id range
> > 
> > On 9/4/2017 10:06 AM, Bruce Richardson wrote:
> > > On Mon, Sep 04, 2017 at 01:57:31PM +0800, Zhiyong Yang wrote:
> > >> Extend port_id definition from uint8_t to uint16_t in lib and drivers
> > >> data structures, specifically rte_eth_dev_data.  Modify the APIs,
> > >> drivers and app using port_id at the same time except some drivers
> > >> such as MLX4 and MLX5 due to fail to compile them in my server.
> > >>
> > > I think you can change those drivers too - it's not hard to set up
> > > compilation for MLX drivers (instruction in DPDK docs on the website),
> > > and even if you can't compile test them, e.g. dpaa2 drivers, or other
> > > SoC ones, others can do so on your behalf. If you are going to change
> > > drivers, I think you should change all of them across the board.
> > 
> > +1
> 
> OK. I will change them.

I haven't applied the series yet but I think mlx4 doesn't need any
modification to support the new width. mlx5, on the other hand, at least
uses the following field in its data path:

 unsigned int port_id:8;

One related question, why not define a new type (like testpmd's portid_t)
part of rte_ethdev.h? (rte_portid_t?)

I think uint16_t may not last long with virtual ports and all, and when it
becomes necessary, the switch to uint32_t will be painful. A typedef should
also ease the conversion of user applications.

If you choose to use a typedef, I suggest to do so in a separate patch first
(uint8_t => rte_portid_t) before upgrading rte_portid_t to 16 bits in the
subsequent patch. It means the first patch is large but trivial while the
second one is shorter but deals with the complex changes such as the one
needed for mlx5.

Thanks.

-- 
Adrien Mazarguil
6WIND