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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Mon, 12 Jul 2021 10:33:15 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     lingfuyi <lingfuyi@....com>
Cc:     davem@...emloft.net, kuba@...nel.org,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: 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" <hkallweit1@...il.com> wrote:
>>On 09.07.2021 11:06, lingfuyi wrote:
>>> 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.
>>
>>> 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 <lingfuyi@....com>
>>> ---
>>>  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.
>>
>>> +
>>> +	/* 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,
>>> 
> 
> 
> 
>  
>