From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 055736D45 for ; Mon, 11 Sep 2017 12:51:27 +0200 (CEST) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Sep 2017 03:51:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,377,1500966000"; d="scan'208";a="147794453" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga005.jf.intel.com with ESMTP; 11 Sep 2017 03:51:25 -0700 Received: from fmsmsx101.amr.corp.intel.com (10.18.124.199) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 11 Sep 2017 03:51:25 -0700 Received: from bgsmsx151.gar.corp.intel.com (10.224.48.42) by fmsmsx101.amr.corp.intel.com (10.18.124.199) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 11 Sep 2017 03:51:25 -0700 Received: from bgsmsx101.gar.corp.intel.com ([169.254.1.62]) by BGSMSX151.gar.corp.intel.com ([169.254.3.33]) with mapi id 14.03.0319.002; Mon, 11 Sep 2017 16:21:19 +0530 From: "Yang, Zhiyong" To: Adrien Mazarguil CC: "dev@dpdk.org" , "thomas@monjalon.net" , "Yigit, Ferruh" , "hemant.agrawal@nxp.com" , "Hunt, David" Thread-Topic: [dpdk-dev] [PATCH v3 1/4] ethdev: increase port_id range Thread-Index: AQHTKXqd4g7AxcbQ90ahh5YYH5gwKqKvE+IAgABsPAA= Date: Mon, 11 Sep 2017 10:51:18 +0000 Message-ID: References: <20170904055734.21354-1-zhiyong.yang@intel.com> <20170909144727.46388-1-zhiyong.yang@intel.com> <20170909144727.46388-2-zhiyong.yang@intel.com> <20170911093702.GD2481@6wind.com> In-Reply-To: <20170911093702.GD2481@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOTJhYTRiZjMtMDFmNi00YmQ5LWJiYmItYTU5Yjc1ZDI5NWExIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IjdQTnFhMUMwRDhQb2hVUHdJa2JGNlZVM0NUYUpNU2tSNjhzVjlnOWRkemc9In0= x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.223.10.10] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3 1/4] ethdev: increase port_id range X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Sep 2017 10:51:28 -0000 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 > Cc: dev@dpdk.org; thomas@monjalon.net; Yigit, Ferruh > ; hemant.agrawal@nxp.com; Hunt, David > > Subject: Re: [dpdk-dev] [PATCH v3 1/4] ethdev: increase port_id range >=20 > Hi Zhiyong, >=20 > 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 >=20 > Thanks for the mlx4/mlx5 update, I have some comments, see below. >=20 > By the way, since this commit breaks ABI compatibility for pretty much al= l 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. >=20 > > > 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. > > */ >=20 > This is a physical port number internal to the device, not DPDK's. Please= drop this > change. My bad. Drop it. >=20 > What would need change is struct rxq's port_id field (already large enoug= h) if > you want to enforce uint16_t everywhere. Ok. >=20 > > 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. */ >=20 > Same comment here. Drop it. >=20 > > 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; >=20 > This is the ethtool interface from the Linux kernel, again this field has= to remain > 8 bits. >=20 > 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. >=20 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; >=20 > This change might have a performance impact. All important fields are fou= nd 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, how= ever you > should at least update the "Remaining bits" comment. >=20 Ok . Thanks again for your pointing out. Adrien. Zhiyong=20 > >=20 > -- > Adrien Mazarguil > 6WIND