From: LiuChao <[email protected]>
When desc->action is empty, there is no need to print out the irq and its'
count in each cpu. The desc is not alloced in request_irq or freed in
free_irq. So some PCI devices, such as rtl8139, uses request_irq and
free_irq, which only modify the action of desc. So /proc/interrupts could
be like this:
CPU0 CPU1
2: 69397 69267 GICv3 27 Level arch_timer
4: 0 0 GICv3 33 Level uart-pl011
38: 46 0 GICv3 36 Level ehci_hcd:usb1
39: 66 0 GICv3 37 Level
40: 0 0 GICv3 38 Level virtio1
42: 0 0 GICv3 23 Level arm-pmu
43: 0 0 ARMH0061:00 3 Edge ACPI:Event
44: 1 0 ITS-MSI 32768 Edge PCIe PME, pciehp
45: 0 0 ITS-MSI 32769 Edge aerdrv
Irqbalance gets the list of interrupts according to /proc/interrupts. In
this case, irqbalance does not remove the interrupt from the balance list,
and the last string in this line,which is Level, is used as irq_name.
Or we can clear desc->kstat_irqs in each cpu in free_irq when desc->action
is null?
Signed-off-by: LiuChao <[email protected]>
Reviewed-by: Thomas Gleixner <[email protected]>
Cc: [email protected]
---
kernel/irq/proc.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index cfc4f088a0e7..b27169e587f4 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -439,7 +439,7 @@ int show_interrupts(struct seq_file *p, void *v)
{
static int prec;
- unsigned long flags, any_count = 0;
+ unsigned long flags;
int i = *(loff_t *) v, j;
struct irqaction *action;
struct irq_desc *desc;
@@ -466,11 +466,7 @@ int show_interrupts(struct seq_file *p, void *v)
if (!desc)
goto outsparse;
- if (desc->kstat_irqs)
- for_each_online_cpu(j)
- any_count |= *per_cpu_ptr(desc->kstat_irqs, j);
-
- if ((!desc->action || irq_desc_is_chained(desc)) && !any_count)
+ if (!desc->action || irq_desc_is_chained(desc))
goto outsparse;
seq_printf(p, "%*d: ", prec, i);
--
2.19.1
Chao,
l00520965 <[email protected]> writes:
> When desc->action is empty, there is no need to print out the irq and its'
> count in each cpu. The desc is not alloced in request_irq or freed in
> free_irq.
request/free_irq() never allocate/free irq descriptors.
> So some PCI devices, such as rtl8139, uses request_irq and free_irq,
All PCI devices use some variant of request_irq()/free_irq(). The
interrupt descriptors are allocated by the underlying PCI
machinery. They are only allocated/freed when the device driver is
loaded/removed.
And this property exists for _ALL_ interrupts independent of PCI.
> which only modify the action of desc. So /proc/interrupts could be
> like this:
I think you want to explain:
If an interrupt is released via free_irq() without removing the
underlying irq descriptor, the interrupt count of the irq descriptor
is not reset. /proc/interrupt shows such interrupts with an empty
action handler name:
> CPU0 CPU1
> 38: 46 0 GICv3 36 Level ehci_hcd:usb1
> 39: 66 0 GICv3 37 Level
irqbalance fails to detect that this interrupt is not longer in use
and parses the last word in the line 'Level' as the action handler
name.
> Irqbalance gets the list of interrupts according to /proc/interrupts. In
> this case, irqbalance does not remove the interrupt from the balance list,
> and the last string in this line,which is Level, is used as irq_name.
Right, this is historic behaviour and I don't know how irqbalance dealt
with that in the past 20+ years. At least I haven't seen any complaints.
I'm not opposed to suppress the output, but I really want the opinion of
the irqbalance maintainers on that.
> Or we can clear desc->kstat_irqs in each cpu in free_irq when
> desc->action is null?
No, we can't. The historic behaviour is that the total interrupt count
for a device is maintained independent of the number of
request/free_irq() pairs.
> Signed-off-by: LiuChao <[email protected]>
> Reviewed-by: Thomas Gleixner <[email protected]>
I really can't remember that I have reviewed this patch already. Please
don't add tags which claim that some one has reviewed or acked your
patch unless you really got that Reviewed-by or Acked-by from that
person.
Thanks,
tglx
On Wed, Jan 22, 2020 at 01:42:48PM +0100, Thomas Gleixner wrote:
> Chao,
>
> l00520965 <[email protected]> writes:
>
> > When desc->action is empty, there is no need to print out the irq and its'
> > count in each cpu. The desc is not alloced in request_irq or freed in
> > free_irq.
>
> request/free_irq() never allocate/free irq descriptors.
>
> > So some PCI devices, such as rtl8139, uses request_irq and free_irq,
>
> All PCI devices use some variant of request_irq()/free_irq(). The
> interrupt descriptors are allocated by the underlying PCI
> machinery. They are only allocated/freed when the device driver is
> loaded/removed.
>
> And this property exists for _ALL_ interrupts independent of PCI.
>
> > which only modify the action of desc. So /proc/interrupts could be
> > like this:
>
> I think you want to explain:
>
> If an interrupt is released via free_irq() without removing the
> underlying irq descriptor, the interrupt count of the irq descriptor
> is not reset. /proc/interrupt shows such interrupts with an empty
> action handler name:
>
> > CPU0 CPU1
> > 38: 46 0 GICv3 36 Level ehci_hcd:usb1
> > 39: 66 0 GICv3 37 Level
>
> irqbalance fails to detect that this interrupt is not longer in use
> and parses the last word in the line 'Level' as the action handler
> name.
>
> > Irqbalance gets the list of interrupts according to /proc/interrupts. In
> > this case, irqbalance does not remove the interrupt from the balance list,
> > and the last string in this line,which is Level, is used as irq_name.
>
> Right, this is historic behaviour and I don't know how irqbalance dealt
> with that in the past 20+ years. At least I haven't seen any complaints.
>
> I'm not opposed to suppress the output, but I really want the opinion of
> the irqbalance maintainers on that.
>
Actually, irqbalance ignores the trailing irq name (or it should at
least), so you should be able to drop that portion of /proc/irqbalance,
though I cant speak for any other users of it.
> > Or we can clear desc->kstat_irqs in each cpu in free_irq when
> > desc->action is null?
>
> No, we can't. The historic behaviour is that the total interrupt count
> for a device is maintained independent of the number of
> request/free_irq() pairs.
>
> > Signed-off-by: LiuChao <[email protected]>
> > Reviewed-by: Thomas Gleixner <[email protected]>
>
> I really can't remember that I have reviewed this patch already. Please
> don't add tags which claim that some one has reviewed or acked your
> patch unless you really got that Reviewed-by or Acked-by from that
> person.
>
> Thanks,
>
> tglx
>
On Thu, Jan 23, 2020 at 03:29AM +0800, Neil Horman wrote:
> On Wed, Jan 22, 2020 at 01:42:48PM +0100, Thomas Gleixner wrote:
> > Chao,
> >
> > l00520965 <[email protected]> writes:
> >
> > > When desc->action is empty, there is no need to print out the irq and its'
> > > count in each cpu. The desc is not alloced in request_irq or freed
> > > in free_irq.
> >
> > request/free_irq() never allocate/free irq descriptors.
> >
> > > So some PCI devices, such as rtl8139, uses request_irq and free_irq,
> >
> > All PCI devices use some variant of request_irq()/free_irq(). The
> > interrupt descriptors are allocated by the underlying PCI machinery.
> > They are only allocated/freed when the device driver is
> > loaded/removed.
> >
> > And this property exists for _ALL_ interrupts independent of PCI.
> >
> > > which only modify the action of desc. So /proc/interrupts could be
> > > like this:
> >
> > I think you want to explain:
> >
> > If an interrupt is released via free_irq() without removing the
> > underlying irq descriptor, the interrupt count of the irq descriptor
> > is not reset. /proc/interrupt shows such interrupts with an empty
> > action handler name:
> >
> > > CPU0 CPU1
> > > 38: 46 0 GICv3 36 Level ehci_hcd:usb1
> > > 39: 66 0 GICv3 37 Level
> >
> > irqbalance fails to detect that this interrupt is not longer in use
> > and parses the last word in the line 'Level' as the action handler
> > name.
> >
> > > Irqbalance gets the list of interrupts according to
> > > /proc/interrupts. In this case, irqbalance does not remove the
> > > interrupt from the balance list, and the last string in this line,which is Level,
> is used as irq_name.
> >
> > Right, this is historic behaviour and I don't know how irqbalance
> > dealt with that in the past 20+ years. At least I haven't seen any complaints.
> >
> > I'm not opposed to suppress the output, but I really want the opinion
> > of the irqbalance maintainers on that.
Irqbalance is an example. I mean, when this happens, users who cat /proc/interrupts
may be confused about where the interrupt came from and what it was used for.
People who use Linux may not understand the principle of this. They are not sure
whether this is a problem of the system or not.
> >
> Actually, irqbalance ignores the trailing irq name (or it should at least), so you
> should be able to drop that portion of /proc/irqbalance, though I cant speak for
> any other users of it.
If irq isn't removed from /proc/interrups, it will still be parsed in collect_full_irq_list
and parse_proc_interrupts. irq_name is used in guess_arm_irq_hints.
>
> > > Or we can clear desc->kstat_irqs in each cpu in free_irq when
> > > desc->action is null?
> >
> > No, we can't. The historic behaviour is that the total interrupt count
> > for a device is maintained independent of the number of
> > request/free_irq() pairs.
> >
> > > Signed-off-by: LiuChao <[email protected]>
> > > Reviewed-by: Thomas Gleixner <[email protected]>
> >
> > I really can't remember that I have reviewed this patch already.
> > Please don't add tags which claim that some one has reviewed or acked
> > your patch unless you really got that Reviewed-by or Acked-by from
> > that person.
> >
> > Thanks,
> >
> > tglx
> >
Thanks,
LiuChao
Chao,
"liuchao (CR)" <[email protected]> writes:
> On Thu, Jan 23, 2020 at 03:29AM +0800, Neil Horman wrote:
>> > I'm not opposed to suppress the output, but I really want the opinion
>> > of the irqbalance maintainers on that.
>
> Irqbalance is an example. I mean, when this happens, users who cat /proc/interrupts
> may be confused about where the interrupt came from and what it was used for.
> People who use Linux may not understand the principle of this. They are not sure
> whether this is a problem of the system or not.
Well, this has been that way for 20+ years and so far nobody got
confused. If it's not documented then we should do so.
>> Actually, irqbalance ignores the trailing irq name (or it should at least), so you
>> should be able to drop that portion of /proc/irqbalance, though I cant speak for
>> any other users of it.
>
> If irq isn't removed from /proc/interrups, it will still be parsed in
> collect_full_irq_list and parse_proc_interrupts.
Sure, and why is that a problem? Again, this is really historic behaviour.
> irq_name is used in guess_arm_irq_hints.
That's a problem of guess_arm_irq_hints() then.
Again, I'm not against supressing such lines in general, but I want to
make sure that no tool depends on that information.
Thanks,
tglx
On Thu, Jan 23, 2020 at 01:34:33PM +0100, Thomas Gleixner wrote:
> Chao,
>
> "liuchao (CR)" <[email protected]> writes:
> > On Thu, Jan 23, 2020 at 03:29AM +0800, Neil Horman wrote:
> >> > I'm not opposed to suppress the output, but I really want the opinion
> >> > of the irqbalance maintainers on that.
> >
> > Irqbalance is an example. I mean, when this happens, users who cat /proc/interrupts
> > may be confused about where the interrupt came from and what it was used for.
> > People who use Linux may not understand the principle of this. They are not sure
> > whether this is a problem of the system or not.
>
> Well, this has been that way for 20+ years and so far nobody got
> confused. If it's not documented then we should do so.
>
> >> Actually, irqbalance ignores the trailing irq name (or it should at least), so you
> >> should be able to drop that portion of /proc/irqbalance, though I cant speak for
> >> any other users of it.
> >
> > If irq isn't removed from /proc/interrups, it will still be parsed in
> > collect_full_irq_list and parse_proc_interrupts.
>
> Sure, and why is that a problem? Again, this is really historic behaviour.
>
> > irq_name is used in guess_arm_irq_hints.
>
> That's a problem of guess_arm_irq_hints() then.
>
> Again, I'm not against supressing such lines in general, but I want to
> make sure that no tool depends on that information.
>
I think it probably makes sense to just keep it then. I'm not sure I
see it as hurting anything to keep it around.
Neil
> Thanks,
>
> tglx
>