From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Ananyev, Konstantin" Subject: Re: [PATCH v4 1/4] ethdev: add apis to support access device info 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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable To: "Wang, Liang-min" , "'dev@dpdk.org'" 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) In-Reply-To: <2601191342CEEE43887BDE71AB97725836A0A9C9@irsmsx105.ger.corp.intel.com> Content-Language: en-US List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" ... > > > > > > > > > > + > > > > > > > > > > +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