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

[Juergen Gross <jgross@xxxxxxxx>]

On 12.04.21 11:32, Jan Beulich wrote:
> 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.

No, they can't race. And yes, the path is really unlikely, so I didn't
want to optimize this rare case.

> 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?

Yes. All required data should be in the caches already, and the extra
processing is only a few instructions.

> 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.)

In theory, yes. OTOH this no longer applies to upstream, so i dind't
bother doing that for stable.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature