[PATCH net-next v2 0/6] Add adaptive interrupt coalescing

[PATCH net-next v2 0/6] Add adaptive interrupt coalescing

[Claudiu Manoil]

Apart from some related cleanup patches, this set
introduces in a straightforward way the support needed
to enable and configure interrupt coalescing for ENETC.

Patch 5 introduces the support needed for configuring the
interrupt coalescing parameters and for switching between
moderated (int. coalescing) and per-packet interrupt modes.
When interrupt coalescing is enabled the Rx/Tx time
thresholds are configurable, packet thresholds are fixed.
To make this work reliably, patch 5 uses the traffic
pause procedure introduced in patch 2.

Patch 6 adds DIM (Dynamic Interrupt Moderation) to implement
adaptive coalescing based on time thresholds, for the Rx 'channel'.
On the Tx side a default optimal value is used instead, optimized for
TCP traffic over 1G and 2.5G links.  This default 'optimal' value can
be overridden anytime via 'ethtool -C tx-usecs'.

netperf -t TCP_MAERTS measurements show a significant CPU load
reduction correlated w/ reduced interrupt rates. For the
measurement results refer to the comments in patch 6.

v2: Replaced Tx DIM with predefined optimal value, giving
better results. This was also suggested by Jakub (cc).
Switched order of patches 4 and 5, for better grouping.

Claudiu Manoil (6):
  enetc: Refine buffer descriptor ring sizes
  enetc: Factor out the traffic start/stop procedures
  enetc: Fix interrupt coalescing register naming
  enetc: Drop redundant ____cacheline_aligned_in_smp
  enetc: Add interrupt coalescing support
  enetc: Add adaptive interrupt coalescing

 drivers/net/ethernet/freescale/enetc/Kconfig  |   2 +
 drivers/net/ethernet/freescale/enetc/enetc.c  | 156 ++++++++++++++----
 drivers/net/ethernet/freescale/enetc/enetc.h  |  37 ++++-
 .../ethernet/freescale/enetc/enetc_ethtool.c  |  91 +++++++++-
 .../net/ethernet/freescale/enetc/enetc_hw.h   |  23 ++-
 5 files changed, 265 insertions(+), 44 deletions(-)

-- 
2.17.1

[PATCH net-next v2 1/6] enetc: Refine buffer descriptor ring sizes

[Claudiu Manoil]

It's time to differentiate between Rx and Tx ring sizes.
Not only Tx rings are processed differently than Rx rings,
but their default number also differs - i.e. up to 8 Tx rings
per device (8 traffic classes) vs. 2 Rx rings (one per CPU).
So let's set Tx rings sizes to half the size of the Rx rings
for now, to be conservative.
The default ring sizes were decreased as well (to the next
lower power of 2), to reduce the memory footprint, buffering
etc., since the measurements I've made so far show that the
rings are very unlikely to get full.
This change also anticipates the introduction of the
dynamic interrupt moderation (dim) algorithm which operates
on maximum packet thresholds of 256 packets for Rx and 128
packets for Tx.

Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
v2: none

 drivers/net/ethernet/freescale/enetc/enetc.c | 4 ++--
 drivers/net/ethernet/freescale/enetc/enetc.h | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 3f32b85ba2cf..d91e52618681 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1064,8 +1064,8 @@ void enetc_init_si_rings_params(struct enetc_ndev_priv *priv)
 	struct enetc_si *si = priv->si;
 	int cpus = num_online_cpus();
 
-	priv->tx_bd_count = ENETC_BDR_DEFAULT_SIZE;
-	priv->rx_bd_count = ENETC_BDR_DEFAULT_SIZE;
+	priv->tx_bd_count = ENETC_TX_RING_DEFAULT_SIZE;
+	priv->rx_bd_count = ENETC_RX_RING_DEFAULT_SIZE;
 
 	/* Enable all available TX rings in order to configure as many
 	 * priorities as possible, when needed.
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index b705464f6882..0dd8ee179753 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -44,8 +44,9 @@ struct enetc_ring_stats {
 	unsigned int rx_alloc_errs;
 };
 
-#define ENETC_BDR_DEFAULT_SIZE	1024
-#define ENETC_DEFAULT_TX_WORK	256
+#define ENETC_RX_RING_DEFAULT_SIZE	512
+#define ENETC_TX_RING_DEFAULT_SIZE	256
+#define ENETC_DEFAULT_TX_WORK		(ENETC_TX_RING_DEFAULT_SIZE / 2)
 
 struct enetc_bdr {
 	struct device *dev; /* for DMA mapping */
-- 
2.17.1

[PATCH net-next v2 2/6] enetc: Factor out the traffic start/stop procedures

[Claudiu Manoil]

A reliable traffic pause (and reconfiguration) procedure
is needed to be able to safely make h/w configuration
changes during run-time, like changing the mode in which the
interrupts are operating (i.e. with or without coalescing),
as opposed to making on-the-fly register updates that
may be subject to h/w or s/w concurrency issues.
To this end, the code responsible of the run-time device
configurations that basically starts resp. stops the traffic
flow through the device has been extracted from the
the enetc_open/_close procedures, to the separate standalone
enetc_start/_stop procedures. Traffic stop should be as
graceful as possible, it lets the executing napi threads to
to finish while the interrupts stay disabled.  But since
the napi thread will try to re-enable interrupts by clearing
the device's unmask register, the enable_irq/ disable_irq
API has been used to avoid this potential concurrency issue
and make the traffic pause procedure more reliable.

Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
v2: none

 drivers/net/ethernet/freescale/enetc/enetc.c | 74 +++++++++++++-------
 1 file changed, 49 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index d91e52618681..51a1c97aedac 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1264,6 +1264,7 @@ static int enetc_setup_irqs(struct enetc_ndev_priv *priv)
 			dev_err(priv->dev, "request_irq() failed!\n");
 			goto irq_err;
 		}
+		disable_irq(irq);
 
 		v->tbier_base = hw->reg + ENETC_BDR(TX, 0, ENETC_TBIER);
 		v->rbier = hw->reg + ENETC_BDR(RX, i, ENETC_RBIER);
@@ -1306,7 +1307,7 @@ static void enetc_free_irqs(struct enetc_ndev_priv *priv)
 	}
 }
 
-static void enetc_enable_interrupts(struct enetc_ndev_priv *priv)
+static void enetc_setup_interrupts(struct enetc_ndev_priv *priv)
 {
 	int i;
 
@@ -1322,7 +1323,7 @@ static void enetc_enable_interrupts(struct enetc_ndev_priv *priv)
 	}
 }
 
-static void enetc_disable_interrupts(struct enetc_ndev_priv *priv)
+static void enetc_clear_interrupts(struct enetc_ndev_priv *priv)
 {
 	int i;
 
@@ -1369,10 +1370,33 @@ static int enetc_phy_connect(struct net_device *ndev)
 	return 0;
 }
 
