Re: [PATCH 1/3] x86/irq: remove duplicated clear_domain_irq_pirq calls

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

On Tue, Jan 26, 2021 at 03:38:27PM +0100, Jan Beulich wrote:
> On 26.01.2021 12:06, Roger Pau Monne wrote:
> > There are two duplicated calls to cleanup_domain_irq_pirq in
> > unmap_domain_pirq.
> > 
> > The first one in the for loop will be called with exactly the same
> > arguments as the call placed closer to the loop start.
> 
> I'm having trouble figuring out which two instances you refer
> to: To me "first one" and "closer to the start" are two ways
> of expressing the same thing. You don't refer to the call to
> clear_domain_irq_pirq(), do you, when the two calls you
> remove are to cleanup_domain_irq_pirq()? Admittedly quite
> similar names, but entirely different functions.

Urg, yes, that's impossible to parse sensibly, sorry.

Also the subject is wrong, should be cleanup_domain_irq_pirq, not
clear_domain_irq_pirq.

This should instead be:

"There are two duplicated calls to cleanup_domain_irq_pirq in
unmap_domain_pirq.

The first removed call to cleanup_domain_irq_pirq will be called with
exactly the same arguments as the previous call placed above it."

It's hard to explain this in a commit message.

Do you agree that the removed calls are duplicated though? I might have
as well missed part of the logic here and be wrong about this.

> > The logic used in the loop seems extremely complex to follow IMO,
> > there are several breaks for exiting the loop, and the index (i) is
> > also updated in different places.
> 
> Indeed, and it didn't feel well already back at the time when
> I much extended this to support multi-vector MSI. I simply
> didn't have any good idea how to streamline all of this
> (short of rewriting it altogether, which would have made
> patch review quite difficult). If you have thoughts, I'm all
> ears.

I would just rewrite it altogether. Code like this is very prone to
cause mistakes in the future IMO. If you want I can try to.

I guess the problem with this is that we would lose the history of the
code for no functional change.

Thanks, Roger.