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