Re: [PATCH v3 11/12] net/e1000e|igb: Fix interrupt throttling rearming

["Nicholas Piggin" <npiggin@gmail.com>]

On Mon May 5, 2025 at 4:03 PM AEST, Akihiko Odaki wrote:
> On 2025/05/02 12:17, Nicholas Piggin wrote:
>> Timer expiry that results in an interrupt does not rearm the timer so
>> an interrupt can appear immediately after the interrupt generated by
>> timer expiry.
>> 
>> Fix this by rearming the throttle timer when a delayed interrupt is
>> processed. e1000e gets this by reusing the e1000e_msix_notify()
>> logic, igb calls igb_intrmgr_rearm_timer() directly.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   hw/net/e1000e_core.c |  5 ++--
>>   hw/net/igb_core.c    | 55 ++++++++++++++++++++++++++------------------
>>   2 files changed, 35 insertions(+), 25 deletions(-)
>> 
>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>> index d53f70065ef..2932122c04b 100644
>> --- a/hw/net/e1000e_core.c
>> +++ b/hw/net/e1000e_core.c
>> @@ -218,7 +218,7 @@ static uint32_t find_msix_causes(E1000ECore *core, int vec)
>>   }
>>   
>>   static void
>> -e1000e_msix_auto_clear_mask(E1000ECore *core, uint32_t cause);
>> +e1000e_msix_notify(E1000ECore *core, uint32_t causes);
>>   
>>   static void
>>   e1000e_intrmgr_on_msix_throttling_timer(void *opaque)
>> @@ -233,8 +233,7 @@ e1000e_intrmgr_on_msix_throttling_timer(void *opaque)
>>       causes = find_msix_causes(core, idx) & core->mac[IMS] & core->mac[ICR];
>>       if (causes) {
>>           trace_e1000e_irq_msix_notify_postponed_vec(idx);
>> -        msix_notify(core->owner, causes);
>> -        e1000e_msix_auto_clear_mask(core, causes);
>> +        e1000e_msix_notify(core, causes);
>>       }
>>   }
>>   
>> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
>> index 035637f81f8..cc25a1d5baa 100644
>> --- a/hw/net/igb_core.c
>> +++ b/hw/net/igb_core.c
>> @@ -152,11 +152,14 @@ igb_intrmgr_arm_timer(IGBIntrDelayTimer *timer, int64_t delay_ns)
>>   static inline void
>>   igb_intrmgr_rearm_timer(IGBIntrDelayTimer *timer)
>>   {
>> -    uint32_t interval = (timer->core->mac[timer->delay_reg] &
>> -                         E1000_EITR_INTERVAL) >> 2;
>> -    int64_t delay_ns = (int64_t)interval * timer->delay_resolution_ns;
>> +    uint32_t eitr = timer->core->mac[timer->delay_reg];
>>   
>> -    igb_intrmgr_arm_timer(timer, delay_ns);
>> +    if (eitr != 0) {
>> +        uint32_t interval = (eitr & E1000_EITR_INTERVAL) >> 2;
>> +        int64_t delay_ns = (int64_t)interval * timer->delay_resolution_ns;
>> +
>> +        igb_intrmgr_arm_timer(timer, delay_ns);
>> +    }
>>   }
>>   
>>   static void
>> @@ -168,21 +171,7 @@ igb_intmgr_timer_resume(IGBIntrDelayTimer *timer)
>>   }
>>   
>>   static void
>> -igb_intrmgr_on_msix_throttling_timer(void *opaque)
>> -{
>> -    IGBIntrDelayTimer *timer = opaque;
>> -    IGBCore *core = timer->core;
>> -    int vector = timer - &core->eitr[0];
>> -    uint32_t causes;
>> -
>> -    timer->running = false;
>> -
>> -    causes = core->mac[EICR] & core->mac[EIMS];
>> -    if (causes & BIT(vector)) {
>> -        trace_e1000e_irq_msix_notify_postponed_vec(vector);
>> -        igb_msix_notify(core, vector);
>> -    }
>> -}
>> +igb_intrmgr_on_msix_throttling_timer(void *opaque);
>>   
>>   static void
>>   igb_intrmgr_initialize_all_timers(IGBCore *core, bool create)
>> @@ -2258,9 +2247,7 @@ igb_postpone_interrupt(IGBIntrDelayTimer *timer)
>>           return true;
>>       }
>>   
>> -    if (timer->core->mac[timer->delay_reg] != 0) {
>> -        igb_intrmgr_rearm_timer(timer);
>> -    }
>> +    igb_intrmgr_rearm_timer(timer);
>>   
>>       return false;
>>   }
>> @@ -2284,6 +2271,30 @@ static void igb_send_msix(IGBCore *core, uint32_t causes)
>>       }
>>   }
>>   
>> +static void
>> +igb_intrmgr_on_msix_throttling_timer(void *opaque)
>> +{
>> +    IGBIntrDelayTimer *timer = opaque;
>> +    IGBCore *core = timer->core;
>> +    int vector = timer - &core->eitr[0];
>> +    uint32_t causes;
>> +
>> +    timer->running = false;
>> +
>> +    causes = core->mac[EICR] & core->mac[EIMS];
>> +    if (causes & BIT(vector)) {
>> +        /*
>> +         * The moderation counter is loaded with interval value whenever the
>> +         * interrupt is signaled. This includes when the interrupt is signaled
>> +         * by the counter reaching 0.
>> +         */
>> +        igb_intrmgr_rearm_timer(timer);
>> +
>> +        trace_e1000e_irq_msix_notify_postponed_vec(vector);
>> +        igb_msix_notify(core, vector);
>> +    }
>> +}
>> +
>
> I wonder why the definition is moved. This patch adds a 
> igb_intrmgr_rearm_timer() call but it's already placed earlier than this 
> function.

Hmm, I did require moving it at one point, but I must have reworked it
enough to avoid it. That makes the patch smaller and nicer. Another
good catch.

Thanks,
Nick