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);
+ }
+}
+
static inline void
igb_fix_icr_asserted(IGBCore *core)
{
--
2.47.1On 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.
> static inline void
> igb_fix_icr_asserted(IGBCore *core)
> {
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
© 2016 - 2025 Red Hat, Inc.