Re: [PATCH v3 1/1] r8169: Coalesce RTL8411b PHY power-down recovery programming instructions to reduce spinlock stalls

From: Marco Elver
Date: Mon Oct 30 2023 - 10:06:29 EST

On Mon, 30 Oct 2023 at 14:53, Mirsad Todorovac
<mirsad.todorovac@xxxxxxxxxxxx> wrote:
>
> On 10/30/23 14:30, Mirsad Todorovac wrote:
> >
> >
> > On 10/30/23 14:17, Heiner Kallweit wrote:
> >> On 29.10.2023 05:56, Mirsad Todorovac wrote:
> >>>
> >>>
> >>> On 10/28/23 21:21, Heiner Kallweit wrote:
> >>>> On 28.10.2023 13:05, Mirsad Goran Todorovac wrote:
> >>>>> On RTL8411b the RX unit gets confused if the PHY is powered-down.
> >>>>> This was reported in [0] and confirmed by Realtek. Realtek provided
> >>>>> a sequence to fix the RX unit after PHY wakeup.
> >>>>>
> >>>>> A series of about 130 r8168_mac_ocp_write() calls is performed to
> >>>>> program the RTL registers for recovery.
> >>>>>
> >>>>> r8168_mac_ocp_write() expands to this code:
> >>>>>
> >>>>> static void __r8168_mac_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
> >>>>> {
> >>>>> if (rtl_ocp_reg_failure(reg))
> >>>>> return;
> >>>>>
> >>>>> RTL_W32(tp, OCPDR, OCPAR_FLAG | (reg << 15) | data);
> >>>>> }
> >>>>>
> >>>>> static void r8168_mac_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
> >>>>> {
> >>>>> unsigned long flags;
> >>>>>
> >>>>> raw_spin_lock_irqsave(&tp->mac_ocp_lock, flags);
> >>>>> __r8168_mac_ocp_write(tp, reg, data);
> >>>>> raw_spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
> >>>>> }
> >>>>>
> >>>>> Register programming is done through RTL_W32() macro which expands into
> >>>>>
> >>>>> #define RTL_W32(tp, reg, val32) writel((val32), tp->mmio_addr + (reg))
> >>>>>
> >>>>> which is further (on Alpha):
> >>>>>
> >>>>> extern inline void writel(u32 b, volatile void __iomem *addr)
> >>>>> {
> >>>>> mb();
> >>>>> __raw_writel(b, addr);
> >>>>> }
> >>>>>
> >>>>> or on i386/x86_64:
> >>>>>
> >>>>> #define build_mmio_write(name, size, type, reg, barrier) \
> >>>>> static inline void name(type val, volatile void __iomem *addr) \
> >>>>> { asm volatile("mov" size " %0,%1": :reg (val), \
> >>>>> "m" (*(volatile type __force *)addr) barrier); }
> >>>>>
> >>>>> build_mmio_write(writel, "l", unsigned int, "r", :"memory")
> >>>>>
> >>>>> This obviously involves iat least a compiler barrier.
> >>>>>
> >>>>> mb() expands into something like this i.e. on x86_64:
> >>>>>
> >>>>> #define mb() asm volatile("lock; addl $0,0(%%esp)" ::: "memory")
> >>>>>
> >>>>> This means a whole lot of memory bus barriers: for spin_lock_irqsave(),
> >>>>> memory barrier, writel(), and spin_unlock_irqrestore().
> >>>>>
> >>>>> With about 130 of these sequential calls to r8168_mac_ocp_write() this looks like
> >>>>> a LOCK storm that will thunder all of the cores and CPUs on the same memory controller
> >>>>> for certain time that locked memory read-modify-write cyclo or I/O takes to finish.
> >>>>>
> >>>>> In a sequential case of RTL register programming, the writes to RTL registers
> >>>>> can be coalesced under a same raw spinlock. This can dramatically decrease the
> >>>>> number of bus stalls in a multicore or multi-CPU system:
> >>>>>
> >>>>> static void __r8168_mac_ocp_write_seq(struct rtl8169_private *tp,
> >>>>> const struct recover_8411b_info *array)
> >>>>> {
> >>>>> struct recover_8411b_info const *p = array;
> >>>>>
> >>>>> while (p->reg) {
> >>>>> if (!rtl_ocp_reg_failure(p->reg))
> >>>>> RTL_W32(tp, OCPDR, OCPAR_FLAG | (p->reg << 15) | p->data);
> >>>>> p++;
> >>>>> }
> >>>>> }
> >>>>>
> >>>>> static void r8168_mac_ocp_write_seq(struct rtl8169_private *tp,
> >>>>> const struct recover_8411b_info *array)
> >>>>> {
> >>>>> unsigned long flags;
> >>>>>
> >>>>> raw_spin_lock_irqsave(&tp->mac_ocp_lock, flags);
> >>>>> __r8168_mac_ocp_write_seq(tp, array);
> >>>>> raw_spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
> >>>>> }
> >>>>>
> >>>>> static void rtl_hw_start_8411_2(struct rtl8169_private *tp)
> >>>>> {
> >>>>>
> >>>>> ...
> >>>>>
> >>>>> /* The following Realtek-provided magic fixes an issue with the RX unit
> >>>>> * getting confused after the PHY having been powered-down.
> >>>>> */
> >>>>>
> >>>>> static const struct recover_8411b_info init_zero_seq[] = {
> >>>>> { 0xFC28, 0x0000 }, { 0xFC2A, 0x0000 }, { 0xFC2C, 0x0000 }, { 0xFC2E, 0x0000 },
> >>>>> ...
> >>>>> };
> >>>>>
> >>>>> static const struct recover_8411b_info recover_seq[] = {
> >>>>> { 0xF800, 0xE008 }, { 0xF802, 0xE00A }, { 0xF804, 0xE00C }, { 0xF806, 0xE00E },
> >>>>> ...
> >>>>> };
> >>>>>
> >>>>> static const struct recover_8411b_info final_seq[] = {
> >>>>> { 0xFC2A, 0x0743 }, { 0xFC2C, 0x0801 }, { 0xFC2E, 0x0BE9 }, { 0xFC30, 0x02FD },
> >>>>> ...
> >>>>> };
> >>>>>
> >>>>> r8168_mac_ocp_write_seq(tp, init_zero_seq);
> >>>>> mdelay(3);
> >>>>> r8168_mac_ocp_write(tp, 0xFC26, 0x0000);
> >>>>> r8168_mac_ocp_write_seq(tp, recover_seq);
> >>>>> r8168_mac_ocp_write(tp, 0xFC26, 0x8000);
> >>>>> r8168_mac_ocp_write_seq(tp, final_seq);
> >>>>> }
> >>>>>
> >>>>> The hex data is preserved intact through s/r8168_mac_ocp_write[(]tp,/{ / and s/[)];/ },/
> >>>>> functions that only changed the function names and the ending of the line, so the actual
> >>>>> hex data is unchanged.
> >>>>>
> >>>>> Note that the original reason for the introduction of the commit fe4e8db0392a6
> >>>>> was to enable recovery of the RX unit on the RTL8411b which was confused by the
> >>>>> powered-down PHY. This sequence of r8168_mac_ocp_write() calls amplifies the problem
> >>>>
> >>>> I still have a problem with this statement as you're saying that the original
> >>>> problem still exists. I don't think that's the case.
> >>>
> >>> I will not disagree about it.
> >>>
> >>> But we have only reduced the number of spin_lock_irqsave/spin_unlock_irqrestore()
> >>> pairs.
> >>>
> >>> Maybe additionally, on the low level, memory barrier isn't required for each write to
> >>> MMIO?
> >>>
> >> One could argue whether in several places writel_relaxed() could be used.
> >> But it's not really worth it, because we're not in a hot path.
> >
> > I see. Thank you for your evaluation.
> >
> > Using writel_relaxed() sounds clever. It expands to:
> >
> > #define build_mmio_write(name, size, type, reg, barrier) \
> > static inline void name(type val, volatile void __iomem *addr) \
> > { asm volatile("mov" size " %0,%1": :reg (val), \
> > "m" (*(volatile type __force *)addr) barrier); }
> > build_mmio_write(__writel, "l", unsigned int, "r", )
> > #define writel_relaxed(v, a) __writel(v, a)
> >
> > Here "barrier" is an empty string. Really clever. ;-)
> >
> > I will not contradict, but the cummulative amount of memory barriers on each MMIO read/write
> > in each single one of the drivers could amount to some degrading of overall performance and
> > latency in a multicore system.
> >
> > As I understood Mr. Jonathan Corbet on LWN, the initiative and trend is to reduce overall
> > kernel latency.
>
> P.S.
>
> On the second thought, if barrier() is only the compiler optimisation barrier from memory
> reordering, then we do not gain much disablin git as it doesn't affect the other cores, and
> reordering MMIO writes can really confuse some NIC hardware.
>
> /* Optimization barrier */
> #ifndef barrier
> /* The "volatile" is due to gcc bugs */
> # define barrier() __asm__ __volatile__("": : :"memory")
> #endif
>
> >>> If it still uses a LOCK addl $0, m32/m64, then it still creates 130 instances of all core
> >>> bus locks for this NIC reset after the lost PHY? I'm just thinking, this is nothing
> >>> authoritative ...

I would recommend looking at the pre-processed source code if you
can't manually untangle the maze of macros. ;-)

Look at the .${obj}.cmd file (e.g.
drivers/net/ethernet/realtek/.r8169_main.o.cmd), copy the compiler
command in it, and add the "-E" option and make the compiler write the
result to some temporary file you can inspect.