Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210812155341.817031-1-kai.heng.feng@canonical.com> <875e7304-20a1-0bca-ee07-41b16f07152a@gmail.com>
In-Reply-To: <875e7304-20a1-0bca-ee07-41b16f07152a@gmail.com>
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
Date:   Fri, 13 Aug 2021 17:46:54 +0800
Message-ID: <CAAd53p41RWp6weA2uXmZvKiVajehYkuC6cmHDeLxtJU_gsxCFA@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] r8169: Implement dynamic ASPM mechanism
To:     Heiner Kallweit <hkallweit1@gmail.com>
Cc:     nic_swsd <nic_swsd@realtek.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        "open list:8169 10/100/1000 GIGABIT ETHERNET DRIVER" 
        <netdev@vger.kernel.org>, open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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?


>
> > 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.

>
> > +     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;
> >
>