Re: [PATCH net-next 2/4] net: macb: Consolidate MACB_CAPS_ISR_CLEAR_ON_WRITE checks in IRQ handler

Nicolai Buchwitz
2026-04-01T11:49:33Z

On 1.4.2026 11:30, Kevin Hao wrote:
>> On Sat, 28 Mar 2026 18:17:46 +0800 Kevin Hao wrote:
> On Tue, Mar 31, 2026 at 07:54:00PM -0700, Jakub Kicinski wrote:
>> > Currently, the MACB_CAPS_ISR_CLEAR_ON_WRITE flag is checked in every
>> > branch of the IRQ handler. This repeated evaluation is unnecessary.
>> > By consolidating the flag check, we eliminate redundant loads of
>> > bp->caps when TX and RX events occur simultaneously, a common scenario
>> > under high network throughput. Additionally, this optimization reduces
>> > the function size from 0x2e8 to 0x2c4.
>>
>> feels a bit subjective TBH. An alternative improvement would be to
>> factor out the conditional to a helper:
>>
>> static void macb_queue_isr_clear(bp, queue, mask)
>> {
>> if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>> queue_writel(queue, ISR, mask);
>> }
>
> In addition to the similar pattern in the macb_interrupt() function,
> there are
> seven other instances of this pattern in the macb driver.
> $ git grep "if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)"
> drivers/net/ethernet/cadence/ | wc -l
> 7
>
> I agree that using a helper function, as you proposed, would reduce the
> number
> of source code lines and improve the readability of the driver.
> However, such
> changes would not affect the final generated code.
>
> The goal of my changes is to reduce both the footprint of
> macb_interrupt() and
> the number of assembly instructions executed within its event loop.
>
> Before the patch, the condition `if (bp->caps &
> MACB_CAPS_ISR_CLEAR_ON_WRITE)`
> produced the following assembly:
> ffff800080d58b1c: 387b6a68 ldrb w8, [x19, x27]
> ffff800080d58b20: 360000c8 tbz w8, #0,
> ffff800080d58b38 <macb_interrupt+0xd0>
>
> After the patch, the condition `if (isr_clear)` results in:
> ffff800080d58a4c: 360000d8 tbz w24, #0,
> ffff800080d58a64 <macb_interrupt+0xac>
>
> Thus, we eliminate two `ldrb` overheads per iteration of the event loop
> in
> macb_interrupt() when both TX and RX events occur simultaneously. This
> also
> reduces the function's footprint by 36 bytes, as the `ldrb`
> instructions are
> omitted in each event branch.
>
> Therefore, your proposed changes and mine serve different purposes.
> I acknowledge that my change represents only a very slight optimization
> for
> the interrupt handler. I am also open to your preference for improving
> source
> code readability. Please let me know your decision.

I'm always a fan of optimizations, but I guess in this case the
saved ldrb is negligible next to the MMIO in the same path. We're
talking a single L1 cache hit (~1ns) vs an uncacheable register
write (~100ns+). Patch 3 also re-reads bp->caps in
macb_interrupt_misc() anyway, undoing the local bool for the misc
path.

I'd ack Jakub's helper approach. A macb_queue_isr_clear() would
be consistent across all callsites, including the 7 other
instances you counted outside macb_interrupt().

> Thanks,
> Kevin

Nicolai