From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 68654C4A8 for ; Wed, 17 Jun 2015 19:25:29 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP; 17 Jun 2015 10:25:28 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,634,1427785200"; d="scan'208";a="729358490" Received: from irsmsx106.ger.corp.intel.com ([163.33.3.31]) by fmsmga001.fm.intel.com with ESMTP; 17 Jun 2015 10:25:28 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.245]) by IRSMSX106.ger.corp.intel.com ([169.254.8.121]) with mapi id 14.03.0224.002; Wed, 17 Jun 2015 18:25:26 +0100 From: "Ananyev, Konstantin" To: "Wang, Liang-min" , "'dev@dpdk.org'" Thread-Topic: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access device info Thread-Index: AQHQo4/Uz84r4LZBA0eIhB09B1a4pJ2nKH/wgAAL/ACAABF3kIAAg7QAgAD8UZCABL/bgIAAEg9wgAAEkICAAEA/gIADHKMg Date: Wed, 17 Jun 2015 17:25:26 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836A12349@irsmsx105.ger.corp.intel.com> References: <1432946276-9424-1-git-send-email-liang-min.wang@intel.com> <1433948996-9716-1-git-send-email-liang-min.wang@intel.com> <1433948996-9716-2-git-send-email-liang-min.wang@intel.com> <2601191342CEEE43887BDE71AB97725836A08BA3@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725836A08BD4@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725836A08FD3@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725836A0A7FF@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725836A0A9C9@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB97725836A0A9C9@irsmsx105.ger.corp.intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access device info X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Jun 2015 17:25:30 -0000 ... > > > > > > > > > > + > > > > > > > > > > +int > > > > > > > > > > +rte_eth_dev_get_ringparam(uint8_t port_id, struct > > > > > > > > > > +rte_dev_ring_info > > > > > > > > > > +*info) { > > > > > > > > > > + struct rte_eth_dev *dev; > > > > > > > > > > + > > > > > > > > > > + if (!rte_eth_dev_is_valid_port(port_id)) { > > > > > > > > > > + PMD_DEBUG_TRACE("Invalid port_id=3D%d\n", > > > > > port_id); > > > > > > > > > > + return -ENODEV; > > > > > > > > > > + } > > > > > > > > > > + > > > > > > > > > > + if ((dev=3D &rte_eth_devices[port_id]) =3D=3D NULL) { > > > > > > > > > > + PMD_DEBUG_TRACE("Invalid port device\n"); > > > > > > > > > > + return -ENODEV; > > > > > > > > > > + } > > > > > > > > > > + > > > > > > > > > > + FUNC_PTR_OR_ERR_RET(*dev->dev_ops->get_ringparam, - > > > > > > > > > ENOTSUP); > > > > > > > > > > + return (*dev->dev_ops->get_ringparam)(dev, info); } > > > > > > > > > > > > > > > > > > I think it will be a useful addition to the ethdev API t= o > > > > > > > > > have an ability to retrieve current RX/TX queue parameter= s. > > > > > > > > > Though again, it need to be more generic, so it could be > > > > > > > > > useful for > > > > > > > > > non- ethtool upper layer too. > > > > > > > > > So I suggest to modify it a bit. > > > > > > > > > Something like that: > > > > > > > > > > > > > > > > > > struct rte_eth_tx_queue_info { > > > > > > > > > struct rte_eth_txconf txconf; > > > > > > > > > uint32_t nb_tx_desc; > > > > > > > > > uint32_t nb_max_tx_desc; /*max allowable TXDs for tha= t > > > queue */ > > > > > > > > > uint32_t nb_tx_free; /* number of free TXD= s at the > > > moment > > > > > of > > > > > > > call. > > > > > > > > > */ > > > > > > > > > /* other tx queue data. */ }; > > > > > > > > > > > > > > > > > > int rte_etdev_get_tx_queue_info(portid, queue_id, struct > > > > > > > > > rte_eth_tx_queue_info *qinfo) > > > > > > > > > > > > > > > > > > Then, your upper layer ethtool wrapper, can implement you= rs > > > > > > > > > ethtool_get_ringparam() by: > > > > > > > > > > > > > > > > > > ... > > > > > > > > > struct rte_eth_tx_queue_info qinfo; > > > > > > > > > rte_ethdev_get_tx_queue_info(port, 0, &qinfo); > > > > > > > > > ring_param->tx_pending =3D qinfo.nb_tx_desc - > > > > > > > > > rte_eth_rx_queue_count(port, 0); > > > > > > > > > > > > > > > > > > Or probably even: > > > > > > > > > ring_param->tx_pending =3D qinfo.nb_tx_desc - > > > > > > > > > qinfo.nb_tx_free; > > > > > > > > > > > > > > > > > > Same for RX. > > > > > > > > > > > > > > > > > For now, this descriptor ring information is used by the et= htool op. > > > > > > > > To make this interface simple, i.e. caller doesn't need to > > > > > > > > access other > > > > > > > queue information. > > > > > > > > > > > > > > I just repeat what I said to you in off-line conversation: > > > > > > > ethdev API is not equal ethtool API. > > > > > > > It is ok to add a new function/structure to ethdev if it rea= lly > > > > > > > needed, but we should do mechanical one to one copy. > > > > > > > It is much better to add a function/structure that would be > > > > > > > more generic, and suit other users, not only ethtool. > > > > > > > There is no point to have dozen functions in rte_ethdev API > > > > > > > providing similar information. > > > > > > > BTW, I don't see how API I proposed is much more complex, th= en > > > > > > > yours > > > > > one. > > > > > > The ring parameter is a run-time information which is different > > > > > > than data > > > > > structure described in this discussion. > > > > > > > > > > I don't see how they are different. > > > > > Looking at ixgbe_get_ringparam(), it returns: > > > > > rx_max_pending - that's a static IXGBE PMD value (max possible > > > > > number of RXDs per one queue). > > > > > rx_pending - number of RXD currently in use by the HW for queue 0 > > > > > (that information can be changed at each call). > > > > > > > > > > With the approach I suggesting - you can get same information for > > > > > each RX queue by calling rte_ethdev_get_rx_queue_info() and > > > > > rte_eth_rx_queue_count(). > > > > > Plus you are getting other RX queue data. > > > > > > > > > > Another thing - what is practical usage of the information you > > > > > retrieving now by get_ringparam()? > > > > > Let say it returned to you: rx_max_pending=3D4096; rx_pending=3D1= 28; How > > > > > that information would help to understand what is going on with t= he > > > device? > > > > > Without knowing value of nb_tx_desc for the queue, you can't say= is > > > > > you queue full or not. > > > > > Again, it could be that all your traffic going through some other > > > > > queue (not 0). > > > > > So from my point rte_eth_dev_get_ringparam() usage is very limit= ed, > > > > > and doesn't provide enough information about current queue state. > > > > > > > > > > Same thing applies for TX. > > > > > > > > > > > > > After careful review the suggestion in this comment, and review the > > > existing dpdk source code. > > > > I came to realize that neither rte_ethdev_get_rx_queue_info, > > > > rte_ethdev_get_tx_queue_info, struct rte_eth_rx_queue_info and stru= ct > > > > rte_eth_tx_queue_info are available in the existing dpdk source cod= e. I > > > could not make a patch based upon a set of non- existent API and data > > > structure. > > > > > > Right now, in dpdk.org source code, struct rte_eth_dev_ring_info, st= ruct > > > rte_dev_eeprom_info and struct rte_dev_reg_info don't exist also. > > > Same as all these functions: > > > > > > rte_eth_dev_default_mac_addr_set > > > rte_eth_dev_reg_length > > > rte_eth_dev_reg_info > > > rte_eth_dev_eeprom_length > > > rte_eth_dev_get_eeprom > > > rte_eth_dev_set_eeprom > > > rte_eth_dev_get_ringparam > > > > > > All this is a new API that's you are trying to add. > > > But, by some reason you consider it is ok to add 'struct > > > rte_eth_dev_ring_info', but couldn't add struct > > > 'rte_ethdev_get_tx_queue_info' > > > That just doesn't make any sense to me. > > > In fact, I think our conversation is going in cycles. > > > If you are not happy with the suggested approach, please provide some > > > meaningful reason why. > > > > All the API's and data structure that you have questions in this commen= t are available from the submitted patch, you could run to > see > > if it works. > > What I learned from your comments are a couple of API name and incomple= te data structure description, so I could not make my > > patch according to your suggestion. > > If you strongly feel the API's that you are proposing are useful for ge= t_ringparam(), please create a patch and submit it for > evaluation. >=20 > Ok, I'll try to create a patch in next few days. > Hopefully it will make things clearer to you and fit everyone. > Konstantin Here is the patch I submitted for proposed changes at re_ethdev and PMDs, a= s discussed: http://dpdk.org/dev/patchwork/patch/5482/ Please have a look. With that, get_ringraram for ethtool shim layer, would look something like: int rte_ethtool_get_ringparam(uint8_t port_id, struct ethtool_ringparam *ring_param) { struct rte_eth_rx_qinfo rx_qinfo; struct rte_eth_tx_qinfo tx_qinfo; int status; if ((status =3D rte_eth_rx_queue_info_get (port_id, 0, &rx_= qinfo)) !=3D 0) return status; if ((status =3D rte_eth_tx_queue_info_get (port_id, 0, &tx_q= info)) !=3D 0) return status; memset(ring_param, 0, sizeof(*ring_param) ring_param->rx_pending =3D rx_qinfo.nb_desc; ring_param->tx_pending =3D tx_qinfo.nb_desc; ring_param->rx_max_pending =3D rx_qinfo.max_desc; ring_param->tx_max_pending =3D tx_qinfo.max_desc; return 0; } As you can see, the changes at ethtool are minimal, while it provides desir= ed info. >>From other side, we have much more generic and easily extendable API at rte= _ethdev. Konstantin >=20 > > As coded in this patch, it's a simple and clean interface and most impo= rtant, it can be validated and it works. > > > > > Konstantin > > > > > > > > > > > > > It's the desire of this patch to separate each data structure t= o > > > > > > avoid cross > > > > > dependency. > > > > > > > > > > That's too cryptic to me. > > > > > Could you explain what cross dependency you are talking about? > > > > > > > > > > Konstantin