netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
To: davem@davemloft.net
Cc: Alexander Duyck <alexander.h.duyck@intel.com>,
	netdev@vger.kernel.org, nhorman@redhat.com, sassmann@redhat.com,
	jogreene@redhat.com, Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Subject: [net-next 03/12] i40e/i40evf: Clean up logic for adaptive ITR
Date: Mon, 12 Feb 2018 12:46:40 -0800	[thread overview]
Message-ID: <20180212204649.24178-4-jeffrey.t.kirsher@intel.com> (raw)
In-Reply-To: <20180212204649.24178-1-jeffrey.t.kirsher@intel.com>

From: Alexander Duyck <alexander.h.duyck@intel.com>

The logic for dynamic ITR update is confusing at best as there were odd
paths chosen for how to find the rings associated with a given queue based
on the vector index and other inconsistencies throughout the code.

This patch is an attempt to clean up the logic so that we can more easily
understand what is going on. Specifically if there is a Rx or Tx ring that
is enabled in dynamic mode on the q_vector it is allowed to override the
other side of the interrupt moderation. While it isn't correct all this
patch is doing is cleaning up the logic for now so that when we come
through and fix it we can more easily identify that this is wrong.

The other big change made here is that we replace references to:
	vsi->rx_rings[q_vector->v_idx]->itr_setting
with:
	q_vector->rx.ring->itr_setting

The general idea is we can avoid the long pointer chase since just
accessing q_vector->rx.ring is a single pointer access versus having to
chase down vsi->rx_rings, and then finding the pointer in the array, and
finally chasing down the itr_setting from there.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 55 +++++++------------------
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 59 +++++++--------------------
 2 files changed, 28 insertions(+), 86 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index b9121a87ee46..6b3fe5d26dc1 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -1016,6 +1016,9 @@ static bool i40e_set_new_dynamic_itr(struct i40e_ring_container *rc)
 	int bytes_per_usec;
 	unsigned int usecs, estimated_usecs;
 
+	if (!rc->ring || !ITR_IS_DYNAMIC(rc->ring->itr_setting))
+		return false;
+
 	if (rc->total_packets == 0 || !rc->itr)
 		return false;
 
@@ -2288,15 +2291,6 @@ static u32 i40e_buildreg_itr(const int type, const u16 itr)
 
 /* a small macro to shorten up some long lines */
 #define INTREG I40E_PFINT_DYN_CTLN
