Re: [PATCH] xen/events: fix setting irq affinity

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.04.2021 08:28, Juergen Gross wrote:
> The backport of upstream patch 25da4618af240fbec61 ("xen/events: don't
> unmask an event channel when an eoi is pending") introduced a
> regression for stable kernels 5.10 and older: setting IRQ affinity for
> IRQs related to interdomain events would no longer work, as moving the
> IRQ to its new cpu was not included in the irq_ack callback for those
> events.
> 
> Fix that by adding the needed call.
> 
> Note that kernels 5.11 and later don't need the explicit moving of the
> IRQ to the target cpu in the irq_ack callback, due to a rework of the
> affinity setting in kernel 5.11.
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
> This patch should be applied to all stable kernel branches up to
> (including) linux-5.10.y, where upstream patch 25da4618af240fbec61 has
> been added.
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

This looks functionally correct to me, so:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

But I have remarks / questions:

> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -1809,7 +1809,7 @@ static void lateeoi_ack_dynirq(struct irq_data *data)
>  
>       if (VALID_EVTCHN(evtchn)) {
>               do_mask(info, EVT_MASK_REASON_EOI_PENDING);
> -             event_handler_exit(info);
> +             ack_dynirq(data);
>       }
>  }
>  
> @@ -1820,7 +1820,7 @@ static void lateeoi_mask_ack_dynirq(struct irq_data 
> *data)
>  
>       if (VALID_EVTCHN(evtchn)) {
>               do_mask(info, EVT_MASK_REASON_EXPLICIT);
> -             event_handler_exit(info);
> +             ack_dynirq(data);
>       }
>  }
>  

Can EVT_MASK_REASON_EOI_{PENDING,EXPLICIT} be cleared in a way racing
event_handler_exit() and (if it was called directly from here)
irq_move_masked_irq()? If not, the extra do_mask() / do_unmask() pair
(granted living on an "unlikely" path) could be avoided.

Even leaving aside the extra overhead in ack_dynirq()'s unlikely code
path, there's now some extra (redundant) processing. I guess this is
assumed to be within noise?

Possibly related, but first of all seeing the redundancy between
eoi_pirq() and ack_dynirq(): Wouldn't it make sense to break out the
common part into a helper? (Really the former could simply call the
latter as it seems.)

Jan