Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/951895/?format=api
{ "id": 951895, "url": "http://patchwork.ozlabs.org/api/patches/951895/?format=api", "web_url": "http://patchwork.ozlabs.org/project/intel-wired-lan/patch/20180801034729.3864-5-anirudh.venkataramanan@intel.com/", "project": { "id": 46, "url": "http://patchwork.ozlabs.org/api/projects/46/?format=api", "name": "Intel Wired Ethernet development", "link_name": "intel-wired-lan", "list_id": "intel-wired-lan.osuosl.org", "list_email": "intel-wired-lan@osuosl.org", "web_url": "", "scm_url": "", "webscm_url": "", "list_archive_url": "", "list_archive_url_format": "", "commit_url_format": "" }, "msgid": "<20180801034729.3864-5-anirudh.venkataramanan@intel.com>", "list_archive_url": null, "date": "2018-08-01T03:47:20", "name": "[v3,04/13] ice: Report stats for allocated queues via ethtool stats", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "2d8956301ca0284154404f834a2c5d4d41c8ede7", "submitter": { "id": 73601, "url": "http://patchwork.ozlabs.org/api/people/73601/?format=api", "name": "Anirudh Venkataramanan", "email": "anirudh.venkataramanan@intel.com" }, "delegate": { "id": 68, "url": "http://patchwork.ozlabs.org/api/users/68/?format=api", "username": "jtkirshe", "first_name": "Jeff", "last_name": "Kirsher", "email": "jeffrey.t.kirsher@intel.com" }, "mbox": "http://patchwork.ozlabs.org/project/intel-wired-lan/patch/20180801034729.3864-5-anirudh.venkataramanan@intel.com/mbox/", "series": [ { "id": 58671, "url": "http://patchwork.ozlabs.org/api/series/58671/?format=api", "web_url": "http://patchwork.ozlabs.org/project/intel-wired-lan/list/?series=58671", "date": "2018-08-01T03:47:18", "name": "Bug fixes for ice", "version": 3, "mbox": "http://patchwork.ozlabs.org/series/58671/mbox/" } ], "comments": "http://patchwork.ozlabs.org/api/patches/951895/comments/", "check": "pending", "checks": "http://patchwork.ozlabs.org/api/patches/951895/checks/", "tags": {}, "related": [], "headers": { "Return-Path": "<intel-wired-lan-bounces@osuosl.org>", "X-Original-To": [ "incoming@patchwork.ozlabs.org", "intel-wired-lan@lists.osuosl.org" ], "Delivered-To": [ "patchwork-incoming@bilbo.ozlabs.org", "intel-wired-lan@lists.osuosl.org" ], "Authentication-Results": [ "ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=osuosl.org\n\t(client-ip=140.211.166.133; helo=hemlock.osuosl.org;\n\tenvelope-from=intel-wired-lan-bounces@osuosl.org;\n\treceiver=<UNKNOWN>)", "ozlabs.org;\n\tdmarc=fail (p=none dis=none) header.from=intel.com" ], "Received": [ "from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 41gK5D1h2Yz9s7Q\n\tfor <incoming@patchwork.ozlabs.org>;\n\tWed, 1 Aug 2018 13:47:43 +1000 (AEST)", "from localhost (localhost [127.0.0.1])\n\tby hemlock.osuosl.org (Postfix) with ESMTP id 5375887F1B;\n\tWed, 1 Aug 2018 03:47:42 +0000 (UTC)", "from hemlock.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id SOGoL2am9w44; Wed, 1 Aug 2018 03:47:41 +0000 (UTC)", "from ash.osuosl.org (ash.osuosl.org [140.211.166.34])\n\tby hemlock.osuosl.org (Postfix) with ESMTP id 84B6587C45;\n\tWed, 1 Aug 2018 03:47:41 +0000 (UTC)", "from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133])\n\tby ash.osuosl.org (Postfix) with ESMTP id 952941C0A9B\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tWed, 1 Aug 2018 03:47:40 +0000 (UTC)", "from localhost (localhost [127.0.0.1])\n\tby hemlock.osuosl.org (Postfix) with ESMTP id 92E9387C45\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tWed, 1 Aug 2018 03:47:40 +0000 (UTC)", "from hemlock.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id NhMZL2bKAaLy for <intel-wired-lan@lists.osuosl.org>;\n\tWed, 1 Aug 2018 03:47:40 +0000 (UTC)", "from mga07.intel.com (mga07.intel.com [134.134.136.100])\n\tby hemlock.osuosl.org (Postfix) with ESMTPS id DBDCB87C41\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tWed, 1 Aug 2018 03:47:39 +0000 (UTC)", "from orsmga004.jf.intel.com ([10.7.209.38])\n\tby orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t31 Jul 2018 20:47:39 -0700", "from lnahar-mobl.amr.corp.intel.com (HELO\n\tavenkata-mobl4.localdomain) ([10.252.134.1])\n\tby orsmga004.jf.intel.com with ESMTP; 31 Jul 2018 20:47:38 -0700" ], "X-Virus-Scanned": [ "amavisd-new at osuosl.org", "amavisd-new at osuosl.org" ], "X-Greylist": "domain auto-whitelisted by SQLgrey-1.7.6", "X-Amp-Result": "SKIPPED(no attachment in message)", "X-Amp-File-Uploaded": "False", "X-ExtLoop1": "1", "X-IronPort-AV": "E=Sophos;i=\"5.51,430,1526367600\"; d=\"scan'208\";a=\"220769240\"", "From": "Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>", "To": "intel-wired-lan@lists.osuosl.org", "Date": "Tue, 31 Jul 2018 20:47:20 -0700", "Message-Id": "<20180801034729.3864-5-anirudh.venkataramanan@intel.com>", "X-Mailer": "git-send-email 2.17.1", "In-Reply-To": "<20180801034729.3864-1-anirudh.venkataramanan@intel.com>", "References": "<20180801034729.3864-1-anirudh.venkataramanan@intel.com>", "Subject": "[Intel-wired-lan] [PATCH v3 04/13] ice: Report stats for allocated\n\tqueues via ethtool stats", "X-BeenThere": "intel-wired-lan@osuosl.org", "X-Mailman-Version": "2.1.24", "Precedence": "list", "List-Id": "Intel Wired Ethernet Linux Kernel Driver Development\n\t<intel-wired-lan.osuosl.org>", "List-Unsubscribe": "<https://lists.osuosl.org/mailman/options/intel-wired-lan>, \n\t<mailto:intel-wired-lan-request@osuosl.org?subject=unsubscribe>", "List-Archive": "<http://lists.osuosl.org/pipermail/intel-wired-lan/>", "List-Post": "<mailto:intel-wired-lan@osuosl.org>", "List-Help": "<mailto:intel-wired-lan-request@osuosl.org?subject=help>", "List-Subscribe": "<https://lists.osuosl.org/mailman/listinfo/intel-wired-lan>, \n\t<mailto:intel-wired-lan-request@osuosl.org?subject=subscribe>", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=\"us-ascii\"", "Content-Transfer-Encoding": "7bit", "Errors-To": "intel-wired-lan-bounces@osuosl.org", "Sender": "\"Intel-wired-lan\" <intel-wired-lan-bounces@osuosl.org>" }, "content": "From: Jacob Keller <jacob.e.keller@intel.com>\n\nIt is not safe to have the string table for statistics change order or\nsize over the lifetime of a given netdevice. This is because of the\nnature of the 3-step process for obtaining stats. First, userspace\nperforms a request for the size of the strings table. Second it performs\na separate request for the strings themselves, after allocating space\nfor the table. Third, it requests the stats themselves, also allocating\nspace for the table.\n\nIf the size decreased, there is potential to see garbage data or stats\nvalues. In the worst case, we could potentially see stats values become\nmis-aligned with their strings, so that it looks like a statistic is\nbeing reported differently than it actually is.\n\nEven worse, if the size increased, there is potential that the strings\ntable or stats table was not allocated large enough and the stats code\ncould access and write to memory it should not, potentially resulting in\nundefined behavior and system crashes.\n\nIt isn't even safe if the size always changes under the RTNL lock. This\nis because the calls take place over multiple userspace commands, so it\nis not possible to hold the RTNL lock for the entire duration of\nobtaining strings and stats. Further, not all consumers of the ethtool\nAPI are the userspace ethtool program, and it is possible that one\nassumes the strings will not change (valid under the current contract),\nand thus only requests the stats values when requesting stats in a loop.\n\nFinally, it's not possible in the general case to detect when the size\nchanges, because it is quite possible that one value which could impact\nthe stat size increased, while another decreased. This would result in\nthe same total number of stats, but reordering them so that stats no\nlonger line up with the strings they belong to. Since only size changes\naren't enough, we would need some sort of hash or token to determine\nwhen the strings no longer match. This would require extending the\nethtool stats commands, but there is no more space in the relevant\nstructures.\n\nThe real solution to resolve this would be to add a completely new API\nfor stats, probably over netlink.\n\nIn the ice driver, the only thing impacting the stats that is not\nconstant is the number of queues. Instead of reporting stats for each\nused queue, report stats for each allocated queue. We do not change the\nnumber of queues allocated for a given netdevice, as we pass this into\nthe alloc_etherdev_mq() function to set the num_tx_queues and\nnum_rx_queues.\n\nThis resolves the potential bugs at the slight cost of displaying many\nqueue statistics which will not be activated.\n\nSigned-off-by: Jacob Keller <jacob.e.keller@intel.com>\nSigned-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>\n---\n[Anirudh Venkataramanan <anirudh.venkataramanan@intel.com> cleaned up commit message]\n---\n drivers/net/ethernet/intel/ice/ice.h | 7 +++\n drivers/net/ethernet/intel/ice/ice_ethtool.c | 52 +++++++++++++++-----\n 2 files changed, 46 insertions(+), 13 deletions(-)", "diff": "diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h\nindex d8b5fff581e7..ed071ea75f20 100644\n--- a/drivers/net/ethernet/intel/ice/ice.h\n+++ b/drivers/net/ethernet/intel/ice/ice.h\n@@ -89,6 +89,13 @@ extern const char ice_drv_ver[];\n #define ice_for_each_rxq(vsi, i) \\\n \tfor ((i) = 0; (i) < (vsi)->num_rxq; (i)++)\n \n+/* Macros for each allocated tx/rx ring whether used or not in a VSI */\n+#define ice_for_each_alloc_txq(vsi, i) \\\n+\tfor ((i) = 0; (i) < (vsi)->alloc_txq; (i)++)\n+\n+#define ice_for_each_alloc_rxq(vsi, i) \\\n+\tfor ((i) = 0; (i) < (vsi)->alloc_rxq; (i)++)\n+\n struct ice_tc_info {\n \tu16 qoffset;\n \tu16 qcount;\ndiff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c\nindex 1db304c01d10..807b683a5707 100644\n--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c\n+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c\n@@ -26,7 +26,7 @@ static int ice_q_stats_len(struct net_device *netdev)\n {\n \tstruct ice_netdev_priv *np = netdev_priv(netdev);\n \n-\treturn ((np->vsi->num_txq + np->vsi->num_rxq) *\n+\treturn ((np->vsi->alloc_txq + np->vsi->alloc_rxq) *\n \t\t(sizeof(struct ice_q_stats) / sizeof(u64)));\n }\n \n@@ -218,7 +218,7 @@ static void ice_get_strings(struct net_device *netdev, u32 stringset, u8 *data)\n \t\t\tp += ETH_GSTRING_LEN;\n \t\t}\n \n-\t\tice_for_each_txq(vsi, i) {\n+\t\tice_for_each_alloc_txq(vsi, i) {\n \t\t\tsnprintf(p, ETH_GSTRING_LEN,\n \t\t\t\t \"tx-queue-%u.tx_packets\", i);\n \t\t\tp += ETH_GSTRING_LEN;\n@@ -226,7 +226,7 @@ static void ice_get_strings(struct net_device *netdev, u32 stringset, u8 *data)\n \t\t\tp += ETH_GSTRING_LEN;\n \t\t}\n \n-\t\tice_for_each_rxq(vsi, i) {\n+\t\tice_for_each_alloc_rxq(vsi, i) {\n \t\t\tsnprintf(p, ETH_GSTRING_LEN,\n \t\t\t\t \"rx-queue-%u.rx_packets\", i);\n \t\t\tp += ETH_GSTRING_LEN;\n@@ -253,6 +253,24 @@ static int ice_get_sset_count(struct net_device *netdev, int sset)\n {\n \tswitch (sset) {\n \tcase ETH_SS_STATS:\n+\t\t/* The number (and order) of strings reported *must* remain\n+\t\t * constant for a given netdevice. This function must not\n+\t\t * report a different number based on run time parameters\n+\t\t * (such as the number of queues in use, or the setting of\n+\t\t * a private ethtool flag). This is due to the nature of the\n+\t\t * ethtool stats API.\n+\t\t *\n+\t\t * Userspace programs such as ethtool must make 3 separate\n+\t\t * ioctl requests, one for size, one for the strings, and\n+\t\t * finally one for the stats. Since these cross into\n+\t\t * userspace, changes to the number or size could result in\n+\t\t * undefined memory access or incorrect string<->value\n+\t\t * correlations for statistics.\n+\t\t *\n+\t\t * Even if it appears to be safe, changes to the size or\n+\t\t * order of strings will suffer from race conditions and are\n+\t\t * not safe.\n+\t\t */\n \t\treturn ICE_ALL_STATS_LEN(netdev);\n \tdefault:\n \t\treturn -EOPNOTSUPP;\n@@ -280,18 +298,26 @@ ice_get_ethtool_stats(struct net_device *netdev,\n \t/* populate per queue stats */\n \trcu_read_lock();\n \n-\tice_for_each_txq(vsi, j) {\n+\tice_for_each_alloc_txq(vsi, j) {\n \t\tring = READ_ONCE(vsi->tx_rings[j]);\n-\t\tif (!ring)\n-\t\t\tcontinue;\n-\t\tdata[i++] = ring->stats.pkts;\n-\t\tdata[i++] = ring->stats.bytes;\n+\t\tif (ring) {\n+\t\t\tdata[i++] = ring->stats.pkts;\n+\t\t\tdata[i++] = ring->stats.bytes;\n+\t\t} else {\n+\t\t\tdata[i++] = 0;\n+\t\t\tdata[i++] = 0;\n+\t\t}\n \t}\n \n-\tice_for_each_rxq(vsi, j) {\n+\tice_for_each_alloc_rxq(vsi, j) {\n \t\tring = READ_ONCE(vsi->rx_rings[j]);\n-\t\tdata[i++] = ring->stats.pkts;\n-\t\tdata[i++] = ring->stats.bytes;\n+\t\tif (ring) {\n+\t\t\tdata[i++] = ring->stats.pkts;\n+\t\t\tdata[i++] = ring->stats.bytes;\n+\t\t} else {\n+\t\t\tdata[i++] = 0;\n+\t\t\tdata[i++] = 0;\n+\t\t}\n \t}\n \n \trcu_read_unlock();\n@@ -519,7 +545,7 @@ ice_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring)\n \t\tgoto done;\n \t}\n \n-\tfor (i = 0; i < vsi->num_txq; i++) {\n+\tfor (i = 0; i < vsi->alloc_txq; i++) {\n \t\t/* clone ring and setup updated count */\n \t\ttx_rings[i] = *vsi->tx_rings[i];\n \t\ttx_rings[i].count = new_tx_cnt;\n@@ -551,7 +577,7 @@ ice_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring)\n \t\tgoto done;\n \t}\n \n-\tfor (i = 0; i < vsi->num_rxq; i++) {\n+\tfor (i = 0; i < vsi->alloc_rxq; i++) {\n \t\t/* clone ring and setup updated count */\n \t\trx_rings[i] = *vsi->rx_rings[i];\n \t\trx_rings[i].count = new_rx_cnt;\n", "prefixes": [ "v3", "04/13" ] }