+static void enetc_start(struct net_device *ndev)
+{
+	struct enetc_ndev_priv *priv = netdev_priv(ndev);
+	int i;
+
+	enetc_setup_interrupts(priv);
+
+	for (i = 0; i < priv->bdr_int_num; i++) {
+		int irq = pci_irq_vector(priv->si->pdev,
+					 ENETC_BDR_INT_BASE_IDX + i);
+
+		napi_enable(&priv->int_vector[i]->napi);
+		enable_irq(irq);
+	}
+
+	if (ndev->phydev)
+		phy_start(ndev->phydev);
+	else
+		netif_carrier_on(ndev);
+
+	netif_tx_start_all_queues(ndev);
+}
+
 int enetc_open(struct net_device *ndev)
 {
 	struct enetc_ndev_priv *priv = netdev_priv(ndev);
-	int i, err;
+	int err;
 
 	err = enetc_setup_irqs(priv);
 	if (err)
@@ -1390,8 +1414,6 @@ int enetc_open(struct net_device *ndev)
 	if (err)
 		goto err_alloc_rx;
 
-	enetc_setup_bdrs(priv);
-
 	err = netif_set_real_num_tx_queues(ndev, priv->num_tx_rings);
 	if (err)
 		goto err_set_queues;
@@ -1400,17 +1422,8 @@ int enetc_open(struct net_device *ndev)
 	if (err)
 		goto err_set_queues;
 
-	for (i = 0; i < priv->bdr_int_num; i++)
-		napi_enable(&priv->int_vector[i]->napi);
-
-	enetc_enable_interrupts(priv);
-
-	if (ndev->phydev)
-		phy_start(ndev->phydev);
-	else
-		netif_carrier_on(ndev);
-
-	netif_tx_start_all_queues(ndev);
+	enetc_setup_bdrs(priv);
+	enetc_start(ndev);
 
 	return 0;
 
@@ -1427,28 +1440,39 @@ int enetc_open(struct net_device *ndev)
 	return err;
 }
 
