Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/989703/?format=api
{ "id": 989703, "url": "http://patchwork.ozlabs.org/api/patches/989703/?format=api", "web_url": "http://patchwork.ozlabs.org/project/intel-wired-lan/patch/20181026174105.11628-10-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": "<20181026174105.11628-10-anirudh.venkataramanan@intel.com>", "list_archive_url": null, "date": "2018-10-26T17:40:58", "name": "[S8,09/16] ice: Fix tx_timeout in PF driver", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "1b2b15875e68ed3f382a52e473391af2a0df4c29", "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/20181026174105.11628-10-anirudh.venkataramanan@intel.com/mbox/", "series": [ { "id": 72783, "url": "http://patchwork.ozlabs.org/api/series/72783/?format=api", "web_url": "http://patchwork.ozlabs.org/project/intel-wired-lan/list/?series=72783", "date": "2018-10-26T17:40:52", "name": "Bug fixes for ice, set 1/2", "version": 1, "mbox": "http://patchwork.ozlabs.org/series/72783/mbox/" } ], "comments": "http://patchwork.ozlabs.org/api/patches/989703/comments/", "check": "pending", "checks": "http://patchwork.ozlabs.org/api/patches/989703/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 ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 42hWk62T7Qz9sDr\n\tfor <incoming@patchwork.ozlabs.org>;\n\tSat, 27 Oct 2018 04:50:38 +1100 (AEDT)", "from localhost (localhost [127.0.0.1])\n\tby fraxinus.osuosl.org (Postfix) with ESMTP id A88A98714A;\n\tFri, 26 Oct 2018 17:50:36 +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 lvtrh3qkMVnw; Fri, 26 Oct 2018 17:50:35 +0000 (UTC)", "from ash.osuosl.org (ash.osuosl.org [140.211.166.34])\n\tby fraxinus.osuosl.org (Postfix) with ESMTP id 22E3B87771;\n\tFri, 26 Oct 2018 17:50:35 +0000 (UTC)", "from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137])\n\tby ash.osuosl.org (Postfix) with ESMTP id 22ABA1BF30F\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tFri, 26 Oct 2018 17:50:29 +0000 (UTC)", "from localhost (localhost [127.0.0.1])\n\tby fraxinus.osuosl.org (Postfix) with ESMTP id 207FF86FEC\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tFri, 26 Oct 2018 17:50:29 +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 7RWSz4VPmkF9 for <intel-wired-lan@lists.osuosl.org>;\n\tFri, 26 Oct 2018 17:50:28 +0000 (UTC)", "from mga03.intel.com (mga03.intel.com [134.134.136.65])\n\tby fraxinus.osuosl.org (Postfix) with ESMTPS id 4BAD186EEE\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tFri, 26 Oct 2018 17:50:28 +0000 (UTC)", "from orsmga004.jf.intel.com ([10.7.209.38])\n\tby orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t26 Oct 2018 10:41:06 -0700", "from shasta.jf.intel.com ([10.166.241.11])\n\tby orsmga004.jf.intel.com with ESMTP; 26 Oct 2018 10:41:05 -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.54,428,1534834800\"; d=\"scan'208\";a=\"244654990\"", "From": "Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>", "To": "intel-wired-lan@lists.osuosl.org", "Date": "Fri, 26 Oct 2018 10:40:58 -0700", "Message-Id": "<20181026174105.11628-10-anirudh.venkataramanan@intel.com>", "X-Mailer": "git-send-email 2.14.3", "In-Reply-To": "<20181026174105.11628-1-anirudh.venkataramanan@intel.com>", "References": "<20181026174105.11628-1-anirudh.venkataramanan@intel.com>", "Subject": "[Intel-wired-lan] [PATCH S8 09/16] ice: Fix tx_timeout in PF driver", "X-BeenThere": "intel-wired-lan@osuosl.org", "X-Mailman-Version": "2.1.29", "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: Brett Creeley <brett.creeley@intel.com>\n\nPrior to this commit the driver was running into tx_timeouts when a\nqueue was stressed enough. This was happening because the HW tail\nand SW tail (NTU) were incorrectly out of sync. Consequently this was\ncausing the HW head to collide with the HW tail, which to the hardware\nmeans that all descriptors posted for Tx have been processed.\n\nDue to the Tx logic used in the driver SW tail and HW tail are allowed\nto be out of sync. This is done as an optimization because it allows the\ndriver to write HW tail as infrequently as possible, while still\nupdating the SW tail index to keep track. However, there are situations\nwhere this results in the tail never getting updated, resulting in Tx\ntimeouts.\n\nTx HW tail write condition:\n\tif (netif_xmit_stopped(txring_txq(tx_ring) || !skb->xmit_more)\n\t\twritel(sw_tail, tx_ring->tail);\n\nAn issue was found in the Tx logic that was causing the aformentioned\ncondition for updating HW tail to never happen, causing tx_timeouts.\n\nIn ice_xmit_frame_ring we calculate how many descriptors we need for the\nTx transaction based on the skb the kernel hands us. This is then passed\ninto ice_maybe_stop_tx along with some extra padding to determine if we\nhave enough descriptors available for this transaction. If we don't then\nwe return -EBUSY to the stack, otherwise we move on and eventually\nprepare the Tx descriptors accordingly in ice_tx_map and set\nnext_to_watch. In ice_tx_map we make another call to ice_maybe_stop_tx\nwith a value of MAX_SKB_FRAGS + 4. The key here is that this value is\npossibly less than the value we sent in the first call to\nice_maybe_stop_tx in ice_xmit_frame_ring. Now, if the number of unused\ndescriptors is between MAX_SKB_FRAGS + 4 and the value used in the first\ncall to ice_maybe_stop_tx in ice_xmit_frame_ring then we do not update\nthe HW tail because of the \"Tx HW tail write condition\" above. This is\nbecause in ice_maybe_stop_tx we return success from ice_maybe_stop_tx\ninstead of calling __ice_maybe_stop_tx and subsequently calling\nnetif_stop_subqueue, which sets the __QUEUE_STATE_DEV_XOFF bit. This\nbit is then checked in the \"Tx HW tail write condition\" by calling\nnetif_xmit_stopped and subsequently updating HW tail if the\naformentioned bit is set.\n\nIn ice_clean_tx_irq, if next_to_watch is not NULL, we end up cleaning\nthe descriptors that HW sets the DD bit on and we have the budget. The\nHW head will eventuallly run into the HW tail in response to the\ndescription in the paragraph above.\n\nThe next time through ice_xmit_frame_ring we make the initial call to\nice_maybe_stop_tx with another skb from the stack. This time we do not\nhave enough descriptors available and we return NETDEV_TX_BUSY to the\nstack and end up setting next_to_watch to NULL.\n\nThis is where we are stuck. In ice_clean_tx_irq we never clean anything\nbecause next_to_watch is always NULL and in ice_xmit_frame_ring we never\nupdate HW tail because we already return NETDEV_TX_BUSY to the stack and\neventually we hit a tx_timeout.\n\nThis issue was fixed by making sure that the second call to\nice_maybe_stop_tx in ice_tx_map is passed a value that is >= the value\nthat was used on the initial call to ice_maybe_stop_tx in\nice_xmit_frame_ring. This was done by adding the following defines to\nmake the logic more clear and to reduce the chance of mucking this up\nagain:\n\nICE_CACHE_LINE_BYTES\t\t64\nICE_DESCS_PER_CACHE_LINE\t(ICE_CACHE_LINE_BYTES / \\\n\t\t\t\t sizeof(struct ice_tx_desc))\nICE_DESCS_FOR_CTX_DESC\t\t1\nICE_DESCS_FOR_SKB_DATA_PTR\t1\n\nThe ICE_CACHE_LINE_BYTES being 64 is an assumption being made so we\ndon't have to figure this out on every pass through the Tx path. Instead\nI added a sanity check in ice_probe to verify cache line size and print\na message if it's not 64 Bytes. This will make it easier to file issues\nif they are seen when the cache line size is not 64 Bytes when reading\nfrom the GLPCI_CNF2 register.\n\nSigned-off-by: Brett Creeley <brett.creeley@intel.com>\nSigned-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>\n---\n drivers/net/ethernet/intel/ice/ice_hw_autogen.h | 2 ++\n drivers/net/ethernet/intel/ice/ice_main.c | 18 ++++++++++++++++++\n drivers/net/ethernet/intel/ice/ice_txrx.c | 9 +++++----\n drivers/net/ethernet/intel/ice/ice_txrx.h | 17 +++++++++++++++--\n 4 files changed, 40 insertions(+), 6 deletions(-)", "diff": "diff --git a/drivers/net/ethernet/intel/ice/ice_hw_autogen.h b/drivers/net/ethernet/intel/ice/ice_hw_autogen.h\nindex 5fdea6ec7675..596b9fb1c510 100644\n--- a/drivers/net/ethernet/intel/ice/ice_hw_autogen.h\n+++ b/drivers/net/ethernet/intel/ice/ice_hw_autogen.h\n@@ -242,6 +242,8 @@\n #define GLNVM_ULD\t\t\t\t0x000B6008\n #define GLNVM_ULD_CORER_DONE_M\t\t\tBIT(3)\n #define GLNVM_ULD_GLOBR_DONE_M\t\t\tBIT(4)\n+#define GLPCI_CNF2\t\t\t\t0x000BE004\n+#define GLPCI_CNF2_CACHELINE_SIZE_M\t\tBIT(1)\n #define PF_FUNC_RID\t\t\t\t0x0009E880\n #define PF_FUNC_RID_FUNC_NUM_S\t\t\t0\n #define PF_FUNC_RID_FUNC_NUM_M\t\t\tICE_M(0x7, 0)\ndiff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c\nindex 82f49dbd762c..333312a1d595 100644\n--- a/drivers/net/ethernet/intel/ice/ice_main.c\n+++ b/drivers/net/ethernet/intel/ice/ice_main.c\n@@ -1994,6 +1994,22 @@ static int ice_init_interrupt_scheme(struct ice_pf *pf)\n \treturn 0;\n }\n \n+/**\n+ * ice_verify_cacheline_size - verify driver's assumption of 64 Byte cache lines\n+ * @pf: pointer to the PF structure\n+ *\n+ * There is no error returned here because the driver should be able to handle\n+ * 128 Byte cache lines, so we only print a warning in case issues are seen,\n+ * specifically with Tx.\n+ */\n+static void ice_verify_cacheline_size(struct ice_pf *pf)\n+{\n+\tif (rd32(&pf->hw, GLPCI_CNF2) & GLPCI_CNF2_CACHELINE_SIZE_M)\n+\t\tdev_warn(&pf->pdev->dev,\n+\t\t\t \"%d Byte cache line assumption is invalid, driver may have Tx timeouts!\\n\",\n+\t\t\t ICE_CACHE_LINE_BYTES);\n+}\n+\n /**\n * ice_probe - Device initialization routine\n * @pdev: PCI device information struct\n@@ -2144,6 +2160,8 @@ static int ice_probe(struct pci_dev *pdev,\n \t/* since everything is good, start the service timer */\n \tmod_timer(&pf->serv_tmr, round_jiffies(jiffies + pf->serv_tmr_period));\n \n+\tice_verify_cacheline_size(pf);\n+\n \treturn 0;\n \n err_alloc_sw_unroll:\ndiff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c\nindex 5dae968d853e..3387c67c848d 100644\n--- a/drivers/net/ethernet/intel/ice/ice_txrx.c\n+++ b/drivers/net/ethernet/intel/ice/ice_txrx.c\n@@ -1556,15 +1556,15 @@ int ice_tso(struct ice_tx_buf *first, struct ice_tx_offload_params *off)\n * magnitude greater than our largest possible GSO size.\n *\n * This would then be implemented as:\n- * return (((size >> 12) * 85) >> 8) + 1;\n+ * return (((size >> 12) * 85) >> 8) + ICE_DESCS_FOR_SKB_DATA_PTR;\n *\n * Since multiplication and division are commutative, we can reorder\n * operations into:\n- * return ((size * 85) >> 20) + 1;\n+ * return ((size * 85) >> 20) + ICE_DESCS_FOR_SKB_DATA_PTR;\n */\n static unsigned int ice_txd_use_count(unsigned int size)\n {\n-\treturn ((size * 85) >> 20) + 1;\n+\treturn ((size * 85) >> 20) + ICE_DESCS_FOR_SKB_DATA_PTR;\n }\n \n /**\n@@ -1706,7 +1706,8 @@ ice_xmit_frame_ring(struct sk_buff *skb, struct ice_ring *tx_ring)\n \t * + 1 desc for context descriptor,\n \t * otherwise try next time\n \t */\n-\tif (ice_maybe_stop_tx(tx_ring, count + 4 + 1)) {\n+\tif (ice_maybe_stop_tx(tx_ring, count + ICE_DESCS_PER_CACHE_LINE +\n+\t\t\t ICE_DESCS_FOR_CTX_DESC)) {\n \t\ttx_ring->tx_stats.tx_busy++;\n \t\treturn NETDEV_TX_BUSY;\n \t}\ndiff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h\nindex 1d0f58bd389b..75d0eaf6c9dd 100644\n--- a/drivers/net/ethernet/intel/ice/ice_txrx.h\n+++ b/drivers/net/ethernet/intel/ice/ice_txrx.h\n@@ -22,8 +22,21 @@\n #define ICE_RX_BUF_WRITE\t16\t/* Must be power of 2 */\n #define ICE_MAX_TXQ_PER_TXQG\t128\n \n-/* Tx Descriptors needed, worst case */\n-#define DESC_NEEDED (MAX_SKB_FRAGS + 4)\n+/* We are assuming that the cache line is always 64 Bytes here for ice.\n+ * In order to make sure that is a correct assumption there is a check in probe\n+ * to print a warning if the read from GLPCI_CNF2 tells us that the cache line\n+ * size is 128 bytes. We do it this way because we do not want to read the\n+ * GLPCI_CNF2 register or a variable containing the value on every pass through\n+ * the Tx path.\n+ */\n+#define ICE_CACHE_LINE_BYTES\t\t64\n+#define ICE_DESCS_PER_CACHE_LINE\t(ICE_CACHE_LINE_BYTES / \\\n+\t\t\t\t\t sizeof(struct ice_tx_desc))\n+#define ICE_DESCS_FOR_CTX_DESC\t\t1\n+#define ICE_DESCS_FOR_SKB_DATA_PTR\t1\n+/* Tx descriptors needed, worst case */\n+#define DESC_NEEDED (MAX_SKB_FRAGS + ICE_DESCS_FOR_CTX_DESC + \\\n+\t\t ICE_DESCS_PER_CACHE_LINE + ICE_DESCS_FOR_SKB_DATA_PTR)\n #define ICE_DESC_UNUSED(R)\t\\\n \t((((R)->next_to_clean > (R)->next_to_use) ? 0 : (R)->count) + \\\n \t(R)->next_to_clean - (R)->next_to_use - 1)\n", "prefixes": [ "S8", "09/16" ] }