Re: [Xen-devel] [PATCH 2/4] x86/IRQ: bail early from irq_guest_eoi_timer_fn() when nothing is in flight

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

On Thu, May 16, 2019 at 06:02:15AM -0600, Jan Beulich wrote:
> >>> On 16.05.19 at 13:37, <roger.pau@xxxxxxxxxx> wrote:
> > On Wed, May 08, 2019 at 06:46:51AM -0600, Jan Beulich wrote:
> >> There's no point entering the loop in the function in this case. Instead
> >> there still being something in flight _after_ the loop would be an
> >> actual problem: No timer would be running anymore for issuing the EOI
> >> eventually, and hence this IRQ (and possibly lower priority ones) would
> >> be blocked, perhaps indefinitely.
> >> 
> >> Issue a warning instead and prefer breaking some (presumably
> >> misbehaving) guest over stalling perhaps the entire system.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> 
> >> --- a/xen/arch/x86/irq.c
> >> +++ b/xen/arch/x86/irq.c
> >> @@ -1115,7 +1115,7 @@ static void irq_guest_eoi_timer_fn(void
> >>  
> >>      action = (irq_guest_action_t *)desc->action;
> >>  
> >> -    if ( timer_is_active(&action->eoi_timer) )
> >> +    if ( !action->in_flight || timer_is_active(&action->eoi_timer) )
> >>          goto out;
> >>  
> >>      if ( action->ack_type != ACKTYPE_NONE )
> >> @@ -1130,8 +1130,10 @@ static void irq_guest_eoi_timer_fn(void
> >>          }
> >>      }
> >>  
> >> -    if ( action->in_flight != 0 )
> >> -        goto out;
> >> +    if ( action->in_flight )
> >> +        printk(XENLOG_G_WARNING
> >> +               "IRQ%d: %d handlers still in flight at forced EOI\n",
> >> +               desc->irq, action->in_flight);
> > 
> > AFAICT action->in_flight should contain the number of guests pirqs
> > that have the pirq masked (pirq->masked == true), because in_flight is
> > only increased by __do_IRQ_guest when the pirq is not already masked.
> > At guest EOI (desc_guest_eoi) the in_flight count is also only
> > decreased if the pirq is unmasked.
> > 
> > Hence I think this condition could be turned into an ASSERT, but I'm
> > likely missing something.
> 
> I don't think you are. Going from if() straight to ASSERT() simply
> seemed too harsh to me, the more in a subsystem where I could
> easily have overlooked some corner case, due to how convoluted
> some of the implementation is.

I agree it's quite convoluted. I think it would be helpful to add an
ASSERT_UNREACHABLE together with the warning message. With that:

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

Thanks, Roger.

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