From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id B9E01C33A for ; Tue, 16 Jun 2015 20:15:25 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga103.jf.intel.com with ESMTP; 16 Jun 2015 11:15:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,627,1427785200"; d="scan'208";a="744601618" Received: from irsmsx106.ger.corp.intel.com ([163.33.3.31]) by fmsmga002.fm.intel.com with ESMTP; 16 Jun 2015 11:15:13 -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; Tue, 16 Jun 2015 19:15:09 +0100 From: "Ananyev, Konstantin" To: "'David Harton (dharton)'" , "Wang, Liang-min" Thread-Topic: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access device info Thread-Index: AQHQo4/Uz84r4LZBA0eIhB09B1a4pJ2nKH/wgAAL/ACAABF3kIAAg7QAgAD8UZCABL/bgIAAEg9wgAAaYwCAABk6EIABqY2w Date: Tue, 16 Jun 2015 18:15:08 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836A11DCE@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> <2601191342CEEE43887BDE71AB97725836A0A9E5@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB97725836A0A9E5@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.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" 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: Tue, 16 Jun 2015 18:15:26 -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. > > > Konstantin > > > > It seems the new API aims at providing users a mechanism to quickly and > > gracefully migrate from using ethtool/ioctl calls. >=20 > I am fine with that goal in general. > But it doesn't mean that all ethool API should be pushed into rte_ethdev = layer. > That's why a shim layer on top of rte_ethdev is created - > it's goal is to provide for the upper layer an ethool-like API and > hide actual implementation based on rte_ethdev API inside. >=20 > > The provided get/set > > ring param info is very similar to that of ethtool and facilitates the > > ethtool needs. >=20 Actually a quick questions to you guys: Looking at linux struct ethtool_ringparam description, I am seeing: http://lxr.free-electrons.com/source/include/uapi/linux/ethtool.h: @rx_pending: Current maximum number of pending entries per RX ring And all linux driver I looked at (ixgbe,i40e,virtio,vmxbet3) returns number of configured RX descriptors for queue 0. In DPDK terms: nb_desc for the queue. While in Larry's patch, ixgbe_get_ringparam() for rx_pending returns number of RX descriptors that are in HW use (for queue 0). So, did you intentionally change linux ethool get_ringparam() behaviour her= e, or is it just a mistake? Konstantin =20