Re: [PATCH v2 1/2] r8169: Implement dynamic ASPM mechanism

Heiner Kallweit
2021-08-14T11:34:18Z

On 13.08.2021 11:46, Kai-Heng Feng wrote:
> j
>
> On Fri, Aug 13, 2021 at 3:39 AM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> On 12.08.2021 17:53, Kai-Heng Feng wrote:
>>> r8169 NICs on some platforms have abysmal speed when ASPM is enabled.
>>> Same issue can be observed with older vendor drivers.
>>>
>>> The issue is however solved by the latest vendor driver. There's a new
>>> mechanism, which disables r8169's internal ASPM when the NIC traffic has
>>> more than 10 packets, and vice versa.
>>>
>>> Realtek confirmed that all their PCIe LAN NICs, r8106, r8168 and r8125
>>> use dynamic ASPM under Windows. So implement the same mechanism here to
>>> resolve the issue.
>>>
>> Realtek using something in their Windows drivers isn't really a proof of
>> quality.
>
> I agree. So it'll be great if Realtek can work with us here.
>
>> Still my concerns haven't been addressed. If ASPM is enabled and
>> there's a congestion in the chip it may take up to a second until ASPM
>> gets disabled. In this second traffic very likely is heavily affected.
>> Who takes care in case of problem reports?
>
> I think we'll know that once the patch is merged in downstream kernel.
>
>>
>> This is a massive change for basically all chip versions. And experience
>> shows that in case of problem reports Realtek never cares, even though
>> they are listed as maintainers. All I see is that they copy more and more
>> code from r8169 into their own drivers. This seems to indicate that they
>> consider quality of their own drivers as not sufficient.
>
> I wonder why they don't want to put their efforts to r8169...
> Obviously they are doing a great job for rtw88 and r8152.
>
>>
>> Still my proposal: Apply this downstream, and if there are no complaints
>> after a few months it may be considered for mainline.
>
> Yes that's my plan. But I'd still like it to be reviewed before
> putting it to the downstream kernel.
>
>>
>> Last but not least the formal issues:
>> - no cover letter
>
> Will write it up once it's tested dowstream.
>
>> - no net/net-next annotation
>
> Does it mean put "net/net-next" in the subject line?
>

https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html#how-do-i-indicate-which-tree-net-vs-net-next-my-patch-should-be-in

>
>>
>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>> ---
>>> v2:
>>> - Use delayed_work instead of timer_list to avoid interrupt context
>>> - Use mutex to serialize packet counter read/write
>>> - Wording change
>>>
>>> drivers/net/ethernet/realtek/r8169_main.c | 45 +++++++++++++++++++++++
>>> 1 file changed, 45 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
>>> index c7af5bc3b8af..7ab2e841dc69 100644
>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>>> @@ -624,6 +624,11 @@ struct rtl8169_private {
>>>
>>> unsigned supports_gmii:1;
>>> unsigned aspm_manageable:1;
>>> + unsigned aspm_enabled:1;
>>> + struct delayed_work aspm_toggle;
>>> + struct mutex aspm_mutex;
>>> + u32 aspm_packet_count;
>>> +
>>> dma_addr_t counters_phys_addr;
>>> struct rtl8169_counters *counters;
>>> struct rtl8169_tc_offsets tc_offset;
>>> @@ -2671,6 +2676,8 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
>>> RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
>>> }
>>>
>>> + tp->aspm_enabled = enable;
>>> +
>>> udelay(10);
>>> }
>>>
>>> @@ -4408,6 +4415,9 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp,
>>>
>>> dirty_tx = tp->dirty_tx;
>>>
>>> + mutex_lock(&tp->aspm_mutex);
>>
>> We are in soft irq context here, therefore you shouldn't sleep.
>
> I thought napi_poll is not using softirq, apparent I was wrong. Will
> correct it too.
>
I saw an automated mail from a test bot to you complaining about this.

>>
>>> + tp->aspm_packet_count += tp->cur_tx - dirty_tx;
>>> + mutex_unlock(&tp->aspm_mutex);
>>> while (READ_ONCE(tp->cur_tx) != dirty_tx) {
>>> unsigned int entry = dirty_tx % NUM_TX_DESC;
>>> u32 status;
>>> @@ -4552,6 +4562,10 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget
>>> rtl8169_mark_to_asic(desc);
>>> }
>>>
>>> + mutex_lock(&tp->aspm_mutex);
>>> + tp->aspm_packet_count += count;
>>> + mutex_unlock(&tp->aspm_mutex);
>>> +
>>> return count;
>>> }
>>>
>>> @@ -4659,8 +4673,33 @@ static int r8169_phy_connect(struct rtl8169_private *tp)
>>> return 0;
>>> }
>>>
>>> +#define ASPM_PACKET_THRESHOLD 10
>>> +#define ASPM_TOGGLE_INTERVAL 1000
>>> +
>>> +static void rtl8169_aspm_toggle(struct work_struct *work)
>>> +{
>>> + struct rtl8169_private *tp = container_of(work, struct rtl8169_private,
>>> + aspm_toggle.work);
>>> + bool enable;
>>> +
>>> + mutex_lock(&tp->aspm_mutex);
>>> + enable = tp->aspm_packet_count <= ASPM_PACKET_THRESHOLD;
>>> + tp->aspm_packet_count = 0;
>>> + mutex_unlock(&tp->aspm_mutex);
>>> +
>>> + if (tp->aspm_enabled != enable) {
>>> + rtl_unlock_config_regs(tp);
>>> + rtl_hw_aspm_clkreq_enable(tp, enable);
>>> + rtl_lock_config_regs(tp);
>>> + }
>>> +
>>> + schedule_delayed_work(&tp->aspm_toggle, ASPM_TOGGLE_INTERVAL);
>>> +}
>>> +
>>> static void rtl8169_down(struct rtl8169_private *tp)
>>> {
>>> + cancel_delayed_work_sync(&tp->aspm_toggle);
>>> +
>>> /* Clear all task flags */
>>> bitmap_zero(tp->wk.flags, RTL_FLAG_MAX);
>>>
>>> @@ -4687,6 +4726,8 @@ static void rtl8169_up(struct rtl8169_private *tp)
>>> rtl_reset_work(tp);
>>>
>>> phy_start(tp->phydev);
>>> +
>>> + schedule_delayed_work(&tp->aspm_toggle, ASPM_TOGGLE_INTERVAL);
>>
>> In the first version you used msecs_to_jiffies(ASPM_TIMER_INTERVAL).
>> Now you use 1000 jiffies what is a major difference.
>
> msecs_to_jiffies() was omitted. Will correct it.
>
> Kai-Heng
>
>>
>>> }
>>>
>>> static int rtl8169_close(struct net_device *dev)
>>> @@ -5347,6 +5388,10 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>
>>> INIT_WORK(&tp->wk.work, rtl_task);
>>>
>>> + INIT_DELAYED_WORK(&tp->aspm_toggle, rtl8169_aspm_toggle);
>>> +
>>> + mutex_init(&tp->aspm_mutex);
>>> +
>>> rtl_init_mac_address(tp);
>>>
>>> dev->ethtool_ops = &rtl8169_ethtool_ops;
>>>
>>