From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <adrien.mazarguil@6wind.com>
Received: from mail-wr0-f182.google.com (mail-wr0-f182.google.com
[209.85.128.182]) by dpdk.org (Postfix) with ESMTP id 0FE437CB3
for <dev@dpdk.org>; Mon, 11 Sep 2017 11:37:13 +0200 (CEST)
Received: by mail-wr0-f182.google.com with SMTP id 108so13273126wra.5
for <dev@dpdk.org>; Mon, 11 Sep 2017 02:37:13 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=6wind-com.20150623.gappssmtp.com; s=20150623;
h=date:from:to:cc:subject:message-id:references:mime-version
:content-disposition:in-reply-to;
bh=k9JkkY9Y5MQN+hCxUnZnLVlLxGf1FwM5mjHjEXLPjWA=;
b=kU8vCo2xOLn0Xz5fKPL7CKZNdeEleX4pQjOFdh5fYWzoCk5aJYlX3jXzqh0O09sAih
MsJSZR5wn9uXU7IlBlBfY41WoNFtj5cjsK9Cris3GB6FX7UOBowWXHwrTIaj79PhZVDv
zzarFowKtnuH2Z2U4V1LCVODCyaAofOVar41d4XbgFbuq3gkUazfQagThPdz9M9QgzW3
rD++0iVSbTiLBWzTsREnxi3kfrTvR1wX5HcO8576Y9FQ6wuvIhi1LFnzNvKre+zwPmze
/DLihBqr+0C3UoaB3u3GhnGNYLuVvOol1WYT/NCNbJIuXADBNP+yfxx3Txv9QoNJccrt
rkSA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20161025;
h=x-gm-message-state:date:from:to:cc:subject:message-id:references
:mime-version:content-disposition:in-reply-to;
bh=k9JkkY9Y5MQN+hCxUnZnLVlLxGf1FwM5mjHjEXLPjWA=;
b=JblyznXjf89O9r2u0ubsf7X3K+Y8myuGsB6ARN3RkSmpNKgiOoQ2RTTGOw28r5MC7q
xtbAT7EyocU8fCFnDAfPtto441w5VyzUF9QKq/2mGP3LcOEa9QTXoc/lydwcLnzf0Ddu
1RP9fWHvw3wqbvZCBPw6poCUT4crgeSSNdEQUY2OezAn7pusqB+xdZsVotxDHbzmZLS9
JLeeaK/Qz0PS+6MKucwkMzhWnk5qNDtyxd/NuZirlbxbKY8eBNDDCRwLlyBMcdkm3LcV
Gfjo4J4sphOGlcBb4PFLhQHuRF0JeUqabuCK2yXYXbGWmkNJHBp//gWo7yIyLeWd7nTF
GvuQ==
X-Gm-Message-State: AHPjjUj5rluuXZt/3bxV/AuVHv14jD0UG+WTid9gFG8GqbQHcLQ3cjZx
y0Kdmu5GeS8bdAvp
X-Google-Smtp-Source: ADKCNb6ansHKuIdbYnOkRIkbs4lDN/i8r8DzQmhTQ78Ql+I4+YvLygbw3crDTutYVcPuvIwOsB5oeA==
X-Received: by 10.223.160.221 with SMTP id n29mr7923847wrn.214.1505122633609;
Mon, 11 Sep 2017 02:37:13 -0700 (PDT)
Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78])
by smtp.gmail.com with ESMTPSA id a19sm14370507wra.64.2017.09.11.02.37.12
(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);
Mon, 11 Sep 2017 02:37:12 -0700 (PDT)
Date: Mon, 11 Sep 2017 11:37:02 +0200
From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Zhiyong Yang <zhiyong.yang@intel.com>
Cc: dev@dpdk.org, thomas@monjalon.net, ferruh.yigit@intel.com,
hemant.agrawal@nxp.com, david.hunt@intel.com
Message-ID: <20170911093702.GD2481@6wind.com>
References: <20170904055734.21354-1-zhiyong.yang@intel.com>
<20170909144727.46388-1-zhiyong.yang@intel.com>
<20170909144727.46388-2-zhiyong.yang@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170909144727.46388-2-zhiyong.yang@intel.com>
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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Mon, 11 Sep 2017 09:37:14 -0000
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.
<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.
What would need change is struct rxq's port_id field (already large enough)
if you want to enforce uint16_t everywhere.
> 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.
> 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.
> 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.
<snip>
--
Adrien Mazarguil
6WIND