-int enetc_close(struct net_device *ndev)
+static void enetc_stop(struct net_device *ndev)
 {
 	struct enetc_ndev_priv *priv = netdev_priv(ndev);
 	int i;
 
 	netif_tx_stop_all_queues(ndev);
 
-	if (ndev->phydev) {
-		phy_stop(ndev->phydev);
-		phy_disconnect(ndev->phydev);
-	} else {
-		netif_carrier_off(ndev);
-	}
-
 	for (i = 0; i < priv->bdr_int_num; i++) {
+		int irq = pci_irq_vector(priv->si->pdev,
+					 ENETC_BDR_INT_BASE_IDX + i);
+
+		disable_irq(irq);
 		napi_synchronize(&priv->int_vector[i]->napi);
 		napi_disable(&priv->int_vector[i]->napi);
 	}
 
-	enetc_disable_interrupts(priv);
+	if (ndev->phydev)
+		phy_stop(ndev->phydev);
+	else
+		netif_carrier_off(ndev);
+
+	enetc_clear_interrupts(priv);
+}
+
+int enetc_close(struct net_device *ndev)
+{
+	struct enetc_ndev_priv *priv = netdev_priv(ndev);
+
+	enetc_stop(ndev);
 	enetc_clear_bdrs(priv);
 
+	if (ndev->phydev)
+		phy_disconnect(ndev->phydev);
 	enetc_free_rxtx_rings(priv);
 	enetc_free_rx_resources(priv);
 	enetc_free_tx_resources(priv);
-- 
2.17.1

[PATCH net-next v2 3/6] enetc: Fix interrupt coalescing register naming

[Claudiu Manoil]

Interrupt coalescing registers naming in the current revision
of the Ref Man (RM) is ICR, deprecating the ICIR name used
in earlier (draft) versions of the RM.

Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
v2: none

 drivers/net/ethernet/freescale/enetc/enetc.c         | 4 ++--
 drivers/net/ethernet/freescale/enetc/enetc_ethtool.c | 2 +-
 drivers/net/ethernet/freescale/enetc/enetc_hw.h      | 8 ++++----
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 51a1c97aedac..be594c7af538 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1140,7 +1140,7 @@ static void enetc_setup_txbdr(struct enetc_hw *hw, struct enetc_bdr *tx_ring)
 	tx_ring->next_to_clean = enetc_txbdr_rd(hw, idx, ENETC_TBCIR);
 
 	/* enable Tx ints by setting pkt thr to 1 */
-	enetc_txbdr_wr(hw, idx, ENETC_TBICIR0, ENETC_TBICIR0_ICEN | 0x1);
+	enetc_txbdr_wr(hw, idx, ENETC_TBICR0, ENETC_TBICR0_ICEN | 0x1);
 
 	tbmr = ENETC_TBMR_EN;
 	if (tx_ring->ndev->features & NETIF_F_HW_VLAN_CTAG_TX)
@@ -1174,7 +1174,7 @@ static void enetc_setup_rxbdr(struct enetc_hw *hw, struct enetc_bdr *rx_ring)
 	enetc_rxbdr_wr(hw, idx, ENETC_RBPIR, 0);
 
 	/* enable Rx ints by setting pkt thr to 1 */
-	enetc_rxbdr_wr(hw, idx, ENETC_RBICIR0, ENETC_RBICIR0_ICEN | 0x1);
+	enetc_rxbdr_wr(hw, idx, ENETC_RBICR0, ENETC_RBICR0_ICEN | 0x1);
 
 	rbmr = ENETC_RBMR_EN;
 
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index 34bd1f3fb415..8aeaa3de0012 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -19,7 +19,7 @@ static const u32 enetc_txbdr_regs[] = {
 
 static const u32 enetc_rxbdr_regs[] = {
 	ENETC_RBMR, ENETC_RBSR, ENETC_RBBSR, ENETC_RBCIR, ENETC_RBBAR0,
-	ENETC_RBBAR1, ENETC_RBPIR, ENETC_RBLENR, ENETC_RBICIR0, ENETC_RBIER
+	ENETC_RBBAR1, ENETC_RBPIR, ENETC_RBLENR, ENETC_RBICR0, ENETC_RBIER
 };
 
 static const u32 enetc_port_regs[] = {
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index fc357bc56835..05bb4c525897 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -121,8 +121,8 @@ enum enetc_bdr_type {TX, RX};
 #define ENETC_RBIER	0xa0
 #define ENETC_RBIER_RXTIE	BIT(0)
 #define ENETC_RBIDR	0xa4
-#define ENETC_RBICIR0	0xa8
-#define ENETC_RBICIR0_ICEN	BIT(31)
+#define ENETC_RBICR0	0xa8
+#define ENETC_RBICR0_ICEN	BIT(31)
 
 /* TX BDR reg offsets */
 #define ENETC_TBMR	0
@@ -141,8 +141,8 @@ enum enetc_bdr_type {TX, RX};
 #define ENETC_TBIER	0xa0
 #define ENETC_TBIER_TXTIE	BIT(0)
 #define ENETC_TBIDR	0xa4
-#define ENETC_TBICIR0	0xa8
-#define ENETC_TBICIR0_ICEN	BIT(31)
+#define ENETC_TBICR0	0xa8
+#define ENETC_TBICR0_ICEN	BIT(31)
 
 #define ENETC_RTBLENR_LEN(n)	((n) & ~0x7)
 
-- 
2.17.1

[PATCH net-next v2 4/6] enetc: Drop redundant ____cacheline_aligned_in_smp

[Claudiu Manoil]

'struct enetc_bdr' is already '____cacheline_aligned_in_smp'.

Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
v2: none

 drivers/net/ethernet/freescale/enetc/enetc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index 0dd8ee179753..81e9072e10d4 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -195,7 +195,7 @@ struct enetc_int_vector {
 	struct napi_struct napi;
 	char name[ENETC_INT_NAME_MAX];
 
-	struct enetc_bdr rx_ring ____cacheline_aligned_in_smp;
+	struct enetc_bdr rx_ring;
 	struct enetc_bdr tx_ring[];
 };
 
-- 
2.17.1

[PATCH net-next v2 5/6] enetc: Add interrupt coalescing support

[Claudiu Manoil]

Enable programming of the interrupt coalescing registers
and allow manual configuration of the coalescing time
thresholds via ethtool.  Packet thresholds have been fixed
to predetermined values as there's no point in making them
run-time configurable, also anticipating the dynamic interrupt
moderation (DIM) algorithm which uses fixed packet thresholds
as well.  If the interface is up when the operation mode of
traffic interrupt events is changed by the user (i.e. switching
from default per-packet interrupts to coalesced interrupts),
the traffic needs to be paused in the process.
This patch also prepares the ground for introducing DIM on Rx.

Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
v2: removed tx_ictt from the "fast path", as Tx updates
remain static (dropped Tx DIM idea)

 drivers/net/ethernet/freescale/enetc/enetc.c  | 32 ++++++--
 drivers/net/ethernet/freescale/enetc/enetc.h  | 18 +++++
 .../ethernet/freescale/enetc/enetc_ethtool.c  | 75 ++++++++++++++++++-
 .../net/ethernet/freescale/enetc/enetc_hw.h   | 19 ++++-
 4 files changed, 134 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index be594c7af538..f4593c044043 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -265,6 +265,7 @@ static irqreturn_t enetc_msix(int irq, void *data)
 
 	/* disable interrupts */
 	enetc_wr_reg(v->rbier, 0);
+	enetc_wr_reg(v->ricr1, v->rx_ictt);
 
 	for_each_set_bit(i, &v->tx_rings_map, ENETC_MAX_NUM_TXQS)
 		enetc_wr_reg(v->tbier_base + ENETC_BDR_OFF(i), 0);
@@ -1268,6 +1269,7 @@ static int enetc_setup_irqs(struct enetc_ndev_priv *priv)
 
 		v->tbier_base = hw->reg + ENETC_BDR(TX, 0, ENETC_TBIER);
 		v->rbier = hw->reg + ENETC_BDR(RX, i, ENETC_RBIER);
+		v->ricr1 = hw->reg + ENETC_BDR(RX, i, ENETC_RBICR1);
 
 		enetc_wr(hw, ENETC_SIMSIRRV(i), entry);
 
@@ -1309,17 +1311,35 @@ static void enetc_free_irqs(struct enetc_ndev_priv *priv)
 
 static void enetc_setup_interrupts(struct enetc_ndev_priv *priv)
 {
+	struct enetc_hw *hw = &priv->si->hw;
+	u32 icpt, ictt;
 	int i;
 
 	/* enable Tx & Rx event indication */
+	if (priv->ic_mode & ENETC_IC_RX_MANUAL) {
+		icpt = ENETC_RBICR0_SET_ICPT(ENETC_RXIC_PKTTHR);
+		/* init to non-0 minimum, will be adjusted later */
+		ictt = 0x1;
+	} else {
+		icpt = 0x1; /* enable Rx ints by setting pkt thr to 1 */
+		ictt = 0;
+	}
+
 	for (i = 0; i < priv->num_rx_rings; i++) {
-		enetc_rxbdr_wr(&priv->si->hw, i,
-			       ENETC_RBIER, ENETC_RBIER_RXTIE);
+		enetc_rxbdr_wr(hw, i, ENETC_RBICR1, ictt);
+		enetc_rxbdr_wr(hw, i, ENETC_RBICR0, ENETC_RBICR0_ICEN | icpt);
+		enetc_rxbdr_wr(hw, i, ENETC_RBIER, ENETC_RBIER_RXTIE);
 	}
 
+	if (priv->ic_mode & ENETC_IC_TX_MANUAL)
+		icpt = ENETC_TBICR0_SET_ICPT(ENETC_TXIC_PKTTHR);
+	else
+		icpt = 0x1; /* enable Tx ints by setting pkt thr to 1 */
+
 	for (i = 0; i < priv->num_tx_rings; i++) {
-		enetc_txbdr_wr(&priv->si->hw, i,
-			       ENETC_TBIER, ENETC_TBIER_TXTIE);
+		enetc_txbdr_wr(hw, i, ENETC_TBICR1, priv->tx_ictt);
+		enetc_txbdr_wr(hw, i, ENETC_TBICR0, ENETC_TBICR0_ICEN | icpt);
+		enetc_txbdr_wr(hw, i, ENETC_TBIER, ENETC_TBIER_TXTIE);
 	}
 }
 
@@ -1370,7 +1390,7 @@ static int enetc_phy_connect(struct net_device *ndev)
 	return 0;
 }
 
-static void enetc_start(struct net_device *ndev)
+void enetc_start(struct net_device *ndev)
 {
 	struct enetc_ndev_priv *priv = netdev_priv(ndev);
 	int i;
@@ -1440,7 +1460,7 @@ int enetc_open(struct net_device *ndev)
 	return err;
 }
 
-static void enetc_stop(struct net_device *ndev)
+void enetc_stop(struct net_device *ndev)
 {
 	struct enetc_ndev_priv *priv = netdev_priv(ndev);
 	int i;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index 81e9072e10d4..4e3af7f07892 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -190,8 +190,10 @@ static inline bool enetc_si_is_pf(struct enetc_si *si)
 struct enetc_int_vector {
 	void __iomem *rbier;
 	void __iomem *tbier_base;
+	void __iomem *ricr1;
 	unsigned long tx_rings_map;
 	int count_tx_rings;
+	u32 rx_ictt;
 	struct napi_struct napi;
 	char name[ENETC_INT_NAME_MAX];
 
@@ -221,6 +223,18 @@ enum enetc_active_offloads {
 	ENETC_F_QCI		= BIT(3),
 };
 
+/* interrupt coalescing modes */
+enum enetc_ic_mode {
+	/* one interrupt per frame */
+	ENETC_IC_NONE = 0,
+	/* activated when int coalescing time is set to a non-0 value */
+	ENETC_IC_RX_MANUAL = BIT(0),
+	ENETC_IC_TX_MANUAL = BIT(1),
+};
+
+#define ENETC_RXIC_PKTTHR	min_t(u32, 256, ENETC_RX_RING_DEFAULT_SIZE / 2)
+#define ENETC_TXIC_PKTTHR	min_t(u32, 128, ENETC_TX_RING_DEFAULT_SIZE / 2)
+
 struct enetc_ndev_priv {
 	struct net_device *ndev;
 	struct device *dev; /* dma-mapping device */
@@ -245,6 +259,8 @@ struct enetc_ndev_priv {
 
 	struct device_node *phy_node;
 	phy_interface_t if_mode;
+	int ic_mode;
+	u32 tx_ictt;
 };
 
 /* Messaging */
@@ -274,6 +290,8 @@ void enetc_free_si_resources(struct enetc_ndev_priv *priv);
 
 int enetc_open(struct net_device *ndev);
 int enetc_close(struct net_device *ndev);
+void enetc_start(struct net_device *ndev);
+void enetc_stop(struct net_device *ndev);
 netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev);
 struct net_device_stats *enetc_get_stats(struct net_device *ndev);
 int enetc_set_features(struct net_device *ndev,
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index 8aeaa3de0012..3bf4fc0bc64a 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -14,12 +14,14 @@ static const u32 enetc_si_regs[] = {
 
 static const u32 enetc_txbdr_regs[] = {
 	ENETC_TBMR, ENETC_TBSR, ENETC_TBBAR0, ENETC_TBBAR1,
-	ENETC_TBPIR, ENETC_TBCIR, ENETC_TBLENR, ENETC_TBIER
+	ENETC_TBPIR, ENETC_TBCIR, ENETC_TBLENR, ENETC_TBIER, ENETC_TBICR0,
+	ENETC_TBICR1
 };
 
 static const u32 enetc_rxbdr_regs[] = {
 	ENETC_RBMR, ENETC_RBSR, ENETC_RBBSR, ENETC_RBCIR, ENETC_RBBAR0,
-	ENETC_RBBAR1, ENETC_RBPIR, ENETC_RBLENR, ENETC_RBICR0, ENETC_RBIER
+	ENETC_RBBAR1, ENETC_RBPIR, ENETC_RBLENR, ENETC_RBIER, ENETC_RBICR0,
+	ENETC_RBICR1
 };
 
 static const u32 enetc_port_regs[] = {
@@ -561,6 +563,67 @@ static void enetc_get_ringparam(struct net_device *ndev,
 	}
 }
 
+static int enetc_get_coalesce(struct net_device *ndev,
+			      struct ethtool_coalesce *ic)
+{
+	struct enetc_ndev_priv *priv = netdev_priv(ndev);
+	struct enetc_int_vector *v = priv->int_vector[0];
+
+	ic->tx_coalesce_usecs = enetc_cycles_to_usecs(priv->tx_ictt);
+	ic->rx_coalesce_usecs = enetc_cycles_to_usecs(v->rx_ictt);
+
+	ic->tx_max_coalesced_frames = ENETC_TXIC_PKTTHR;
+	ic->rx_max_coalesced_frames = ENETC_RXIC_PKTTHR;
+
+	return 0;
+}
+
+static int enetc_set_coalesce(struct net_device *ndev,
+			      struct ethtool_coalesce *ic)
+{
+	struct enetc_ndev_priv *priv = netdev_priv(ndev);
+	u32 rx_ictt, tx_ictt;
+	int i, ic_mode;
+	bool changed;
+
+	tx_ictt = enetc_usecs_to_cycles(ic->tx_coalesce_usecs);
+	rx_ictt = enetc_usecs_to_cycles(ic->rx_coalesce_usecs);
+
+	if (ic->rx_max_coalesced_frames != ENETC_RXIC_PKTTHR)
+		netif_warn(priv, hw, ndev, "rx-frames fixed to %d\n",
+			   ENETC_RXIC_PKTTHR);
+
+	if (ic->tx_max_coalesced_frames != ENETC_TXIC_PKTTHR)
+		netif_warn(priv, hw, ndev, "tx-frames fixed to %d\n",
+			   ENETC_TXIC_PKTTHR);
+
+	ic_mode = ENETC_IC_NONE;
+	ic_mode |= tx_ictt ? ENETC_IC_TX_MANUAL : 0;
+	ic_mode |= rx_ictt ? ENETC_IC_RX_MANUAL : 0;
+
+	/* commit the settings */
+	changed = (ic_mode != priv->ic_mode);
+
+	priv->ic_mode = ic_mode;
+	priv->tx_ictt = tx_ictt;
+
+	for (i = 0; i < priv->bdr_int_num; i++) {
+		struct enetc_int_vector *v = priv->int_vector[i];
+
+		v->rx_ictt = rx_ictt;
+	}
+
+	if (netif_running(ndev) && changed) {
+		/* reconfigure the operation mode of h/w interrupts,
+		 * traffic needs to be paused in the process
+		 */
+		enetc_stop(ndev);
+		enetc_start(ndev);
+	}
+
+	return 0;
+}
+
 static int enetc_get_ts_info(struct net_device *ndev,
 			     struct ethtool_ts_info *info)
 {
@@ -617,6 +680,8 @@ static int enetc_set_wol(struct net_device *dev,
 }
 
 static const struct ethtool_ops enetc_pf_ethtool_ops = {
+	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
+				     ETHTOOL_COALESCE_MAX_FRAMES,
 	.get_regs_len = enetc_get_reglen,
 	.get_regs = enetc_get_regs,
 	.get_sset_count = enetc_get_sset_count,
@@ -629,6 +694,8 @@ static const struct ethtool_ops enetc_pf_ethtool_ops = {
 	.get_rxfh = enetc_get_rxfh,
 	.set_rxfh = enetc_set_rxfh,
 	.get_ringparam = enetc_get_ringparam,
+	.get_coalesce = enetc_get_coalesce,
+	.set_coalesce = enetc_set_coalesce,
 	.get_link_ksettings = phy_ethtool_get_link_ksettings,
 	.set_link_ksettings = phy_ethtool_set_link_ksettings,
 	.get_link = ethtool_op_get_link,
@@ -638,6 +705,8 @@ static const struct ethtool_ops enetc_pf_ethtool_ops = {
 };
 
 static const struct ethtool_ops enetc_vf_ethtool_ops = {
+	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
+				     ETHTOOL_COALESCE_MAX_FRAMES,
 	.get_regs_len = enetc_get_reglen,
 	.get_regs = enetc_get_regs,
 	.get_sset_count = enetc_get_sset_count,
@@ -649,6 +718,8 @@ static const struct ethtool_ops enetc_vf_ethtool_ops = {
 	.get_rxfh = enetc_get_rxfh,
 	.set_rxfh = enetc_set_rxfh,
 	.get_ringparam = enetc_get_ringparam,
+	.get_coalesce = enetc_get_coalesce,
+	.set_coalesce = enetc_set_coalesce,
 	.get_link = ethtool_op_get_link,
 	.get_ts_info = enetc_get_ts_info,
 };
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index 05bb4c525897..95f3c4d8f602 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -122,7 +122,10 @@ enum enetc_bdr_type {TX, RX};
 #define ENETC_RBIER_RXTIE	BIT(0)
 #define ENETC_RBIDR	0xa4
 #define ENETC_RBICR0	0xa8
-#define ENETC_RBICR0_ICEN	BIT(31)
+#define ENETC_RBICR0_ICEN		BIT(31)
+#define ENETC_RBICR0_ICPT_MASK		0x1ff
+#define ENETC_RBICR0_SET_ICPT(n)	((n) & ENETC_RBICR0_ICPT_MASK)
+#define ENETC_RBICR1	0xac
 
 /* TX BDR reg offsets */
 #define ENETC_TBMR	0
@@ -142,7 +145,10 @@ enum enetc_bdr_type {TX, RX};
 #define ENETC_TBIER_TXTIE	BIT(0)
 #define ENETC_TBIDR	0xa4
 #define ENETC_TBICR0	0xa8
-#define ENETC_TBICR0_ICEN	BIT(31)
+#define ENETC_TBICR0_ICEN		BIT(31)
+#define ENETC_TBICR0_ICPT_MASK		0xf
+#define ENETC_TBICR0_SET_ICPT(n) ((ilog2(n) + 1) & ENETC_TBICR0_ICPT_MASK)
+#define ENETC_TBICR1	0xac
 
 #define ENETC_RTBLENR_LEN(n)	((n) & ~0x7)
 
@@ -784,6 +790,15 @@ struct enetc_cbd {
 };
 
 #define ENETC_CLK  400000000ULL
+static inline u32 enetc_cycles_to_usecs(u32 cycles)
+{
+	return (u32)div_u64(cycles * 1000000ULL, ENETC_CLK);
+}
+
+static inline u32 enetc_usecs_to_cycles(u32 usecs)
+{
+	return (u32)div_u64(usecs * ENETC_CLK, 1000000ULL);
+}
 
 /* port time gating control register */
 #define ENETC_QBV_PTGCR_OFFSET		0x11a00
-- 
2.17.1

[PATCH net-next v2 6/6] enetc: Add adaptive interrupt coalescing

[Claudiu Manoil]

Use the generic dynamic interrupt moderation (DIM)
framework to implement adaptive interrupt coalescing
on Rx.  With the per-packet interrupt scheme, a high
interrupt rate has been noted for moderate traffic flows
leading to high CPU utilization.  The DIM scheme
implemented by the current patch addresses this issue
improving CPU utilization while using minimal coalescing
time thresholds in order to preserve a good latency.
On the Tx side use an optimal time threshold value by
default.  This value has been optimized for Tx TCP
streams at a rate of around 85kpps on a 1G link,
at which rate half of the Tx ring size (128) gets filled
in 1500 usecs.  Scaling this down to 2.5G links yields
the current value of 600 usecs, which is conservative
and gives good enough results for 1G links too (see
next).

Below are some measurement results for before and after
this patch (and related dependencies) basically, for a
2 ARM Cortex-A72 @1.3Ghz CPUs system, 32 KB L1 data cache,
using 60secs log netperf TCP stream tests @ 1Gbit link
(maximum throughput):

1) 1 Rx TCP flow, both Rx and Tx processed by the same NAPI
thread on the same CPU:
	CPU utilization		int rate (ints/sec)
Before:	50%-60% (over 50%)		92k
After:  13%-22%				3.5k-12k
Comment:  Major CPU utilization improvement for a single flow
	  Rx TCP flow (i.e. netperf -t TCP_MAERTS) on a single
	  CPU. Usually settles under 16% for longer tests.

2) 4 Rx TCP flows + 4 Tx TCP flows (+ pings to check the latency):
	Total CPU utilization	Total int rate (ints/sec)
Before:	~80% (spikes to 90%)		~100k
After:   60% (more steady)		  ~4k
Comment:  Important improvement for this load test, while the
	  ping test outcome does not show any notable
	  difference compared to before.

Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
v2: Replaced Tx DIM with static optimal value.

 drivers/net/ethernet/freescale/enetc/Kconfig  |  2 +
 drivers/net/ethernet/freescale/enetc/enetc.c  | 50 ++++++++++++++++++-
 drivers/net/ethernet/freescale/enetc/enetc.h  | 12 ++++-
 .../ethernet/freescale/enetc/enetc_ethtool.c  | 24 +++++++--
 4 files changed, 80 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/Kconfig b/drivers/net/ethernet/freescale/enetc/Kconfig
index 2b43848e1363..37b804f8bd76 100644
--- a/drivers/net/ethernet/freescale/enetc/Kconfig
+++ b/drivers/net/ethernet/freescale/enetc/Kconfig
@@ -4,6 +4,7 @@ config FSL_ENETC
 	depends on PCI && PCI_MSI
 	select FSL_ENETC_MDIO
 	select PHYLIB
+	select DIMLIB
 	help
 	  This driver supports NXP ENETC gigabit ethernet controller PCIe
 	  physical function (PF) devices, managing ENETC Ports at a privileged
@@ -15,6 +16,7 @@ config FSL_ENETC_VF
 	tristate "ENETC VF driver"
 	depends on PCI && PCI_MSI
 	select PHYLIB
+	select DIMLIB
 	help
 	  This driver supports NXP ENETC gigabit ethernet controller PCIe
 	  virtual function (VF) devices enabled by the ENETC PF driver.
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index f4593c044043..c408bb068f51 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -279,6 +279,34 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget);
 static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
 			       struct napi_struct *napi, int work_limit);
 
+static void enetc_rx_dim_work(struct work_struct *w)
+{
+	struct dim *dim = container_of(w, struct dim, work);
+	struct dim_cq_moder moder =
+		net_dim_get_rx_moderation(dim->mode, dim->profile_ix);
+	struct enetc_int_vector	*v =
+		container_of(dim, struct enetc_int_vector, rx_dim);
+
+	v->rx_ictt = enetc_usecs_to_cycles(moder.usec);
+	dim->state = DIM_START_MEASURE;
+}
+
+static void enetc_rx_net_dim(struct enetc_int_vector *v)
+{
+	struct dim_sample dim_sample;
+
+	v->comp_cnt++;
+
+	if (!v->rx_napi_work)
+		return;
+
+	dim_update_sample(v->comp_cnt,
+			  v->rx_ring.stats.packets,
+			  v->rx_ring.stats.bytes,
+			  &dim_sample);
+	net_dim(&v->rx_dim, dim_sample);
+}
+
 static int enetc_poll(struct napi_struct *napi, int budget)
 {
 	struct enetc_int_vector
@@ -294,12 +322,19 @@ static int enetc_poll(struct napi_struct *napi, int budget)
 	work_done = enetc_clean_rx_ring(&v->rx_ring, napi, budget);
 	if (work_done == budget)
 		complete = false;
+	if (work_done)
+		v->rx_napi_work = true;
 
 	if (!complete)
 		return budget;
 
 	napi_complete_done(napi, work_done);
 
+	if (likely(v->rx_dim_en))
+		enetc_rx_net_dim(v);
+
+	v->rx_napi_work = false;
+
 	/* enable interrupts */
 	enetc_wr_reg(v->rbier, ENETC_RBIER_RXTIE);
 
@@ -1075,6 +1110,8 @@ void enetc_init_si_rings_params(struct enetc_ndev_priv *priv)
 	priv->num_rx_rings = min_t(int, cpus, si->num_rx_rings);
 	priv->num_tx_rings = si->num_tx_rings;
 	priv->bdr_int_num = cpus;
+	priv->ic_mode = ENETC_IC_RX_ADAPTIVE | ENETC_IC_TX_OPTIMAL;
+	priv->tx_ictt = ENETC_TXIC_TIMETHR;
 
 	/* SI specific */
 	si->cbd_ring.bd_count = ENETC_CBDR_DEFAULT_SIZE;
@@ -1316,7 +1353,8 @@ static void enetc_setup_interrupts(struct enetc_ndev_priv *priv)
 	int i;
 
 	/* enable Tx & Rx event indication */
-	if (priv->ic_mode & ENETC_IC_RX_MANUAL) {
+	if (priv->ic_mode &
+	    (ENETC_IC_RX_MANUAL | ENETC_IC_RX_ADAPTIVE)) {
 		icpt = ENETC_RBICR0_SET_ICPT(ENETC_RXIC_PKTTHR);
 		/* init to non-0 minimum, will be adjusted later */
 		ictt = 0x1;
@@ -1331,7 +1369,7 @@ static void enetc_setup_interrupts(struct enetc_ndev_priv *priv)
 		enetc_rxbdr_wr(hw, i, ENETC_RBIER, ENETC_RBIER_RXTIE);
 	}
 
-	if (priv->ic_mode & ENETC_IC_TX_MANUAL)
+	if (priv->tx_ictt)
 		icpt = ENETC_TBICR0_SET_ICPT(ENETC_TXIC_PKTTHR);
 	else
 		icpt = 0x1; /* enable Tx ints by setting pkt thr to 1 */
@@ -1786,6 +1824,12 @@ int enetc_alloc_msix(struct enetc_ndev_priv *priv)
 
 		priv->int_vector[i] = v;
 
+		/* init defaults for adaptive IC */
+		if (priv->ic_mode & ENETC_IC_RX_ADAPTIVE) {
+			v->rx_ictt = 0x1;
+			v->rx_dim_en = true;
+		}
+		INIT_WORK(&v->rx_dim.work, enetc_rx_dim_work);
 		netif_napi_add(priv->ndev, &v->napi, enetc_poll,
 			       NAPI_POLL_WEIGHT);
 		v->count_tx_rings = v_tx_rings;
@@ -1821,6 +1865,7 @@ int enetc_alloc_msix(struct enetc_ndev_priv *priv)
 fail:
 	while (i--) {
 		netif_napi_del(&priv->int_vector[i]->napi);
+		cancel_work_sync(&priv->int_vector[i]->rx_dim.work);
 		kfree(priv->int_vector[i]);
 	}
 
@@ -1837,6 +1882,7 @@ void enetc_free_msix(struct enetc_ndev_priv *priv)
 		struct enetc_int_vector *v = priv->int_vector[i];
 
 		netif_napi_del(&v->napi);
+		cancel_work_sync(&v->rx_dim.work);
 	}
 
 	for (i = 0; i < priv->num_rx_rings; i++)
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index 4e3af7f07892..486e1ed4fe64 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -10,6 +10,7 @@
 #include <linux/ethtool.h>
 #include <linux/if_vlan.h>
 #include <linux/phy.h>
+#include <linux/dim.h>
 
 #include "enetc_hw.h"
 
@@ -194,12 +195,15 @@ struct enetc_int_vector {
 	unsigned long tx_rings_map;
 	int count_tx_rings;
 	u32 rx_ictt;
-	struct napi_struct napi;
+	u16 comp_cnt;
+	bool rx_dim_en, rx_napi_work;
+	struct napi_struct napi ____cacheline_aligned_in_smp;
+	struct dim rx_dim ____cacheline_aligned_in_smp;
 	char name[ENETC_INT_NAME_MAX];
 
 	struct enetc_bdr rx_ring;
 	struct enetc_bdr tx_ring[];
-};
+} ____cacheline_aligned_in_smp;
 
 struct enetc_cls_rule {
 	struct ethtool_rx_flow_spec fs;
@@ -230,10 +234,14 @@ enum enetc_ic_mode {
 	/* activated when int coalescing time is set to a non-0 value */
 	ENETC_IC_RX_MANUAL = BIT(0),
 	ENETC_IC_TX_MANUAL = BIT(1),
+	/* use dynamic interrupt moderation */
+	ENETC_IC_RX_ADAPTIVE = BIT(2),
+	ENETC_IC_TX_OPTIMAL = BIT(3),
 };
 
 #define ENETC_RXIC_PKTTHR	min_t(u32, 256, ENETC_RX_RING_DEFAULT_SIZE / 2)
 #define ENETC_TXIC_PKTTHR	min_t(u32, 128, ENETC_TX_RING_DEFAULT_SIZE / 2)
+#define ENETC_TXIC_TIMETHR	enetc_usecs_to_cycles(600)
 
 struct enetc_ndev_priv {
 	struct net_device *ndev;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index 3bf4fc0bc64a..bef6ef4852cf 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -575,6 +575,8 @@ static int enetc_get_coalesce(struct net_device *ndev,
 	ic->tx_max_coalesced_frames = ENETC_TXIC_PKTTHR;
 	ic->rx_max_coalesced_frames = ENETC_RXIC_PKTTHR;
 
+	ic->use_adaptive_rx_coalesce = priv->ic_mode & ENETC_IC_RX_ADAPTIVE;
+
 	return 0;
 }
 
@@ -598,11 +600,22 @@ static int enetc_set_coalesce(struct net_device *ndev,
 			   ENETC_TXIC_PKTTHR);
 
 	ic_mode = ENETC_IC_NONE;
-	ic_mode |= tx_ictt ? ENETC_IC_TX_MANUAL : 0;
-	ic_mode |= rx_ictt ? ENETC_IC_RX_MANUAL : 0;
+	if (ic->use_adaptive_rx_coalesce) {
+		ic_mode |= ENETC_IC_RX_ADAPTIVE;
+		rx_ictt = 0x1;
+	} else {
+		ic_mode |= rx_ictt ? ENETC_IC_RX_MANUAL : 0;
+	}
+
+	if (tx_ictt == ENETC_TXIC_TIMETHR)
+		ic_mode |= ENETC_IC_TX_OPTIMAL;
+	else
+		ic_mode |= tx_ictt ? ENETC_IC_TX_MANUAL : 0;
 
 	/* commit the settings */
 	changed = (ic_mode != priv->ic_mode);
+	if (ic_mode & ENETC_IC_TX_MANUAL && priv->tx_ictt != tx_ictt)
+		changed = true;
 
 	priv->ic_mode = ic_mode;
 	priv->tx_ictt = tx_ictt;
@@ -611,6 +624,7 @@ static int enetc_set_coalesce(struct net_device *ndev,
 		struct enetc_int_vector *v = priv->int_vector[i];
 
 		v->rx_ictt = rx_ictt;
+		v->rx_dim_en = !!(ic_mode & ENETC_IC_RX_ADAPTIVE);
 	}
 
 	if (netif_running(ndev) && changed) {
@@ -681,7 +695,8 @@ static int enetc_set_wol(struct net_device *dev,
 
 static const struct ethtool_ops enetc_pf_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
-				     ETHTOOL_COALESCE_MAX_FRAMES,
+				     ETHTOOL_COALESCE_MAX_FRAMES |
+				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX,
 	.get_regs_len = enetc_get_reglen,
 	.get_regs = enetc_get_regs,
 	.get_sset_count = enetc_get_sset_count,
@@ -706,7 +721,8 @@ static const struct ethtool_ops enetc_pf_ethtool_ops = {
 
 static const struct ethtool_ops enetc_vf_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
-				     ETHTOOL_COALESCE_MAX_FRAMES,
+				     ETHTOOL_COALESCE_MAX_FRAMES |
+				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX,
 	.get_regs_len = enetc_get_reglen,
 	.get_regs = enetc_get_regs,
 	.get_sset_count = enetc_get_sset_count,
-- 
2.17.1

Re: [PATCH net-next v2 6/6] enetc: Add adaptive interrupt coalescing

[Jakub Kicinski]

On Fri, 17 Jul 2020 18:37:04 +0300 Claudiu Manoil wrote:
> +	if (tx_ictt == ENETC_TXIC_TIMETHR)
> +		ic_mode |= ENETC_IC_TX_OPTIMAL;

Doesn't seem you ever read/check the ENETC_IC_TX_OPTIMAL flag?

Re: [PATCH net-next v2 5/6] enetc: Add interrupt coalescing support

[Jakub Kicinski]

On Fri, 17 Jul 2020 18:37:03 +0300 Claudiu Manoil wrote:
> +	if (ic->rx_max_coalesced_frames != ENETC_RXIC_PKTTHR)
> +		netif_warn(priv, hw, ndev, "rx-frames fixed to %d\n",
> +			   ENETC_RXIC_PKTTHR);
> +
> +	if (ic->tx_max_coalesced_frames != ENETC_TXIC_PKTTHR)
> +		netif_warn(priv, hw, ndev, "tx-frames fixed to %d\n",
> +			   ENETC_TXIC_PKTTHR);

On second thought - why not return an error here? Since only one value
is supported seems like the right way to communicate to the users that
they can't change this.

> +	if (netif_running(ndev) && changed) {
> +		/* reconfigure the operation mode of h/w interrupts,
> +		 * traffic needs to be paused in the process
> +		 */
> +		enetc_stop(ndev);
> +		enetc_start(ndev);

Is start going to print an error when it fails? Kinda scary if this
could turn into a silent failure.

Re: [PATCH net-next v2 5/6] enetc: Add interrupt coalescing support

[Claudiu Manoil]

On 17.07.2020 22:32, Jakub Kicinski wrote:
> On Fri, 17 Jul 2020 18:37:03 +0300 Claudiu Manoil wrote:
>> +	if (ic->rx_max_coalesced_frames != ENETC_RXIC_PKTTHR)
>> +		netif_warn(priv, hw, ndev, "rx-frames fixed to %d\n",
>> +			   ENETC_RXIC_PKTTHR);
>> +
>> +	if (ic->tx_max_coalesced_frames != ENETC_TXIC_PKTTHR)
>> +		netif_warn(priv, hw, ndev, "tx-frames fixed to %d\n",
>> +			   ENETC_TXIC_PKTTHR);
> 
> On second thought - why not return an error here? Since only one value
> is supported seems like the right way to communicate to the users that
> they can't change this.
> 

Do you mean to return -EOPNOTSUPP without any error message instead?
If so, I think it's less punishing not to return an error code and 
invalidate the rest of the ethtool -C parameters that might have been
correct if the user forgets that rx/tx-frames cannot be changed.
There's also this flag:
	.supported_coalesce_params = .. |
				     ETHTOOL_COALESCE_MAX_FRAMES |
				     ..,
needed for printing the preconfigured values for the rx/tx packet 
thresholds, and this flag basically says that the 'rx/tx-frames'
parameters are supported (just that they cannot be changed... :) ).
But I don't have a strong bias for this, if you prefer the return
-EOPNOTSUPP option I'll make this change, just let me know if I got
it right.

>> +	if (netif_running(ndev) && changed) {
>> +		/* reconfigure the operation mode of h/w interrupts,
>> +		 * traffic needs to be paused in the process
>> +		 */
>> +		enetc_stop(ndev);
>> +		enetc_start(ndev);
> 
> Is start going to print an error when it fails? Kinda scary if this
> could turn into a silent failure.
> 

enetc_start() returns void, just like enetc_stop().  If you look it up
it only sets some run-time configurable registers and calls some basic
run-time commands that should no fail like napi_enable(), enable_irq(), 
phy_start(), all void returning functions. This function doesn't 
allocate resources or anything of that sort, and should be kept that 
way. And indeed, it should not fail. But regarding error codes there's
nothing I can do for this function, as nothing inside it generates any 
error code.

Re: [PATCH net-next v2 6/6] enetc: Add adaptive interrupt coalescing

[Claudiu Manoil]

On 17.07.2020 22:30, Jakub Kicinski wrote:
> On Fri, 17 Jul 2020 18:37:04 +0300 Claudiu Manoil wrote:
>> +	if (tx_ictt == ENETC_TXIC_TIMETHR)
>> +		ic_mode |= ENETC_IC_TX_OPTIMAL;
> 
> Doesn't seem you ever read/check the ENETC_IC_TX_OPTIMAL flag?
> 

It's used implicitly though ;), as it signals a state change when
the user changes the default value of the tx time threshold,
triggering the device recofiguration with the new value. True that
the said reconfiguration could be also performed in the 'MANUAL' state.
I added the extra state called 'OPTIMAL' to make the code more easier to 
follow actually. I mean, it's easy to follow that the tx coalescing 
state starts in the "OPTIMAL" mode, w/ the preconfigured "optimal" 
value. Then if the user changes the value, doing some manual tuning of 
tx-usecs, it moves into the 'MANUAL' mode, returning to the 'OPTIMAL' 
mode if the user goes back to the optimal value.
This handling could also be done in the 'MANUAL' mode alone, so if you 
want me to make this change pls let me know.

Thanks,
Claudiu

Re: [PATCH net-next v2 5/6] enetc: Add interrupt coalescing support

[Jakub Kicinski]

On Sat, 18 Jul 2020 20:20:10 +0300 Claudiu Manoil wrote:
> On 17.07.2020 22:32, Jakub Kicinski wrote:
> > On Fri, 17 Jul 2020 18:37:03 +0300 Claudiu Manoil wrote:  
> >> +	if (ic->rx_max_coalesced_frames != ENETC_RXIC_PKTTHR)
> >> +		netif_warn(priv, hw, ndev, "rx-frames fixed to %d\n",
> >> +			   ENETC_RXIC_PKTTHR);
> >> +
> >> +	if (ic->tx_max_coalesced_frames != ENETC_TXIC_PKTTHR)
> >> +		netif_warn(priv, hw, ndev, "tx-frames fixed to %d\n",
> >> +			   ENETC_TXIC_PKTTHR);  
> > 
> > On second thought - why not return an error here? Since only one value
> > is supported seems like the right way to communicate to the users that
> > they can't change this.
> 
> Do you mean to return -EOPNOTSUPP without any error message instead?

Yes.

> If so, I think it's less punishing not to return an error code and 
> invalidate the rest of the ethtool -C parameters that might have been
> correct if the user forgets that rx/tx-frames cannot be changed.

IMHO if configuring manually - user can correct the params on the CLI.
If there's an orchestration system trying to configure those - it's 
better to return an error and alert the administrator than confuse 
the orchestration by allowing a write which is then not reflected 
on read.

> There's also this flag:
> 	.supported_coalesce_params = .. |
> 				     ETHTOOL_COALESCE_MAX_FRAMES |
> 				     ..,
> needed for printing the preconfigured values for the rx/tx packet 
> thresholds, and this flag basically says that the 'rx/tx-frames'
> parameters are supported (just that they cannot be changed... :) ).

Right, unfortunately core can't do the checking here, but I think 
the driver should.

> But I don't have a strong bias for this, if you prefer the return
> -EOPNOTSUPP option I'll make this change, just let me know if I got
> it right.
> 
> >> +	if (netif_running(ndev) && changed) {
> >> +		/* reconfigure the operation mode of h/w interrupts,
> >> +		 * traffic needs to be paused in the process
> >> +		 */
> >> +		enetc_stop(ndev);
> >> +		enetc_start(ndev);  
> > 
> > Is start going to print an error when it fails? Kinda scary if this
> > could turn into a silent failure.
> 
> enetc_start() returns void, just like enetc_stop().  If you look it up
> it only sets some run-time configurable registers and calls some basic
> run-time commands that should no fail like napi_enable(), enable_irq(), 
> phy_start(), all void returning functions. This function doesn't 
> allocate resources or anything of that sort, and should be kept that 
> way. And indeed, it should not fail. But regarding error codes there's
> nothing I can do for this function, as nothing inside it generates any 
> error code.

Got it!

RE: [PATCH net-next v2 5/6] enetc: Add interrupt coalescing support

[Claudiu Manoil]

>-----Original Message-----
>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Monday, July 20, 2020 7:59 PM
[...]
>Subject: Re: [PATCH net-next v2 5/6] enetc: Add interrupt coalescing support
>
>On Sat, 18 Jul 2020 20:20:10 +0300 Claudiu Manoil wrote:
>> On 17.07.2020 22:32, Jakub Kicinski wrote:
>> > On Fri, 17 Jul 2020 18:37:03 +0300 Claudiu Manoil wrote:
>> >> +	if (ic->rx_max_coalesced_frames != ENETC_RXIC_PKTTHR)
>> >> +		netif_warn(priv, hw, ndev, "rx-frames fixed to %d\n",
>> >> +			   ENETC_RXIC_PKTTHR);
>> >> +
>> >> +	if (ic->tx_max_coalesced_frames != ENETC_TXIC_PKTTHR)
>> >> +		netif_warn(priv, hw, ndev, "tx-frames fixed to %d\n",
>> >> +			   ENETC_TXIC_PKTTHR);
>> >
>> > On second thought - why not return an error here? Since only one value
>> > is supported seems like the right way to communicate to the users that
>> > they can't change this.
>>
>> Do you mean to return -EOPNOTSUPP without any error message instead?
>
>Yes.
>
>> If so, I think it's less punishing not to return an error code and
>> invalidate the rest of the ethtool -C parameters that might have been
>> correct if the user forgets that rx/tx-frames cannot be changed.
>
>IMHO if configuring manually - user can correct the params on the CLI.
>If there's an orchestration system trying to configure those - it's
>better to return an error and alert the administrator than confuse
>the orchestration by allowing a write which is then not reflected
>on read.
>

Good point, ok.  Updated accordingly in v3.
Thanks.

RE: [PATCH net-next v2 6/6] enetc: Add adaptive interrupt coalescing

[Claudiu Manoil]

>-----Original Message-----
>From: Claudiu Manoil <claudiu.manoil@gmail.com>
>Sent: Saturday, July 18, 2020 8:20 PM
[...]
>Subject: Re: [PATCH net-next v2 6/6] enetc: Add adaptive interrupt coalescing
>
>
>On 17.07.2020 22:30, Jakub Kicinski wrote:
>> On Fri, 17 Jul 2020 18:37:04 +0300 Claudiu Manoil wrote:
>>> +	if (tx_ictt == ENETC_TXIC_TIMETHR)
>>> +		ic_mode |= ENETC_IC_TX_OPTIMAL;
>>
>> Doesn't seem you ever read/check the ENETC_IC_TX_OPTIMAL flag?
>>
>
>It's used implicitly though ;), as it signals a state change when
>the user changes the default value of the tx time threshold,
>triggering the device recofiguration with the new value. True that
>the said reconfiguration could be also performed in the 'MANUAL' state.
>I added the extra state called 'OPTIMAL' to make the code more easier to
>follow actually. I mean, it's easy to follow that the tx coalescing
>state starts in the "OPTIMAL" mode, w/ the preconfigured "optimal"
>value. Then if the user changes the value, doing some manual tuning of
>tx-usecs, it moves into the 'MANUAL' mode, returning to the 'OPTIMAL'
>mode if the user goes back to the optimal value.
>This handling could also be done in the 'MANUAL' mode alone, so if you
>want me to make this change pls let me know.
>

Removed the 'OPTIMAL' flag, the code looks simpler (fewer lines), and
I'm ok with it.  Updated in v3.