[PATCH V1 net 0/2] fixes of interrupt moderation bugs

[PATCH V1 net 0/2] fixes of interrupt moderation bugs

[akiyano]

From: Arthur Kiyanovski <akiyano@amazon.com>

This patchset includes a couple of fixes of bugs in the implemenation of
interrupt moderation.

Arthur Kiyanovski (2):
  net: ena: fix issues in setting interrupt moderation params in ethtool
  net: ena: fix too long default tx interrupt moderation interval

 drivers/net/ethernet/amazon/ena/ena_com.h     |  2 +-
 drivers/net/ethernet/amazon/ena/ena_ethtool.c | 40 ++++++++-----------
 2 files changed, 17 insertions(+), 25 deletions(-)

-- 
2.17.1

[PATCH V1 net 1/2] net: ena: fix issues in setting interrupt moderation params in ethtool

[akiyano]

From: Arthur Kiyanovski <akiyano@amazon.com>

Issue 1:
--------
Reproduction steps:
1. sudo ethtool -C eth0 rx-usecs 128
2. sudo ethtool -C eth0 adaptive-rx on
3. sudo ethtool -C eth0 adaptive-rx off
4. ethtool -c eth0

expected output: rx-usecs 128
actual output: rx-usecs 0

Reason for issue:
In stage 3, ethtool userspace calls first the ena_get_coalesce() handler
to get the current value of all properties, and then the ena_set_coalesce()
handler. When ena_get_coalesce() is called the adaptive interrupt
moderation is still on. There is an if in the code that returns the
rx_coalesce_usecs only if the adaptive interrupt moderation is off.
And since it is still on, rx_coalesce_usecs is not set, meaning it
stays 0.

Solution to issue:
Remove this if static interrupt moderation intervals have nothing to do with
dynamic ones.

Issue 2:
--------
Reproduction steps:
1. sudo ethtool -C eth0 rx-usecs 128
2. sudo ethtool -C eth0 adaptive-rx on
3. sudo ethtool -C eth0 rx-usecs 128
4. ethtool -c eth0

expected output: rx-usecs 128
actual output: rx-usecs 0

Reason for issue:
In stage 3, when ena_set_coalesce() is called, the handler tests if
rx adaptive interrupt moderation is on, and if it is, it returns before
getting to the part in the function that sets the rx non-adaptive
interrupt moderation interval.

Solution to issue:
Remove the return from the function when rx adaptive interrupt moderation
is on.

