| Message ID | 20210126110606.21741-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/intr: interrupt related fixes | expand |
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
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.
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 --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 ) {
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(-)