-static inline int get_rx_itr(struct i40e_vsi *vsi, int idx)
-{
-	return vsi->rx_rings[idx]->itr_setting;
-}
-
-static inline int get_tx_itr(struct i40e_vsi *vsi, int idx)
-{
-	return vsi->tx_rings[idx]->itr_setting;
-}
 
 /**
  * i40e_update_enable_itr - Update itr and re-enable MSIX interrupt
@@ -2309,9 +2303,7 @@ static inline void i40e_update_enable_itr(struct i40e_vsi *vsi,
 {
 	struct i40e_hw *hw = &vsi->back->hw;
 	bool rx = false, tx = false;
-	u32 rxval, txval;
-	int idx = q_vector->v_idx;
-	int rx_itr_setting, tx_itr_setting;
+	u32 txval;
 
 	/* If we don't have MSIX, then we only need to re-enable icr0 */
 	if (!(vsi->back->flags & I40E_FLAG_MSIX_ENABLED)) {
@@ -2319,29 +2311,15 @@ static inline void i40e_update_enable_itr(struct i40e_vsi *vsi,
 		return;
 	}
 
-	/* avoid dynamic calculation if in countdown mode OR if
-	 * all dynamic is disabled
-	 */
 	txval = i40e_buildreg_itr(I40E_ITR_NONE, 0);
 
-	rx_itr_setting = get_rx_itr(vsi, idx);
-	tx_itr_setting = get_tx_itr(vsi, idx);
-
-	if (q_vector->itr_countdown > 0 ||
-	    (!ITR_IS_DYNAMIC(rx_itr_setting) &&
-	     !ITR_IS_DYNAMIC(tx_itr_setting))) {
+	/* avoid dynamic calculation if in countdown mode */
+	if (q_vector->itr_countdown > 0)
 		goto enable_int;
-	}
 
-	if (ITR_IS_DYNAMIC(rx_itr_setting)) {
-		rx = i40e_set_new_dynamic_itr(&q_vector->rx);
-		rxval = i40e_buildreg_itr(I40E_RX_ITR, q_vector->rx.itr);
-	}
-
-	if (ITR_IS_DYNAMIC(tx_itr_setting)) {
-		tx = i40e_set_new_dynamic_itr(&q_vector->tx);
-		txval = i40e_buildreg_itr(I40E_TX_ITR, q_vector->tx.itr);
-	}
+	/* these will return false if dynamic mode is disabled */
+	rx = i40e_set_new_dynamic_itr(&q_vector->rx);
+	tx = i40e_set_new_dynamic_itr(&q_vector->tx);
 
 	if (rx || tx) {
 		/* get the higher of the two ITR adjustments and
@@ -2349,25 +2327,20 @@ static inline void i40e_update_enable_itr(struct i40e_vsi *vsi,
 		 * when in adaptive mode (Rx and/or Tx)
 		 */
 		u16 itr = max(q_vector->tx.itr, q_vector->rx.itr);
+		u32 rxval;
 
 		q_vector->tx.itr = q_vector->rx.itr = itr;
-		txval = i40e_buildreg_itr(I40E_TX_ITR, itr);
-		tx = true;
-		rxval = i40e_buildreg_itr(I40E_RX_ITR, itr);
-		rx = true;
-	}
 
-	/* only need to enable the interrupt once, but need
-	 * to possibly update both ITR values
-	 */
-	if (rx) {
 		/* set the INTENA_MSK_MASK so that this first write
 		 * won't actually enable the interrupt, instead just
 		 * updating the ITR (it's bit 31 PF and VF)
 		 */
-		rxval |= BIT(31);
+		rxval = i40e_buildreg_itr(I40E_RX_ITR, itr) | BIT(31);
+
 		/* don't check _DOWN because interrupt isn't being enabled */
 		wr32(hw, INTREG(q_vector->reg_idx), rxval);
+
+		txval = i40e_buildreg_itr(I40E_TX_ITR, itr);
 	}
 
 enable_int:
diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
index 291130af2985..3fd7e9731f49 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
@@ -413,6 +413,9 @@ static bool i40e_set_new_dynamic_itr(struct i40e_ring_container *rc)
 	int bytes_per_usec;
 	unsigned int usecs, estimated_usecs;
 
+	if (!rc->ring || !ITR_IS_DYNAMIC(rc->ring->itr_setting))
+		return false;
+
 	if (rc->total_packets == 0 || !rc->itr)
 		return false;
 
@@ -1471,19 +1474,6 @@ static u32 i40e_buildreg_itr(const int type, const u16 itr)
 
 /* a small macro to shorten up some long lines */
 #define INTREG I40E_VFINT_DYN_CTLN1
-static inline int get_rx_itr(struct i40e_vsi *vsi, int idx)
-{
-	struct i40evf_adapter *adapter = vsi->back;
-
-	return adapter->rx_rings[idx].itr_setting;
-}
-
-static inline int get_tx_itr(struct i40e_vsi *vsi, int idx)
-{
-	struct i40evf_adapter *adapter = vsi->back;
-
-	return adapter->tx_rings[idx].itr_setting;
-}
 
 /**
  * i40e_update_enable_itr - Update itr and re-enable MSIX interrupt
@@ -1496,33 +1486,17 @@ static inline void i40e_update_enable_itr(struct i40e_vsi *vsi,
 {
 	struct i40e_hw *hw = &vsi->back->hw;
 	bool rx = false, tx = false;
-	u32 rxval, txval;
-	int idx = q_vector->v_idx;
-	int rx_itr_setting, tx_itr_setting;
+	u32 txval;
 
-	/* avoid dynamic calculation if in countdown mode OR if
-	 * all dynamic is disabled
-	 */
 	txval = i40e_buildreg_itr(I40E_ITR_NONE, 0);
 
-	rx_itr_setting = get_rx_itr(vsi, idx);
-	tx_itr_setting = get_tx_itr(vsi, idx);
-
-	if (q_vector->itr_countdown > 0 ||
-	    (!ITR_IS_DYNAMIC(rx_itr_setting) &&
-	     !ITR_IS_DYNAMIC(tx_itr_setting))) {
+	/* avoid dynamic calculation if in countdown mode */
+	if (q_vector->itr_countdown > 0)
 		goto enable_int;
-	}
 
-	if (ITR_IS_DYNAMIC(rx_itr_setting)) {
-		rx = i40e_set_new_dynamic_itr(&q_vector->rx);
-		rxval = i40e_buildreg_itr(I40E_RX_ITR, q_vector->rx.itr);
-	}
-
-	if (ITR_IS_DYNAMIC(tx_itr_setting)) {
-		tx = i40e_set_new_dynamic_itr(&q_vector->tx);
-		txval = i40e_buildreg_itr(I40E_TX_ITR, q_vector->tx.itr);
-	}
+	/* these will return false if dynamic mode is disabled */
+	rx = i40e_set_new_dynamic_itr(&q_vector->rx);
+	tx = i40e_set_new_dynamic_itr(&q_vector->tx);
 
 	if (rx || tx) {
 		/* get the higher of the two ITR adjustments and
@@ -1530,25 +1504,20 @@ static inline void i40e_update_enable_itr(struct i40e_vsi *vsi,
 		 * when in adaptive mode (Rx and/or Tx)
 		 */
 		u16 itr = max(q_vector->tx.itr, q_vector->rx.itr);
+		u32 rxval;
 
 		q_vector->tx.itr = q_vector->rx.itr = itr;
-		txval = i40e_buildreg_itr(I40E_TX_ITR, itr);
-		tx = true;
-		rxval = i40e_buildreg_itr(I40E_RX_ITR, itr);
-		rx = true;
-	}
 
-	/* only need to enable the interrupt once, but need
-	 * to possibly update both ITR values
-	 */
-	if (rx) {
 		/* set the INTENA_MSK_MASK so that this first write
 		 * won't actually enable the interrupt, instead just
 		 * updating the ITR (it's bit 31 PF and VF)
 		 */
-		rxval |= BIT(31);
+		rxval = i40e_buildreg_itr(I40E_RX_ITR, itr) | BIT(31);
+
 		/* don't check _DOWN because interrupt isn't being enabled */
 		wr32(hw, INTREG(q_vector->reg_idx), rxval);
+
+		txval = i40e_buildreg_itr(I40E_TX_ITR, itr);
 	}
 
 enable_int:
-- 
2.14.3

  parent reply	other threads:[~2018-02-12 20:46 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-12 20:46 [net-next 00/12][pull request] 40GbE Intel Wired LAN Driver Updates 2018-02-12 Jeff Kirsher
2018-02-12 20:46 ` [net-next 01/12] i40e: fix typo in function description Jeff Kirsher
2018-02-12 20:46 ` [net-next 02/12] i40e/i40evf: Only track one ITR setting per ring instead of Tx/Rx Jeff Kirsher
2018-02-12 20:46 ` Jeff Kirsher [this message]
2018-02-12 20:46 ` [net-next 04/12] i40e: Add delay after EMP reset for firmware to recover Jeff Kirsher
2018-02-12 20:46 ` [net-next 05/12] i40e: Warn when setting link-down-on-close while in MFP Jeff Kirsher
2018-02-12 20:46 ` [net-next 06/12] i40e: use changed_flags to check I40E_FLAG_DISABLE_FW_LLDP Jeff Kirsher
2018-02-12 20:46 ` [net-next 07/12] i40e/i40evf: Clean-up of bits related to using q_vector->reg_idx Jeff Kirsher
2018-02-12 20:46 ` [net-next 08/12] i40e/i40evf: Don't bother setting the CLEARPBA bit Jeff Kirsher
2018-02-12 20:46 ` [net-next 09/12] i40e/i40evf: Use usec value instead of reg value for ITR defines Jeff Kirsher
2018-02-12 20:46 ` [net-next 10/12] i40evf: Correctly populate rxitr_idx and txitr_idx Jeff Kirsher
2018-02-12 20:46 ` [net-next 11/12] i40e/i40evf: Split container ITR into current_itr and target_itr Jeff Kirsher
2018-02-12 20:46 ` [net-next 12/12] i40e/i40evf: Add support for new mechanism of updating adaptive ITR Jeff Kirsher
2018-02-13  1:04 ` [net-next 00/12][pull request] 40GbE Intel Wired LAN Driver Updates 2018-02-12 David Miller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180212204649.24178-4-jeffrey.t.kirsher@intel.com \
    --to=jeffrey.t.kirsher@intel.com \
    --cc=alexander.h.duyck@intel.com \
    --cc=davem@davemloft.net \
    --cc=jogreene@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=nhorman@redhat.com \
    --cc=sassmann@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).