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

["Yang, Zhiyong" <zhiyong.yang@intel.com>]

Hi, Adrien:
	Thanks for your reviews. Reply inline.

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Monday, September 11, 2017 5:37 PM
> To: Yang, Zhiyong <zhiyong.yang@intel.com>
> Cc: dev@dpdk.org; thomas@monjalon.net; Yigit, Ferruh
> <ferruh.yigit@intel.com>; hemant.agrawal@nxp.com; Hunt, David
> <david.hunt@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 1/4] ethdev: increase port_id range
> 
> Hi Zhiyong,
> 
> On Sat, Sep 09, 2017 at 10:47:24PM +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.
> >
> > Fix some checkpatch issues from the original code and remove some
> > unnecessary cast operations.
> >
> > Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
> 
> Thanks for the mlx4/mlx5 update, I have some comments, see below.
> 
> By the way, since this commit breaks ABI compatibility for pretty much all public
> functions in librte_ether and a few in other places, remember to update
> rte_ethdev_version.map and other affected .map files accordingly.
> I think this change has to be part of this series.

Thanks a lot for your reminder.
> 
> <snip>
> > diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h index
> > c0ade4f1a..fe911c3a7 100644
> > --- a/drivers/net/mlx4/mlx4.h
> > +++ b/drivers/net/mlx4/mlx4.h
> > @@ -334,7 +334,7 @@ struct priv {
> >  	} vlan_filter[MLX4_MAX_VLAN_IDS]; /* VLAN filters table. */
> >  	/* Device properties. */
> >  	uint16_t mtu; /* Configured MTU. */
> > -	uint8_t port; /* Physical port number. */
> > +	uint16_t port; /* Physical port number. */
> >  	unsigned int started:1; /* Device started, flows enabled. */
> >  	unsigned int promisc:1; /* Device in promiscuous mode. */
> >  	unsigned int allmulti:1; /* Device receives all multicast packets.
> > */
> 
> This is a physical port number internal to the device, not DPDK's. Please drop this
> change.

My bad.  Drop it.

> 
> What would need change is struct rxq's port_id field (already large enough) if
> you want to enforce uint16_t everywhere.

Ok.

> 
> > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> > 43c538419..54a2e8a54 100644
> > --- a/drivers/net/mlx5/mlx5.h
> > +++ b/drivers/net/mlx5/mlx5.h
> > @@ -114,7 +114,7 @@ struct priv {
> >  	unsigned int vlan_filter_n; /* Number of configured VLAN filters. */
> >  	/* Device properties. */
> >  	uint16_t mtu; /* Configured MTU. */
> > -	uint8_t port; /* Physical port number. */
> > +	uint16_t port; /* Physical port number. */
> 
> Same comment here.

Drop it.

> 
> >  	unsigned int started:1; /* Device started, flows enabled. */
> >  	unsigned int promisc_req:1; /* Promiscuous mode requested. */
> >  	unsigned int allmulti_req:1; /* All multicast mode requested. */
> > diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> > b/drivers/net/mlx5/mlx5_ethdev.c index b0eb3cdfc..58e7cf571 100644
> > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > @@ -75,7 +75,7 @@ struct ethtool_link_settings {
> >  	uint32_t cmd;
> >  	uint32_t speed;
> >  	uint8_t duplex;
> > -	uint8_t port;
> > +	uint16_t port;
> 
> This is the ethtool interface from the Linux kernel, again this field has to remain
> 8 bits.
> 
> That's the kind of mistakes a dedicated type might have prevented by the way,
> I'm still not convinced that not adding one was the right decision.
> 

Ok. I see.

> >  	uint8_t phy_address;
> >  	uint8_t autoneg;
> >  	uint8_t mdio_support;
> > diff --git a/drivers/net/mlx5/mlx5_rxtx.h
> > b/drivers/net/mlx5/mlx5_rxtx.h index 7de1d1086..c47e54fe0 100644
> > --- a/drivers/net/mlx5/mlx5_rxtx.h
> > +++ b/drivers/net/mlx5/mlx5_rxtx.h
> > @@ -112,7 +112,6 @@ struct rxq {
> >  	unsigned int sges_n:2; /* Log 2 of SGEs (max buffers per packet). */
> >  	unsigned int cqe_n:4; /* Log 2 of CQ elements. */
> >  	unsigned int elts_n:4; /* Log 2 of Mbufs. */
> > -	unsigned int port_id:8;
> >  	unsigned int rss_hash:1; /* RSS hash result is enabled. */
> >  	unsigned int mark:1; /* Marked flow available on the queue. */
> >  	unsigned int pending_err:1; /* CQE error needs to be handled. */ @@
> > -120,6 +119,7 @@ struct rxq {
> >  	unsigned int :6; /* Remaining bits. */
> >  	volatile uint32_t *rq_db;
> >  	volatile uint32_t *cq_db;
> > +	uint16_t port_id;
> >  	uint16_t rq_ci;
> >  	uint16_t rq_pi;
> >  	uint16_t cq_ci;
> 
> This change might have a performance impact. All important fields are found at
> the beginning of this structure for that reason. Since there's not enough room in
> the above bit-field (8 + 6), I guess there's no other choice for now, however you
> should at least update the "Remaining bits" comment.
> 

Ok . Thanks again for your pointing out. Adrien.

Zhiyong 

> <snip>
> 
> --
> Adrien Mazarguil
> 6WIND