Re: [Xen-devel] [PATCH 1/4] x86/IRQ: don't keep EOI timer running without need

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, May 16, 2019 at 04:50:22AM -0600, Jan Beulich wrote:
> >>> On 16.05.19 at 12:32, <roger.pau@xxxxxxxxxx> wrote:
> > On Wed, May 08, 2019 at 06:46:25AM -0600, Jan Beulich wrote:
> >> The timer needs to remain active only until all pending IRQ instances
> >> have seen EOIs from their respective domains. Stop it when the in-flight
> >> count has reached zero in desc_guest_eoi(). Note that this is race free
> >> (with __do_IRQ_guest()), as the IRQ descriptor lock is being held at
> >> that point.
> >> 
> >> Also pull up stopping of the timer in __do_IRQ_guest() itself: Instead
> >> of stopping it immediately before re-setting, stop it as soon as we've
> >> made it past any early returns from the function (and hence we're sure
> >> it'll get set again).
> >> 
> >> Finally bail from the actual timer handler in case we find the timer
> >> already active again by the time we've managed to acquire the IRQ
> >> descriptor lock. Without this we may forcibly EOI an IRQ immediately
> >> after it got sent to a guest. For this, timer_is_active() gets split out
> >> of active_timer(), deliberately moving just one of the two ASSERT()s (to
> >> allow the function to be used also on a never initialized timer).
> > 
> > AFAICT timer_is_active is exclusively used in irq_guest_eoi_timer_fn,
> > which must have initialized the timer in order for
> > irq_guest_eoi_timer_fn to be called, and hence I'm not sure why you
> > need to be able to call timer_is_active with an uninitialized timer.
> 
> It's not needed here, but I consider this useful behavior when used
> outside of the specific timer's handler.
> 
> > Is this maybe used by other patches?
> 
> None that I would have in the works.

Then IMO I would rather make timer_is_active a replacement for
active_timer (or just move active_timer to the header) if there's no
user that can call timer_is_active with an uninitialized timer. Ie: I
would keep the asserts as restrictive as possible unless there's a
user that requires less restrictive assertions.

Anyway, the change is an improvement, so with or without that changed:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel