Messages in this thread | | | Date | Mon, 30 Oct 2023 16:13:25 +0100 | Subject | Re: [PATCH v3 1/1] r8169: Coalesce RTL8411b PHY power-down recovery programming instructions to reduce spinlock stalls | From | Mirsad Todorovac <> |
| |
On 10/30/23 15:05, Marco Elver wrote: > On Mon, 30 Oct 2023 at 14:53, Mirsad Todorovac > <mirsad.todorovac@alu.unizg.hr> 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.
Thanks for the tip. I tried to add
EXTRA_CFLAGS += -save-temps
to the driver/net/ethernet/realtek/Kbuild, but something went wrong. Now I see that it is deprecated.
Best regards, Mirsad Todorovac
| |