lkml.org 
[lkml]   [2023]   [Oct]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v4 1/5] r8169: Coalesce r8169_mac_ocp_write/modify calls to reduce spinlock stalls
From


On 10/31/23 00:14, Jacob Keller wrote:
>
>
> On 10/30/2023 3:08 PM, Heiner Kallweit wrote:
>> On 30.10.2023 22:50, Jacob Keller wrote:
>>>
>>>
>>> On 10/29/2023 4:04 AM, Mirsad Goran Todorovac wrote:> A pair of new
>>> helpers r8168_mac_ocp_write_seq() and r8168_mac_ocp_modify_seq()
>>>> are introduced.
>>>>
>>>> The motivation for these helpers was the locking overhead of 130 consecutive
>>>> r8168_mac_ocp_write() calls in the RTL8411b reset after the NIC gets confused
>>>> if the PHY is powered-down.
>>>>
>>>> To quote Heiner:
>>>>
>>>> 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, each doing an expensive spin_lock_irqsave() and
>>>> spin_unlock_irqrestore().
>>>>
>>>> Each mac ocp write is made of:
>>>>
>>>> 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 stalls: 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 stall all of the cores and CPUs on the same memory controller
>>>> for certain time 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.
>>>>
>>>> Macro helpers r8168_mac_ocp_write_seq() and r8168_mac_ocp_modify_seq() are
>>>> provided to reduce lock contention:
>>>>
>>>> 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 },
>>>> ...
>>>> };
>>>>
>>>> ...
>>>>
>>>> r8168_mac_ocp_write_seq(tp, init_zero_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.
>>>>
>>>> To repeat, the reason for the introduction of the original commit
>>>> 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
>>>> into a series of about 500+ memory bus locks, most waiting for the main memory read,
>>>> modify and write under a LOCK. The memory barrier in RTL_W32 should suffice for
>>>> the programming sequence to reach RTL NIC registers.
>>>>
>>>> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1692075
>>>>
>>>
>>>
>>> I might have chosen to send some of this information as the cover letter
>>> for the series instead of just as part of the commit message for [1/5],
>>> but either way:
>>>
>>> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
>>
>> Cover letter is still missing, and there's a v5 already.
>> Good example why we have the "max one version per day" rule.
>>
>> There's still some issues with the series, see my review comments
>> for v5. As-is I'd NAK the series.

I realise we need to keep the development process coherent. I am sorry that
my inexperience in the patch submission process made the whole series look bad.

As I previously stated to Mr. Kallweit, I will do the required number of iterations
to ensure the quality of the patches (I saw some go up to over 20 versions).

> Heh, ya. A v5 was sent without there being a single (public) comment on
> the list prior to my reviewing. I didn't notice the v5, and my mail
> scripts pointed out this series didn't have anyone who'd looked at it
> yet.. I guess I could have searched for and noticed a newer version.

Well, dear Sir,

I see I owe you an apology for I did not know about the "max one version per day"
rule. I was warned however not to overwhelm the maintainers by Guillaume Nault in
January and somehow I hypomanicaly OCD'd on this. My fault entirely.

I hope we can mend this.

I guess this is my time to take a break, do some homework and return to the drawing
board.

Besides, now we are in the merge window anyway, so I should thank Mr. Kallweit for
the special attention and for making an exception.

Am I allowed to keep Mr. Keller's Reviewed-by: tags on the reviewed diffs provided
that I fix the cover letter issue and objections?

Have a nice day.

Regards,
Mirsad

\
 
 \ /
  Last update: 2023-10-31 04:52    [W:0.083 / U:0.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site