Message-ID: <19e2d5fc-7e30-4bb2-943c-f83b44099192@alu.unizg.hr>
Date:   Tue, 31 Oct 2023 04:51:10 +0100
From:   Mirsad Todorovac <mirsad.todorovac@....unizg.hr>
To:     Jacob Keller <jacob.e.keller@...el.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Jason Gunthorpe <jgg@...pe.ca>, Joerg Roedel <jroedel@...e.de>,
        Lu Baolu <baolu.lu@...ux.intel.com>, iommu@...ts.linux.dev,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Cc:     Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>, nic_swsd@...ltek.com,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Marco Elver <elver@...gle.com>
Subject: Re: [PATCH v4 1/5] r8169: Coalesce r8169_mac_ocp_write/modify calls
 to reduce spinlock stalls



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

Powered by blists - more mailing lists