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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.01.2021 17:08, Roger Pau Monné wrote:
> 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."

And which one is "the previous call"? I'm still getting the
impression you're mixing up cleanup_domain_irq_pirq() and
clear_domain_irq_pirq(). (I guess we need to resort to line
numbers in current staging or master, to avoid
misunderstandings. Not for the commit message [if any], but
for the discussion.)

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

No, for the moment I don't agree yet, because I don't see
the redundancy so far.

>>> 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 wouldn't mind, but yes, besides review difficulties ...

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

... this also wouldn't be overly nice (but can be dealt with
via a few more steps wading through git history).

Jan