From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f48.google.com (mail-wm0-f48.google.com [74.125.82.48]) by dpdk.org (Postfix) with ESMTP id E784C2E8A for ; Wed, 12 Apr 2017 19:11:00 +0200 (CEST) Received: by mail-wm0-f48.google.com with SMTP id u2so29009491wmu.0 for ; Wed, 12 Apr 2017 10:11:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:user-agent:in-reply-to :references:mime-version:content-transfer-encoding; bh=LvJl0sh72KxLrZ32krw6LKPyebr86Z/dXKybmzfy/nA=; b=FIaZ9jMn9qEx4O7HHa2WZSvPliN5UDsnCvnOe2IyQsQaPKI9Dydf9dOYxtkqttzzHx MJVfT7xqF7lOP9xRuM9Dm7nioheoRca5lT2xmsgkpqDbofEeCTLEWuCXJsTQzih3Q6Qp x4nq2lsZF/af0A5q8WQbMBZqHgElo9HKFCQOzVXYPmxUD7kir3ZJgLtBUBLs24izuVft D3mYhakGyetVCwS4wVL8q6HcvmC9T4lj0nopVIqw3LIQG/yzMdp3tyfoNS8qx2sghE1j HY9g84V771Hq7bCj/D6/+78+In64gwdilU020tJ1JgNOol1UUEo1KtPQ2iO/XyUv3riP /1qA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:user-agent :in-reply-to:references:mime-version:content-transfer-encoding; bh=LvJl0sh72KxLrZ32krw6LKPyebr86Z/dXKybmzfy/nA=; b=cgbRruFxXsx26ATZFK+YDz5facIY/LDdVnnKSj8ba7ftSLaNHuHs+54Rmi/i5bFW5E G8ioDCEWZhWmdbBDSPVmEK5dqk6zvPbV4wW5TH2Hdjxe9AOOUTYyLMtlYMLrh8Nf0fBf 42S9TlsCXx/RiNjX4Z3iFDhUoio0+U+pzE5UdlHLqC5Huw/rgu7KShlvApRmZKIoh9Ae c3zC6yiLwdVspLZWCvcntEVZa8+psWNq6JAvAoyBmY2TSXoPsMHaksw9h5ftDmhYpuYp qzbksRovJ/NaVT5tVG1xZ3j8hEOtJeXjqVsyoBNhBmc5E3o8nReJ9XOboycrzbYaU4kd C8eg== X-Gm-Message-State: AN3rC/6STMHQnYTjdE3dwTbIvacaxoRtiNM85qlNkVFL62JTnBc6XowQP8F971S+E+M3VSE5 X-Received: by 10.28.18.21 with SMTP id 21mr3666216wms.77.1492017060594; Wed, 12 Apr 2017 10:11:00 -0700 (PDT) Received: from xps13.localnet (184.203.134.77.rev.sfr.net. [77.134.203.184]) by smtp.gmail.com with ESMTPSA id o71sm11049677wrb.47.2017.04.12.10.10.59 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 12 Apr 2017 10:11:00 -0700 (PDT) From: Thomas Monjalon To: Michal Jastrzebski , harry.van.haaren@intel.com, Jacek Piasecki , Kuba Kozak , Tomasz Kulasek Cc: dev@dpdk.org, deepak.k.jain@intel.com Date: Wed, 12 Apr 2017 19:10:58 +0200 Message-ID: <2044924.BXrfah3NT8@xps13> User-Agent: KMail/4.14.10 (Linux/4.5.4-1-ARCH; KDE/4.14.11; x86_64; ; ) In-Reply-To: <1491928644-10383-2-git-send-email-michalx.k.jastrzebski@intel.com> References: <1491847180-24784-2-git-send-email-jacekx.piasecki@intel.com> <1491928644-10383-1-git-send-email-michalx.k.jastrzebski@intel.com> <1491928644-10383-2-git-send-email-michalx.k.jastrzebski@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v5 1/3] ethdev: new xstats API add retrieving by ID 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: Wed, 12 Apr 2017 17:11:01 -0000 > --- a/app/proc_info/main.c > +++ b/app/proc_info/main.c > " --stats: to display port statistics, enabled by default\n" > " --xstats: to display extended port statistics, disabled by " > "default\n" > - " --metrics: to display derived metrics of the ports, disabled by " > - "default\n" > + " --xstats-name NAME: to display single xstat value by NAME\n" > + " --xstats-ids IDLIST: to display xstat values by id. " > + "The argument is comma-separated list of xstat ids to print out.\n" > " --stats-reset: to reset port statistics\n" > " --xstats-reset: to reset port extended statistics\n" Why removing --metrics? Is it a rebase mistake? Please, could you introduce these new proc_info options in a separate patch? > --- a/doc/guides/prog_guide/poll_mode_drv.rst > +++ b/doc/guides/prog_guide/poll_mode_drv.rst > +The extended statistics API allows a PMD to expose all statistics that are > +available to it, including statistics that are unique to the device. > +Each statistic has three properties ``name``, ``id`` and ``value``: > + > +* ``name``: A human readable string formatted by the scheme detailed below. > +* ``id``: An integer that represents only that statistic. Suggestion: we could say that the id/name pair may change from a device to another one. > --- a/lib/librte_ether/rte_ethdev.c > +++ b/lib/librte_ether/rte_ethdev.c > int > -rte_eth_xstats_get_names(uint8_t port_id, > - struct rte_eth_xstat_name *xstats_names, > - unsigned size) > +rte_eth_xstats_get_id_by_name(uint8_t port_id, const char *xstat_name, > + uint64_t *id) [...] > +/* retrieve ethdev extended statistics */ > +int > +rte_eth_xstats_get_v22(uint8_t port_id, struct rte_eth_xstat *xstats, > + unsigned int n) I do not manage to review the function changes because of the mix of versioning old functions and introduce new ones. It would have been simpler to separate these two steps in separate patches. > --- a/lib/librte_ether/rte_ethdev.h > +++ b/lib/librte_ether/rte_ethdev.h > - eth_xstats_get_names_t xstats_get_names; > - /**< Get names of extended statistics. */ > + eth_xstats_get_names_t xstats_get_names; > + /**< Get names of extended device statistics. */ It seems a space alignment has been removed here. > - * Retrieve names of extended statistics of an Ethernet device. > +* Gets the ID of a statistic from its name. > +* > +* Note this function searches for the statistics using string compares, and > +* as such should not be used on the fast-path. For fast-path retrieval of > +* specific statistics, store the ID as provided in *id* from this function, > +* and pass the ID to rte_eth_xstats_get() Good to note :) > -int rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstat *xstats, > - unsigned n); [...] > +int rte_eth_xstats_get_v1705(uint8_t port_id, uint64_t *ids, uint64_t *values, > + unsigned int n); > + > +int rte_eth_xstats_get(uint8_t port_id, uint64_t *ids, uint64_t *values, > + unsigned int n); It is an API change. Please reference it in the release notes. > --- a/lib/librte_ether/rte_ether_version.map > +++ b/lib/librte_ether/rte_ether_version.map > @@ -159,5 +159,10 @@ DPDK_17.05 { > global: > > rte_eth_find_next; > + rte_eth_xstats_get_names; > + rte_eth_xstats_get; > + rte_eth_xstats_get_all; > + rte_eth_xstats_get_names_all; > + rte_eth_xstats_get_id_by_name; Please keep alphabetical order. Conclusion: The API looks right. I have not checked the implementation and hope there will be less bugs than when introducing the old one ;)