Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/955592/?format=api
{ "id": 955592, "url": "http://patchwork.ozlabs.org/api/patches/955592/?format=api", "web_url": "http://patchwork.ozlabs.org/project/intel-wired-lan/patch/20180809132903.22819-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": "<20180809132903.22819-5-anirudh.venkataramanan@intel.com>", "list_archive_url": null, "date": "2018-08-09T13:28:54", "name": "[v4,04/13] ice: Report stats for allocated queues via ethtool stats", "commit_ref": null, "pull_url": null, "state": "accepted", "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/20180809132903.22819-5-anirudh.venkataramanan@intel.com/mbox/", "series": [ { "id": 60077, "url": "http://patchwork.ozlabs.org/api/series/60077/?format=api", "web_url": "http://patchwork.ozlabs.org/project/intel-wired-lan/list/?series=60077", "date": "2018-08-09T13:28:50", "name": "Bug fixes for ice", "version": 4, "mbox": "http://patchwork.ozlabs.org/series/60077/mbox/" } ], "comments": "http://patchwork.ozlabs.org/api/patches/955592/comments/", "check": "pending", "checks": "http://patchwork.ozlabs.org/api/patches/955592/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.137; helo=fraxinus.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 fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137])\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 41mTcn2jsvz9s7Q\n\tfor <incoming@patchwork.ozlabs.org>;\n\tThu, 9 Aug 2018 23:29:29 +1000 (AEST)", "from localhost (localhost [127.0.0.1])\n\tby fraxinus.osuosl.org (Postfix) with ESMTP id C0F3586696;\n\tThu, 9 Aug 2018 13:29:27 +0000 (UTC)", "from fraxinus.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id a6RSHxeJx5cv; Thu, 9 Aug 2018 13:29:27 +0000 (UTC)", "from ash.osuosl.org (ash.osuosl.org [140.211.166.34])\n\tby fraxinus.osuosl.org (Postfix) with ESMTP id 0844A8666B;\n\tThu, 9 Aug 2018 13:29:27 +0000 (UTC)", "from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136])\n\tby ash.osuosl.org (Postfix) with ESMTP id 8B3C11C3F93\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tThu, 9 Aug 2018 13:29:24 +0000 (UTC)", "from localhost (localhost [127.0.0.1])\n\tby silver.osuosl.org (Postfix) with ESMTP id 892E529350\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tThu, 9 Aug 2018 13:29:24 +0000 (UTC)", "from silver.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id 3uM487TFCoh1 for <intel-wired-lan@lists.osuosl.org>;\n\tThu, 9 Aug 2018 13:29:23 +0000 (UTC)", "from mga14.intel.com (mga14.intel.com [192.55.52.115])\n\tby silver.osuosl.org (Postfix) with ESMTPS id 94B78220E2\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tThu, 9 Aug 2018 13:29:19 +0000 (UTC)", "from orsmga004.jf.intel.com ([10.7.209.38])\n\tby fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t09 Aug 2018 06:29:19 -0700", "from kyungmin-mobl.amr.corp.intel.com (HELO\n\tavenkata-mobl4.localdomain) ([10.254.101.153])\n\tby orsmga004.jf.intel.com with ESMTP; 09 Aug 2018 06:29:17 -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.53,215,1531810800\"; d=\"scan'208\";a=\"223273539\"", "From": "Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>", "To": "intel-wired-lan@lists.osuosl.org", "Date": "Thu, 9 Aug 2018 06:28:54 -0700", "Message-Id": "<20180809132903.22819-5-anirudh.venkataramanan@intel.com>", "X-Mailer": "git-send-email 2.17.1", "In-Reply-To": "<20180809132903.22819-1-anirudh.venkataramanan@intel.com>", "References": "<20180809132903.22819-1-anirudh.venkataramanan@intel.com>", "Subject": "[Intel-wired-lan] [PATCH v4 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": [ "v4", "04/13" ] }