diff mbox series

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

Message ID 20210126110606.21741-2-roger.pau@citrix.com (mailing list archive)
State New
Headers show
Series x86/intr: interrupt related fixes | expand

Commit Message

Roger Pau Monné Jan. 26, 2021, 11:06 a.m. UTC
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.

The second one will only be executed when desc != NULL, and that's
already covered by the first call in the for loop above, as any
attempt to unmap a multi vector MSI range will have nr != 1 and thus
always exit the loop with desc == NULL.

Note that those calls are harmless, but make the code harder to read.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
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.
---
 xen/arch/x86/irq.c | 6 ------
 1 file changed, 6 deletions(-)

Comments

Jan Beulich Jan. 26, 2021, 2:38 p.m. UTC | #1
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.

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

Jan
Roger Pau Monné Jan. 26, 2021, 4:08 p.m. UTC | #2
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.
Jan Beulich Jan. 26, 2021, 4:13 p.m. UTC | #3
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
diff mbox series

Patch

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 032fe82167..49849bd7d3 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2383,9 +2383,6 @@  int unmap_domain_pirq(struct domain *d, int pirq)
 
         spin_unlock_irqrestore(&desc->lock, flags);
 
-        if ( !forced_unbind )
-           cleanup_domain_irq_pirq(d, irq, info);
-
         rc = irq_deny_access(d, irq);
         if ( rc )
         {
@@ -2419,9 +2416,6 @@  int unmap_domain_pirq(struct domain *d, int pirq)
     {
         spin_unlock_irqrestore(&desc->lock, flags);
 
-        if ( !forced_unbind )
-            cleanup_domain_irq_pirq(d, irq, info);
-
         rc = irq_deny_access(d, irq);
         if ( rc )
         {