Re: [PATCH] r8169: Disable eee when device init

On 12.07.2021 04:37, lingfuyi wrote:

I found through git bisect that the delay was introduced after the commid id: b6c7fa401625d949e5e370f32e74f22c3bbaed51. At the same time, I tested many subsequent versions of this commit id, which basically floated between 8000 and 15000. If this option is disabled, it will remain stable at about 27000.

I tested 8169 and 8168, the performance is the same, I think this option should be set as a power management option, rather than the default activation

What you call performance here refers to latency.iperf3 gives me ca. 950Mbps
also with EEE enabled.If EEE-induced latency is a problem for you, then just
disable EEE.

At 2021-07-11 06:15:44, "Heiner Kallweit" [off-list ref] wrote:

quoted

On 09.07.2021 11:06, lingfuyi wrote:

quoted

The kernel default option enables the EEE function of the network card
When this option is turned on, both TCP_RR and UDP_RR of netperf will
be reduced.
The test data is as follows ：
                EEE enable      EEE disable
TCP_RR          15333           25895
UDP_RR          15888           26908

AFAIK it's normal that EEE adds some latency. Was the latency you
measured greater than what you expected?

With which chip versions did you test? r8169 supports ~ 50 chip versions
and they may behave quite different regarding EEE.
Did you also test with the r8168 vendor driver? Is it the same there?

Other users (especially of mobile devices) may weight the energy saving
higher than latency. Therefore there may be different opinions on what
should be the default.

quoted

Now modify the kernel code to disable the EEE function by default to
improve system performance. If you need to open it, you can use the
following command：
ethtool --set-eee DEVICENAME eee on/off

Signed-off-by: lingfuyi <redacted>
---
 drivers/net/ethernet/realtek/r8169_main.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index f744557c33a3..507005fac98e 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c

@@ -1959,7 +1959,7 @@ static const struct ethtool_ops rtl8169_ethtool_ops = {
 	.set_pauseparam		= rtl8169_set_pauseparam,
 };
 
-static void rtl_enable_eee(struct rtl8169_private *tp)
+static void rtl_init_eee(struct rtl8169_private *tp)
 {
 	struct phy_device *phydev = tp->phydev;
 	int adv;

@@ -2209,7 +2209,7 @@ static void rtl8169_init_phy(struct rtl8169_private *tp)
 	phy_speed_up(tp->phydev);
 
 	if (rtl_supports_eee(tp))
-		rtl_enable_eee(tp);
+		rtl_init_eee(tp);
 
 	genphy_soft_reset(tp->phydev);
 }

@@ -5260,7 +5260,9 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	tp->dev = dev;
 	tp->pci_dev = pdev;
 	tp->supports_gmii = ent->driver_data == RTL_CFG_NO_GBIT ? 0 : 1;
-	tp->eee_adv = -1;

If you remove this then not much functionality is left in
rtl_enable_eee() and it could be inlined.

quoted

+
+	/* Disable eee when device init */
+	tp->eee_adv = 0;
 	tp->ocp_base = OCP_STD_PHY_BASE;
 
 	dev->tstats = devm_netdev_alloc_pcpu_stats(&pdev->dev,