From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; Mon, 11 Sep 2017 11:37:13 +0200 (CEST) Received: by mail-wr0-f182.google.com with SMTP id 108so13273126wra.5 for ; 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 To: Zhiyong Yang 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 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. > 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. -- Adrien Mazarguil 6WIND