Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/948117/?format=api
{ "id": 948117, "url": "http://patchwork.ozlabs.org/api/patches/948117/?format=api", "web_url": "http://patchwork.ozlabs.org/project/intel-wired-lan/patch/20180723220520.16678-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": "<20180723220520.16678-5-anirudh.venkataramanan@intel.com>", "list_archive_url": null, "date": "2018-07-23T22:05:11", "name": "[v2,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/20180723220520.16678-5-anirudh.venkataramanan@intel.com/mbox/", "series": [ { "id": 57149, "url": "http://patchwork.ozlabs.org/api/series/57149/?format=api", "web_url": "http://patchwork.ozlabs.org/project/intel-wired-lan/list/?series=57149", "date": "2018-07-23T22:05:10", "name": "Bug fixes for ice", "version": 2, "mbox": "http://patchwork.ozlabs.org/series/57149/mbox/" } ], "comments": "http://patchwork.ozlabs.org/api/patches/948117/comments/", "check": "pending", "checks": "http://patchwork.ozlabs.org/api/patches/948117/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.138; helo=whitealder.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 whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138])\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 41ZFvk5Ty4z9s0R\n\tfor <incoming@patchwork.ozlabs.org>;\n\tTue, 24 Jul 2018 08:06:58 +1000 (AEST)", "from localhost (localhost [127.0.0.1])\n\tby whitealder.osuosl.org (Postfix) with ESMTP id 310F687440;\n\tMon, 23 Jul 2018 22:06:57 +0000 (UTC)", "from whitealder.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id 710sM+uM3or1; Mon, 23 Jul 2018 22:06:56 +0000 (UTC)", "from ash.osuosl.org (ash.osuosl.org [140.211.166.34])\n\tby whitealder.osuosl.org (Postfix) with ESMTP id 2128F87384;\n\tMon, 23 Jul 2018 22:06:56 +0000 (UTC)", "from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138])\n\tby ash.osuosl.org (Postfix) with ESMTP id 239571CF315\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tMon, 23 Jul 2018 22:06:32 +0000 (UTC)", "from localhost (localhost [127.0.0.1])\n\tby whitealder.osuosl.org (Postfix) with ESMTP id 217118735F\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tMon, 23 Jul 2018 22:06:32 +0000 (UTC)", "from whitealder.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id dfdE8jBW2B2y for <intel-wired-lan@lists.osuosl.org>;\n\tMon, 23 Jul 2018 22:06:28 +0000 (UTC)", "from mga11.intel.com (mga11.intel.com [192.55.52.93])\n\tby whitealder.osuosl.org (Postfix) with ESMTPS id 4D68E873B8\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tMon, 23 Jul 2018 22:06:28 +0000 (UTC)", "from fmsmga002.fm.intel.com ([10.253.24.26])\n\tby fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t23 Jul 2018 15:06:28 -0700", "from shasta.jf.intel.com ([10.166.241.10])\n\tby fmsmga002.fm.intel.com with ESMTP; 23 Jul 2018 15:05:20 -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,394,1526367600\"; d=\"scan'208\";a=\"69599940\"", "From": "Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>", "To": "intel-wired-lan@lists.osuosl.org", "Date": "Mon, 23 Jul 2018 15:05:11 -0700", "Message-Id": "<20180723220520.16678-5-anirudh.venkataramanan@intel.com>", "X-Mailer": "git-send-email 2.14.3", "In-Reply-To": "<20180723220520.16678-1-anirudh.venkataramanan@intel.com>", "References": "<20180723220520.16678-1-anirudh.venkataramanan@intel.com>", "Subject": "[Intel-wired-lan] [PATCH v2 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": [ "v2", "04/13" ] }