Additional small fixes in this commit:
--------------------------------------
1. Remove 2 unnecessary comments.
2. Remove 4 unnecesary "{}" in single row if statements.
3. Reorder ena_set_coalesce() to make sense.
4. Change the names of ena_update_tx/rx_rings_intr_moderation()
   functions to ena_update_tx/rx_rings_nonadaptive_intr_moderation() for
   clarity.

Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_ethtool.c | 40 ++++++++-----------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index 16553d92fad2..b042c70504cd 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -306,19 +306,16 @@ static int ena_get_coalesce(struct net_device *net_dev,
 	struct ena_adapter *adapter = netdev_priv(net_dev);
 	struct ena_com_dev *ena_dev = adapter->ena_dev;
 
-	if (!ena_com_interrupt_moderation_supported(ena_dev)) {
-		/* the devie doesn't support interrupt moderation */
+	if (!ena_com_interrupt_moderation_supported(ena_dev))
 		return -EOPNOTSUPP;
-	}
 
 	coalesce->tx_coalesce_usecs =
 		ena_com_get_nonadaptive_moderation_interval_tx(ena_dev) *
 			ena_dev->intr_delay_resolution;
 
-	if (!ena_com_get_adaptive_moderation_enabled(ena_dev))
-		coalesce->rx_coalesce_usecs =
-			ena_com_get_nonadaptive_moderation_interval_rx(ena_dev)
-			* ena_dev->intr_delay_resolution;
+	coalesce->rx_coalesce_usecs =
+		ena_com_get_nonadaptive_moderation_interval_rx(ena_dev)
+		* ena_dev->intr_delay_resolution;
 
 	coalesce->use_adaptive_rx_coalesce =
 		ena_com_get_adaptive_moderation_enabled(ena_dev);
@@ -326,7 +323,7 @@ static int ena_get_coalesce(struct net_device *net_dev,
 	return 0;
 }
 
-static void ena_update_tx_rings_intr_moderation(struct ena_adapter *adapter)
+static void ena_update_tx_rings_nonadaptive_intr_moderation(struct ena_adapter *adapter)
 {
 	unsigned int val;
 	int i;
@@ -337,7 +334,7 @@ static void ena_update_tx_rings_intr_moderation(struct ena_adapter *adapter)
 		adapter->tx_ring[i].smoothed_interval = val;
 }
 
-static void ena_update_rx_rings_intr_moderation(struct ena_adapter *adapter)
+static void ena_update_rx_rings_nonadaptive_intr_moderation(struct ena_adapter *adapter)
 {
 	unsigned int val;
 	int i;
@@ -355,35 +352,30 @@ static int ena_set_coalesce(struct net_device *net_dev,
 	struct ena_com_dev *ena_dev = adapter->ena_dev;
 	int rc;
 
-	if (!ena_com_interrupt_moderation_supported(ena_dev)) {
-		/* the devie doesn't support interrupt moderation */
+	if (!ena_com_interrupt_moderation_supported(ena_dev))
 		return -EOPNOTSUPP;
-	}
 
 	rc = ena_com_update_nonadaptive_moderation_interval_tx(ena_dev,
 							       coalesce->tx_coalesce_usecs);
 	if (rc)
 		return rc;
 
-	ena_update_tx_rings_intr_moderation(adapter);
-
-	if (coalesce->use_adaptive_rx_coalesce) {
-		if (!ena_com_get_adaptive_moderation_enabled(ena_dev))
-			ena_com_enable_adaptive_moderation(ena_dev);
-		return 0;
-	}
+	ena_update_tx_rings_nonadaptive_intr_moderation(adapter);
 
 	rc = ena_com_update_nonadaptive_moderation_interval_rx(ena_dev,
 							       coalesce->rx_coalesce_usecs);
 	if (rc)
 		return rc;
 
-	ena_update_rx_rings_intr_moderation(adapter);
+	ena_update_rx_rings_nonadaptive_intr_moderation(adapter);
 
-	if (!coalesce->use_adaptive_rx_coalesce) {
-		if (ena_com_get_adaptive_moderation_enabled(ena_dev))
-			ena_com_disable_adaptive_moderation(ena_dev);
-	}
+	if ((coalesce->use_adaptive_rx_coalesce) &&
+	    (!ena_com_get_adaptive_moderation_enabled(ena_dev)))
+		ena_com_enable_adaptive_moderation(ena_dev);
+
+	if ((!coalesce->use_adaptive_rx_coalesce) &&
+	    (ena_com_get_adaptive_moderation_enabled(ena_dev)))
+		ena_com_disable_adaptive_moderation(ena_dev);
 
 	return 0;
 }
-- 
2.17.1

[PATCH V1 net 2/2] net: ena: fix too long default tx interrupt moderation interval

[akiyano]

From: Arthur Kiyanovski <akiyano@amazon.com>

Current default non-adaptive tx interrupt moderation interval is 196 us.
This commit sets it to 0, which is much more sensible as a default value.
It can be modified using ethtool -C.

Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_com.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h b/drivers/net/ethernet/amazon/ena/ena_com.h
index 7c941eba0bc9..164c45c04867 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.h
+++ b/drivers/net/ethernet/amazon/ena/ena_com.h
@@ -72,7 +72,7 @@
 /*****************************************************************************/
 /* ENA adaptive interrupt moderation settings */
 
-#define ENA_INTR_INITIAL_TX_INTERVAL_USECS		196
+#define ENA_INTR_INITIAL_TX_INTERVAL_USECS		0
 #define ENA_INTR_INITIAL_RX_INTERVAL_USECS		0
 #define ENA_DEFAULT_INTR_DELAY_RESOLUTION		1
 
-- 
2.17.1

Re: [PATCH V1 net 2/2] net: ena: fix too long default tx interrupt moderation interval

[David Miller]

From: <akiyano@amazon.com>
Date: Mon, 4 Nov 2019 13:58:48 +0200

> From: Arthur Kiyanovski <akiyano@amazon.com>
> 
> Current default non-adaptive tx interrupt moderation interval is 196 us.
> This commit sets it to 0, which is much more sensible as a default value.
> It can be modified using ethtool -C.
> 
> Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>

I do not agree that turning TX interrupt moderation off completely is
a more sensible default value.

Maybe a much smaller value, but turning off the coalescing delay
completely is a bit much.

RE: [PATCH V1 net 2/2] net: ena: fix too long default tx interrupt moderation interval

[Kiyanovski, Arthur]

> -----Original Message-----
> From: David Miller <davem@davemloft.net>
> Sent: Monday, November 4, 2019 9:19 PM
> To: Kiyanovski, Arthur <akiyano@amazon.com>
> Cc: netdev@vger.kernel.org; Woodhouse, David <dwmw@amazon.co.uk>;
> Machulsky, Zorik <zorik@amazon.com>; Matushevsky, Alexander
> <matua@amazon.com>; Bshara, Saeed <saeedb@amazon.com>; Wilson,
> Matt <msw@amazon.com>; Liguori, Anthony <aliguori@amazon.com>;
> Bshara, Nafea <nafea@amazon.com>; Tzalik, Guy <gtzalik@amazon.com>;
> Belgazal, Netanel <netanel@amazon.com>; Saidi, Ali
> <alisaidi@amazon.com>; Herrenschmidt, Benjamin <benh@amazon.com>;
> Dagan, Noam <ndagan@amazon.com>; Agroskin, Shay
> <shayagr@amazon.com>
> Subject: Re: [PATCH V1 net 2/2] net: ena: fix too long default tx interrupt
> moderation interval
> 
> From: <akiyano@amazon.com>
> Date: Mon, 4 Nov 2019 13:58:48 +0200
> 
> > From: Arthur Kiyanovski <akiyano@amazon.com>
> >
> > Current default non-adaptive tx interrupt moderation interval is 196 us.
> > This commit sets it to 0, which is much more sensible as a default value.
> > It can be modified using ethtool -C.
> >
> > Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
> 
> I do not agree that turning TX interrupt moderation off completely is a more
> sensible default value.
> 
> Maybe a much smaller value, but turning off the coalescing delay completely
> is a bit much.

David,
Up until now, the ENA device did not support interrupt moderation, so effectively the default tx interrupt moderation interval was 0.
You are probably right that 0 is not an optimal value.
However until we research and find such an optimal value, in order to avoid a degradation in default performance, we want the default value in the new driver to be (effectively) the same as in the old driver.

Re: [PATCH V1 net 2/2] net: ena: fix too long default tx interrupt moderation interval

[Machulsky, Zorik]

     > -----Original Message-----
    > From: David Miller <davem@davemloft.net>
    > Sent: Monday, November 4, 2019 9:19 PM
    > To: Kiyanovski, Arthur <akiyano@amazon.com>
    > Cc: netdev@vger.kernel.org; Woodhouse, David <dwmw@amazon.co.uk>;
    > Machulsky, Zorik <zorik@amazon.com>; Matushevsky, Alexander
    > <matua@amazon.com>; Bshara, Saeed <saeedb@amazon.com>; Wilson,
    > Matt <msw@amazon.com>; Liguori, Anthony <aliguori@amazon.com>;
    > Bshara, Nafea <nafea@amazon.com>; Tzalik, Guy <gtzalik@amazon.com>;
    > Belgazal, Netanel <netanel@amazon.com>; Saidi, Ali
    > <alisaidi@amazon.com>; Herrenschmidt, Benjamin <benh@amazon.com>;
    > Dagan, Noam <ndagan@amazon.com>; Agroskin, Shay
    > <shayagr@amazon.com>
    > Subject: Re: [PATCH V1 net 2/2] net: ena: fix too long default tx interrupt
    > moderation interval
    > 
    > From: <akiyano@amazon.com>
    > Date: Mon, 4 Nov 2019 13:58:48 +0200
    > 
    > > From: Arthur Kiyanovski <akiyano@amazon.com>
    > >
    > > Current default non-adaptive tx interrupt moderation interval is 196 us.
    > > This commit sets it to 0, which is much more sensible as a default value.
    > > It can be modified using ethtool -C.
    > >
    > > Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
    > 
    > I do not agree that turning TX interrupt moderation off completely is a more
    > sensible default value.
    > 
    > Maybe a much smaller value, but turning off the coalescing delay completely
    > is a bit much.
    
    David,
    Up until now, the ENA device did not support interrupt moderation, so effectively the default tx interrupt moderation interval was 0.
    You are probably right that 0 is not an optimal value.
    However until we research and find such an optimal value, in order to avoid a degradation in default performance, we want the default value in the new driver to be (effectively) the same as in the old driver.
    
 David,
Just wanted to re-iterate what Arthur has mentioned. We clearly see BW and CPU utilization improvement with introduction of DIM on the Rx side and non-adaptive moderation on the Tx side in our driver. 
We'd like to deliver this to our customers ASAP. We are usually very cautious with introduction of the new features, therefore we'd like to keep interrupt moderation disabled by default for now. We'd advise our customers awaiting for it to enable it using ethtool. 
We are going to enable moderation by default after we accumulate more mileage with it and fine tune it further. That's the reason behind having Tx interval =0 for now (Rx moderation is disabled by default as well). Hope it makes sense.    
  	       

Re: [PATCH V1 net 2/2] net: ena: fix too long default tx interrupt moderation interval

[Machulsky, Zorik]

    
         > -----Original Message-----
        > From: David Miller <davem@davemloft.net>
        > Sent: Monday, November 4, 2019 9:19 PM
        > To: Kiyanovski, Arthur <akiyano@amazon.com>
        > Cc: netdev@vger.kernel.org; Woodhouse, David <dwmw@amazon.co.uk>;
        > Machulsky, Zorik <zorik@amazon.com>; Matushevsky, Alexander
        > <matua@amazon.com>; Bshara, Saeed <saeedb@amazon.com>; Wilson,
        > Matt <msw@amazon.com>; Liguori, Anthony <aliguori@amazon.com>;
        > Bshara, Nafea <nafea@amazon.com>; Tzalik, Guy <gtzalik@amazon.com>;
        > Belgazal, Netanel <netanel@amazon.com>; Saidi, Ali
        > <alisaidi@amazon.com>; Herrenschmidt, Benjamin <benh@amazon.com>;
        > Dagan, Noam <ndagan@amazon.com>; Agroskin, Shay
        > <shayagr@amazon.com>
        > Subject: Re: [PATCH V1 net 2/2] net: ena: fix too long default tx interrupt
        > moderation interval
        > 
        > From: <akiyano@amazon.com>
        > Date: Mon, 4 Nov 2019 13:58:48 +0200
        > 
        > > From: Arthur Kiyanovski <akiyano@amazon.com>
        > >
        > > Current default non-adaptive tx interrupt moderation interval is 196 us.
        > > This commit sets it to 0, which is much more sensible as a default value.
        > > It can be modified using ethtool -C.
        > >
        > > Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
        > 
        > I do not agree that turning TX interrupt moderation off completely is a more
        > sensible default value.
        > 
        > Maybe a much smaller value, but turning off the coalescing delay completely
        > is a bit much.
        
        David,
        Up until now, the ENA device did not support interrupt moderation, so effectively the default tx interrupt moderation interval was 0.
        You are probably right that 0 is not an optimal value.
        However until we research and find such an optimal value, in order to avoid a degradation in default performance, we want the default value in the new driver to be (effectively) the same as in the old driver.
        
     David,
    Just wanted to re-iterate what Arthur has mentioned. We clearly see BW and CPU utilization improvement with introduction of DIM on the Rx side and non-adaptive moderation on the Tx side in our driver. 
    We'd like to deliver this to our customers ASAP. We are usually very cautious with introduction of the new features, therefore we'd like to keep interrupt moderation disabled by default for now. We'd advise our customers awaiting for it to enable it using ethtool. 
    We are going to enable moderation by default after we accumulate more mileage with it and fine tune it further. That's the reason behind having Tx interval =0 for now (Rx moderation is disabled by default as well). Hope it makes sense.    

David,
I'm bumping this up to bring it to your attention.
Thanks.