[PATCH 0/3 v2] softirq: Statistics for softirq

[PATCH 0/3 v2] softirq: Statistics for softirq

[Keika Kobayashi]

This is v2 of statistics for softirq patch series.
Thanks to Andrew for comments!
These patches are fixed about the following.

Against: next-20081121

Change Log v1 -> v2
[2/3] ...  proc: Export statistics for softirq to /proc
o Change from for_each_online_cpu() to for_each_possible_cpu()
  in show_softirqs().
o Make "proc_softirqs_operations" const.
o Don't introduce for_each_softirq_nr() for readability.

[PATCH 1/3 v2] softirq: Introduce statistics for softirq

[Keika Kobayashi]

Statistics for softirq doesn't exist.
It will be helpful like statistics for interrupts.
This patch introduces counting the number of softirq,
which will be exported in /proc/softirqs.

When softirq handler consumes much CPU time,
/proc/stat is like the following.

$ while :; do  cat /proc/stat | head -n1 ; sleep 10 ; done
cpu  88 0 408 739665 583 28 2 0 0
cpu  450 0 1090 740970 594 28 1294 0 0
                              ^^^^
                             softirq

In such a situation,
/proc/softirqs shows us which softirq handler is invoked.
We can see the increase rate of softirqs.

<before>
$ cat /proc/softirqs
                CPU0       CPU1       CPU2       CPU3
HI                 0          0          0          0
TIMER         462850     462805     462782     462718
NET_TX             0          0          0        365
NET_RX          2472          2          2         40
BLOCK              0          0        381       1164
TASKLET            0          0          0        224
SCHED         462654     462689     462698     462427
RCU             3046       2423       3367       3173

<after>
$ cat /proc/softirqs
                CPU0       CPU1       CPU2       CPU3
HI                 0          0          0          0
TIMER         463361     465077     465056     464991
NET_TX            53          0          1        365
NET_RX          3757          2          2         40
BLOCK              0          0        398       1170
TASKLET            0          0          0        224
SCHED         463074     464318     464612     463330
RCU             3505       2948       3947       3673

When CPU TIME of softirq is high,
the rates of increase is the following.
  TIMER  : 220/sec     : CPU1-3
  NET_TX : 5/sec       : CPU0
  NET_RX : 120/sec     : CPU0
  SCHED  : 40-200/sec  : all CPU
  RCU    : 45-58/sec   : all CPU

The rates of increase in an idle mode is the following.
  TIMER  : 250/sec
  SCHED  : 250/sec
  RCU    : 2/sec

It seems many softirqs for receiving packets and rcu are invoked.
This gives us help for checking system.

Signed-off-by: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
Reviewed-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
---
 include/linux/kernel_stat.h |   12 ++++++++++++
 kernel/softirq.c            |    1 +
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/include/linux/kernel_stat.h b/include/linux/kernel_stat.h
index 4a145ca..57c1643 100644
--- a/include/linux/kernel_stat.h
+++ b/include/linux/kernel_stat.h
@@ -5,6 +5,7 @@
 #include <linux/threads.h>
 #include <linux/percpu.h>
 #include <linux/cpumask.h>
+#include <linux/interrupt.h>
 #include <asm/irq.h>
 #include <asm/cputime.h>
 
@@ -29,6 +30,7 @@ struct cpu_usage_stat {
 struct kernel_stat {
 	struct cpu_usage_stat	cpustat;
 	unsigned int irqs[NR_IRQS];
+	unsigned int softirqs[NR_SOFTIRQS];
 };
 
 DECLARE_PER_CPU(struct kernel_stat, kstat);
@@ -52,6 +54,16 @@ static inline unsigned int kstat_irqs_cpu(unsigned int irq, int cpu)
        return kstat_cpu(cpu).irqs[irq];
 }
 
+static inline void kstat_incr_softirqs_this_cpu(unsigned int irq)
+{
+	kstat_this_cpu.softirqs[irq]++;
+}
+
+static inline unsigned int kstat_softirqs_cpu(unsigned int irq, int cpu)
+{
+       return kstat_cpu(cpu).softirqs[irq];
+}
+
 /*
  * Number of interrupts per specific IRQ source, since bootup
  */
diff --git a/kernel/softirq.c b/kernel/softirq.c
index e7c69a7..088e179 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -208,6 +208,7 @@ restart:
 	do {
 		if (pending & 1) {
 			int prev_count = preempt_count();
+			kstat_incr_softirqs_this_cpu(h - softirq_vec);
 
 			h->action(h);
 
-- 
1.5.0.6

[PATCH 2/3 v2] proc: Export statistics for softirq to /proc

[Keika Kobayashi]

Export statistics for softirq in /proc/softirqs and /proc/stat.

1. /proc/softirqs
Implement /proc/softirqs which shows the number of softirq
for each CPU like /proc/interrupts.

2. /proc/stat
Add the "softirq" line to /proc/stat.
This line shows the number of softirq for all cpu.
The first column is the total of all softirqs and
each subsequent column is the total for particular softirq.

Signed-off-by: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
Reviewed-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
---
 fs/proc/Makefile   |    1 +
 fs/proc/softirqs.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/proc/stat.c     |   17 +++++++++++++++
 3 files changed, 75 insertions(+), 0 deletions(-)
 create mode 100644 fs/proc/softirqs.c

diff --git a/fs/proc/Makefile b/fs/proc/Makefile
index 63d9651..11a7b5c 100644
--- a/fs/proc/Makefile
+++ b/fs/proc/Makefile
@@ -18,6 +18,7 @@ proc-y	+= meminfo.o
 proc-y	+= stat.o
 proc-y	+= uptime.o
 proc-y	+= version.o
+proc-y	+= softirqs.o
 proc-$(CONFIG_PROC_SYSCTL)	+= proc_sysctl.o
 proc-$(CONFIG_NET)		+= proc_net.o
 proc-$(CONFIG_PROC_KCORE)	+= kcore.o
diff --git a/fs/proc/softirqs.c b/fs/proc/softirqs.c
new file mode 100644
index 0000000..543f9d9
--- /dev/null
+++ b/fs/proc/softirqs.c
@@ -0,0 +1,57 @@
+#include <linux/init.h>
+#include <linux/kernel_stat.h>
+#include <linux/proc_fs.h>
+#include <linux/seq_file.h>
+
+static const char *desc_array[] = {
+	"HI",
+	"TIMER",
+	"NET_TX",
+	"NET_RX",
+	"BLOCK",
+	"TASKLET",
+	"SCHED",
+#ifdef CONFIG_HIGH_RES_TIMERS
+	"HRTIMER",
+#endif
+	"RCU"};
+
+/*
+ * /proc/softirqs  ... display the number of softirqs
+ */
+static int show_softirqs(struct seq_file *p, void *v)
+{
+	int i, j;
+
+	seq_printf(p, "                ");
+	for_each_possible_cpu(i)
+		seq_printf(p, "CPU%-8d", i);
+	seq_printf(p, "\n");
+
+	for (i = 0; i < NR_SOFTIRQS; i++) {
+		seq_printf(p, "%-9s", desc_array[i]);
+		for_each_possible_cpu(j)
+			seq_printf(p, " %10u", kstat_softirqs_cpu(i, j));
+		seq_printf(p, "\n");
+	}
+	return 0;
+}
+
+static int softirqs_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, show_softirqs, NULL);
+}
+
+static const struct file_operations proc_softirqs_operations = {
+	.open		= softirqs_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static int __init proc_softirqs_init(void)
+{
+	proc_create("softirqs", 0, NULL, &proc_softirqs_operations);
+	return 0;
+}
+module_init(proc_softirqs_init);
diff --git a/fs/proc/stat.c b/fs/proc/stat.c
index 81904f0..74ac85a 100644
--- a/fs/proc/stat.c
+++ b/fs/proc/stat.c
@@ -25,6 +25,7 @@ static int show_stat(struct seq_file *p, void *v)
 	cputime64_t user, nice, system, idle, iowait, irq, softirq, steal;
 	cputime64_t guest;
 	u64 sum = 0;
+	u64 sum_softirq = 0;
 	struct timespec boottime;
 	unsigned int per_irq_sum;
 
@@ -49,6 +50,10 @@ static int show_stat(struct seq_file *p, void *v)
 			sum += kstat_irqs_cpu(j, i);
 
 		sum += arch_irq_stat_cpu(i);
+
+		for (j = 0; j < NR_SOFTIRQS; j++)
+			sum_softirq += kstat_softirqs_cpu(j, i);
+
 	}
 	sum += arch_irq_stat();
 
@@ -111,6 +116,18 @@ static int show_stat(struct seq_file *p, void *v)
 		nr_running(),
 		nr_iowait());
 
+	seq_printf(p, "softirq %llu", (unsigned long long)sum_softirq);
+
+	for (i = 0; i < NR_SOFTIRQS; i++) {
+		per_irq_sum = 0;
+
+		for_each_possible_cpu(j)
+			per_irq_sum += kstat_softirqs_cpu(i, j);
+
+		seq_printf(p, " %u", per_irq_sum);
+	}
+	seq_printf(p, "\n");
+
 	return 0;
 }
 
-- 
1.5.0.6

[PATCH 3/3 v2] proc: Update document for /proc/softirqs and /proc/stat

[Keika Kobayashi]

/proc/softirqs and /proc/stat have been changed.
This patch updates proc.txt for these usage.

Signed-off-by: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
Reviewed-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
---
 Documentation/filesystems/proc.txt |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index bcceb99..ef1eeae 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -288,6 +288,7 @@ Table 1-4: Kernel info in /proc
  rtc         Real time clock                                   
  scsi        SCSI info (see text)                              
  slabinfo    Slab pool info                                    
+ softirqs    softirq usage
  stat        Overall statistics                                
  swaps       Swap space utilization                            
  sys         See chapter 2                                     
@@ -602,6 +603,24 @@ on the kind of area :
 0xffffffffa0017000-0xffffffffa0022000   45056 sys_init_module+0xc27/0x1d00 ...
    pages=10 vmalloc N0=10
 
+..............................................................................
+
+softirqs:
+
+Provides counts of softirq handlers serviced since boot time, for each cpu.
+
+> cat /proc/softirqs
+                CPU0       CPU1       CPU2       CPU3
+HI                 0          0          0          0
+TIMER          27166      27120      27097      27034
+NET_TX             0          0          0         17
+NET_RX            42          0          0         39
+BLOCK              0          0        107       1121
+TASKLET            0          0          0        290
+SCHED          27035      26983      26971      26746
+RCU             1678       1769       2178       2250
+
+
 1.3 IDE devices in /proc/ide
 ----------------------------
 
@@ -888,6 +907,7 @@ since the system first booted.  For a quick look, simply cat the file:
   processes 2915
   procs_running 1
   procs_blocked 0
+  softirq 183433 0 21755 12 39 1137 231 21459 2263
 
 The very first  "cpu" line aggregates the  numbers in all  of the other "cpuN"
 lines.  These numbers identify the amount of time the CPU has spent performing
@@ -923,6 +943,11 @@ CPUs.
 The   "procs_blocked" line gives  the  number of  processes currently blocked,
 waiting for I/O to complete.
 
+The "softirq" line gives counts of softirqs serviced since boot time, for each
+of the possible system softirqs. The first column is the total of all
+softirqs serviced; each subsequent column is the total for that particular
+softirq.
+
 
 1.9 Ext4 file system parameters
 ------------------------------
-- 
1.5.0.6

Re: [PATCH 1/3 v2] softirq: Introduce statistics for softirq

[KOSAKI Motohiro]

> Statistics for softirq doesn't exist.
> It will be helpful like statistics for interrupts.
> This patch introduces counting the number of softirq,
> which will be exported in /proc/softirqs.
> 
> When softirq handler consumes much CPU time,
> /proc/stat is like the following.
> 
> $ while :; do  cat /proc/stat | head -n1 ; sleep 10 ; done
> cpu  88 0 408 739665 583 28 2 0 0
> cpu  450 0 1090 740970 594 28 1294 0 0
>                               ^^^^
>                              softirq
> 
> In such a situation,
> /proc/softirqs shows us which softirq handler is invoked.
> We can see the increase rate of softirqs.
> 
> <before>
> $ cat /proc/softirqs
>                 CPU0       CPU1       CPU2       CPU3
> HI                 0          0          0          0
> TIMER         462850     462805     462782     462718
> NET_TX             0          0          0        365
> NET_RX          2472          2          2         40
> BLOCK              0          0        381       1164
> TASKLET            0          0          0        224
> SCHED         462654     462689     462698     462427
> RCU             3046       2423       3367       3173
> 
> <after>
> $ cat /proc/softirqs
>                 CPU0       CPU1       CPU2       CPU3
> HI                 0          0          0          0
> TIMER         463361     465077     465056     464991
> NET_TX            53          0          1        365
> NET_RX          3757          2          2         40
> BLOCK              0          0        398       1170
> TASKLET            0          0          0        224
> SCHED         463074     464318     464612     463330
> RCU             3505       2948       3947       3673

Maybe, this printing format don't works well on >100 CPUS.

Do you have any experience of trouble-shooting by this patch?
Actually, I don't understand this patch usuful working situation.

Honestly, I think HI and TASKLET are not beneficialness.
end user can't know what tasklet spent time.
TIMER also are not so much useful because it always have the same rate.

So.. guessing...

ultra band-width networking problem?



> 
> When CPU TIME of softirq is high,
> the rates of increase is the following.
>   TIMER  : 220/sec     : CPU1-3
>   NET_TX : 5/sec       : CPU0
>   NET_RX : 120/sec     : CPU0
>   SCHED  : 40-200/sec  : all CPU
>   RCU    : 45-58/sec   : all CPU
> 
> The rates of increase in an idle mode is the following.
>   TIMER  : 250/sec
>   SCHED  : 250/sec
>   RCU    : 2/sec
> 
> It seems many softirqs for receiving packets and rcu are invoked.
> This gives us help for checking system.
> 
> Signed-off-by: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
> Reviewed-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> ---
>  include/linux/kernel_stat.h |   12 ++++++++++++
>  kernel/softirq.c            |    1 +
>  2 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/kernel_stat.h b/include/linux/kernel_stat.h
> index 4a145ca..57c1643 100644
> --- a/include/linux/kernel_stat.h
> +++ b/include/linux/kernel_stat.h
> @@ -5,6 +5,7 @@
>  #include <linux/threads.h>
>  #include <linux/percpu.h>
>  #include <linux/cpumask.h>
> +#include <linux/interrupt.h>
>  #include <asm/irq.h>
>  #include <asm/cputime.h>
>  
> @@ -29,6 +30,7 @@ struct cpu_usage_stat {
>  struct kernel_stat {
>  	struct cpu_usage_stat	cpustat;
>  	unsigned int irqs[NR_IRQS];
> +	unsigned int softirqs[NR_SOFTIRQS];
>  };
>  
>  DECLARE_PER_CPU(struct kernel_stat, kstat);
> @@ -52,6 +54,16 @@ static inline unsigned int kstat_irqs_cpu(unsigned int irq, int cpu)
>         return kstat_cpu(cpu).irqs[irq];
>  }
>  
> +static inline void kstat_incr_softirqs_this_cpu(unsigned int irq)
> +{
> +	kstat_this_cpu.softirqs[irq]++;
> +}
> +
> +static inline unsigned int kstat_softirqs_cpu(unsigned int irq, int cpu)
> +{
> +       return kstat_cpu(cpu).softirqs[irq];
> +}
> +
>  /*
>   * Number of interrupts per specific IRQ source, since bootup
>   */
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index e7c69a7..088e179 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -208,6 +208,7 @@ restart:
>  	do {
>  		if (pending & 1) {
>  			int prev_count = preempt_count();
> +			kstat_incr_softirqs_this_cpu(h - softirq_vec);
>  
>  			h->action(h);

you should explain this patch don't decrease system performance.

Re: [PATCH 2/3 v2] proc: Export statistics for softirq to /proc

[Eric Dumazet]

Keika Kobayashi a écrit :
> Export statistics for softirq in /proc/softirqs and /proc/stat.
> 
> 1. /proc/softirqs
> Implement /proc/softirqs which shows the number of softirq
> for each CPU like /proc/interrupts.
> 
> 2. /proc/stat
> Add the "softirq" line to /proc/stat.
> This line shows the number of softirq for all cpu.
> The first column is the total of all softirqs and
> each subsequent column is the total for particular softirq.
> 
> Signed-off-by: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
> Reviewed-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> ---
>  fs/proc/Makefile   |    1 +
>  fs/proc/softirqs.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/proc/stat.c     |   17 +++++++++++++++
>  3 files changed, 75 insertions(+), 0 deletions(-)
>  create mode 100644 fs/proc/softirqs.c
> 
> diff --git a/fs/proc/Makefile b/fs/proc/Makefile
> index 63d9651..11a7b5c 100644
> --- a/fs/proc/Makefile
> +++ b/fs/proc/Makefile
> @@ -18,6 +18,7 @@ proc-y	+= meminfo.o
>  proc-y	+= stat.o
>  proc-y	+= uptime.o
>  proc-y	+= version.o
> +proc-y	+= softirqs.o
>  proc-$(CONFIG_PROC_SYSCTL)	+= proc_sysctl.o
>  proc-$(CONFIG_NET)		+= proc_net.o
>  proc-$(CONFIG_PROC_KCORE)	+= kcore.o
> diff --git a/fs/proc/softirqs.c b/fs/proc/softirqs.c
> new file mode 100644
> index 0000000..543f9d9
> --- /dev/null
> +++ b/fs/proc/softirqs.c
> @@ -0,0 +1,57 @@
> +#include <linux/init.h>
> +#include <linux/kernel_stat.h>
> +#include <linux/proc_fs.h>
> +#include <linux/seq_file.h>
> +
> +static const char *desc_array[] = {
> +	"HI",
> +	"TIMER",
> +	"NET_TX",
> +	"NET_RX",
> +	"BLOCK",
> +	"TASKLET",
> +	"SCHED",
> +#ifdef CONFIG_HIGH_RES_TIMERS
> +	"HRTIMER",
> +#endif
> +	"RCU"};
> +


You could use C99 initializers here

[HI_SOFTIRQ] = "HI",
[TIMER_SOFTIRQ] = "TIMER",
...

Re: [PATCH 1/3 v2] softirq: Introduce statistics for softirq

[Keika Kobayashi]

On Sat, 22 Nov 2008 14:36:34 +0900 (JST)
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> > Statistics for softirq doesn't exist.
> > It will be helpful like statistics for interrupts.
> > This patch introduces counting the number of softirq,
> > which will be exported in /proc/softirqs.
> > 
> > When softirq handler consumes much CPU time,
> > /proc/stat is like the following.
> > 
> > $ while :; do  cat /proc/stat | head -n1 ; sleep 10 ; done
> > cpu  88 0 408 739665 583 28 2 0 0
> > cpu  450 0 1090 740970 594 28 1294 0 0
> >                               ^^^^
> >                              softirq
> > 
> > In such a situation,
> > /proc/softirqs shows us which softirq handler is invoked.
> > We can see the increase rate of softirqs.
> > 
> > <before>
> > $ cat /proc/softirqs
> >                 CPU0       CPU1       CPU2       CPU3
> > HI                 0          0          0          0
> > TIMER         462850     462805     462782     462718
> > NET_TX             0          0          0        365
> > NET_RX          2472          2          2         40
> > BLOCK              0          0        381       1164
> > TASKLET            0          0          0        224
> > SCHED         462654     462689     462698     462427
> > RCU             3046       2423       3367       3173
> > 
> > <after>
> > $ cat /proc/softirqs
> >                 CPU0       CPU1       CPU2       CPU3
> > HI                 0          0          0          0
> > TIMER         463361     465077     465056     464991
> > NET_TX            53          0          1        365
> > NET_RX          3757          2          2         40
> > BLOCK              0          0        398       1170
> > TASKLET            0          0          0        224
> > SCHED         463074     464318     464612     463330
> > RCU             3505       2948       3947       3673
> 
> Maybe, this printing format don't works well on >100 CPUS.
> 
> Do you have any experience of trouble-shooting by this patch?
> Actually, I don't understand this patch usuful working situation.

When reported that CPU TIME of softirq went up,
we had to investigate the cause.
In that case, kind of network looked suspicious.
We added such a function to confirm.

Original patch shows ticks of softirqs.
But /proc/interrups have counts of interrupts.
So we changed from ticks to counts.
 
> Honestly, I think HI and TASKLET are not beneficialness.
> end user can't know what tasklet spent time.
> TIMER also are not so much useful because it always have the same rate.
> 
> So.. guessing...
> 
> ultra band-width networking problem?
> 
> 
<snip>
> 
> >  /*
> >   * Number of interrupts per specific IRQ source, since bootup
> >   */
> > diff --git a/kernel/softirq.c b/kernel/softirq.c
> > index e7c69a7..088e179 100644
> > --- a/kernel/softirq.c
> > +++ b/kernel/softirq.c
> > @@ -208,6 +208,7 @@ restart:
> >  	do {
> >  		if (pending & 1) {
> >  			int prev_count = preempt_count();
> > +			kstat_incr_softirqs_this_cpu(h - softirq_vec);
> >  
> >  			h->action(h);
> 
> you should explain this patch don't decrease system performance.

In this pach, only kstat_incr_softirqs_this_cpu() may 
influence system performance.
But this just increments one variable.
I don't think we can detect the decrease in the performance.

Re: [PATCH 2/3 v2] proc: Export statistics for softirq to /proc

[KOSAKI Motohiro]

nit

> @@ -49,6 +50,10 @@ static int show_stat(struct seq_file *p, void *v)
>  			sum += kstat_irqs_cpu(j, i);
>  
>  		sum += arch_irq_stat_cpu(i);
> +
> +		for (j = 0; j < NR_SOFTIRQS; j++)
> +			sum_softirq += kstat_softirqs_cpu(j, i);
> +
>  	}

You can calcurate per_irq_sum here.
Typically, # of possible cpu are very big.

So, I don't like unnecessary twrice looping.


>  	sum += arch_irq_stat();
>  
> @@ -111,6 +116,18 @@ static int show_stat(struct seq_file *p, void *v)
>  		nr_running(),
>  		nr_iowait());
>  
> +	seq_printf(p, "softirq %llu", (unsigned long long)sum_softirq);
> +
> +	for (i = 0; i < NR_SOFTIRQS; i++) {
> +		per_irq_sum = 0;
> +
> +		for_each_possible_cpu(j)
> +			per_irq_sum += kstat_softirqs_cpu(i, j);
> +
> +		seq_printf(p, " %u", per_irq_sum);
> +	}
> +	seq_printf(p, "\n");
> +
>  	return 0;
>  }

Re: [PATCH 3/3 v2] proc: Update document for /proc/softirqs and /proc/stat

[KOSAKI Motohiro]

> /proc/softirqs and /proc/stat have been changed.
> This patch updates proc.txt for these usage.
> 
> Signed-off-by: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
> Reviewed-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>

looks good to me.
	Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Re: [PATCH 1/3 v2] softirq: Introduce statistics for softirq

[KOSAKI Motohiro]

> On Sat, 22 Nov 2008 14:36:34 +0900 (JST)
> KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
> 
> > > Statistics for softirq doesn't exist.
> > > It will be helpful like statistics for interrupts.
> > > This patch introduces counting the number of softirq,
> > > which will be exported in /proc/softirqs.
> > > 
> > > When softirq handler consumes much CPU time,
> > > /proc/stat is like the following.
> > > 
> > > $ while :; do  cat /proc/stat | head -n1 ; sleep 10 ; done
> > > cpu  88 0 408 739665 583 28 2 0 0
> > > cpu  450 0 1090 740970 594 28 1294 0 0
> > >                               ^^^^
> > >                              softirq
> > > 
> > > In such a situation,
> > > /proc/softirqs shows us which softirq handler is invoked.
> > > We can see the increase rate of softirqs.
> > > 
> > > <before>
> > > $ cat /proc/softirqs
> > >                 CPU0       CPU1       CPU2       CPU3
> > > HI                 0          0          0          0
> > > TIMER         462850     462805     462782     462718
> > > NET_TX             0          0          0        365
> > > NET_RX          2472          2          2         40
> > > BLOCK              0          0        381       1164
> > > TASKLET            0          0          0        224
> > > SCHED         462654     462689     462698     462427
> > > RCU             3046       2423       3367       3173
> > > 
> > > <after>
> > > $ cat /proc/softirqs
> > >                 CPU0       CPU1       CPU2       CPU3
> > > HI                 0          0          0          0
> > > TIMER         463361     465077     465056     464991
> > > NET_TX            53          0          1        365
> > > NET_RX          3757          2          2         40
> > > BLOCK              0          0        398       1170
> > > TASKLET            0          0          0        224
> > > SCHED         463074     464318     464612     463330
> > > RCU             3505       2948       3947       3673
> > 
> > Maybe, this printing format don't works well on >100 CPUS.
> > 
> > Do you have any experience of trouble-shooting by this patch?
> > Actually, I don't understand this patch usuful working situation.
> 
> When reported that CPU TIME of softirq went up,
> we had to investigate the cause.
> In that case, kind of network looked suspicious.
> We added such a function to confirm.
> 
> Original patch shows ticks of softirqs.
> But /proc/interrups have counts of interrupts.
> So we changed from ticks to counts.

ok. it seems reasonable reason.
and this patch looks good to me.

	Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>


>  
> > Honestly, I think HI and TASKLET are not beneficialness.
> > end user can't know what tasklet spent time.
> > TIMER also are not so much useful because it always have the same rate.
> > 
> > So.. guessing...
> > 
> > ultra band-width networking problem?

Re: [PATCH 2/3 v2] proc: Export statistics for softirq to /proc

[Andrew Morton]

On Wed, 26 Nov 2008 19:10:28 +0900 (JST)
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> nit
> 
> > @@ -49,6 +50,10 @@ static int show_stat(struct seq_file *p, void *v)
> >  			sum += kstat_irqs_cpu(j, i);
> >  
> >  		sum += arch_irq_stat_cpu(i);
> > +
> > +		for (j = 0; j < NR_SOFTIRQS; j++)
> > +			sum_softirq += kstat_softirqs_cpu(j, i);
> > +
> >  	}
> 
> You can calcurate per_irq_sum here.
> Typically, # of possible cpu are very big.
> 
> So, I don't like unnecessary twrice looping.

I was about to send these patches to Linus, but it seems that this
optmisation hasn't been addressed?

Re: [PATCH 2/3 v2] proc: Export statistics for softirq to /proc

[kobayashi.kk]

2009/1/9 Andrew Morton <akpm@linux-foundation.org>:
> On Wed, 26 Nov 2008 19:10:28 +0900 (JST)
> KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
>
>> nit
>>
>> > @@ -49,6 +50,10 @@ static int show_stat(struct seq_file *p, void *v)
>> >                     sum += kstat_irqs_cpu(j, i);
>> >
>> >             sum += arch_irq_stat_cpu(i);
>> > +
>> > +           for (j = 0; j < NR_SOFTIRQS; j++)
>> > +                   sum_softirq += kstat_softirqs_cpu(j, i);
>> > +
>> >     }
>>
>> You can calcurate per_irq_sum here.
>> Typically, # of possible cpu are very big.
>>
>> So, I don't like unnecessary twrice looping.
>
> I was about to send these patches to Linus, but it seems that this
> optmisation hasn't been addressed?

Thanks for your notice, Andrew.
Of course, thanks for your advice, Kosaki-san.
I didn't check carefully about this point...

Now I am on a business trip.
So, I'll post the fix next week.

// Keika Kobayashi

Re: [PATCH 2/3 v2] proc: Export statistics for softirq to /proc

[KOSAKI Motohiro]

>>> You can calcurate per_irq_sum here.
>>> Typically, # of possible cpu are very big.
>>>
>>> So, I don't like unnecessary twrice looping.
>>
>> I was about to send these patches to Linus, but it seems that this
>> optmisation hasn't been addressed?
>
> Thanks for your notice, Andrew.
> Of course, thanks for your advice, Kosaki-san.
> I didn't check carefully about this point...
>
> Now I am on a business trip.
> So, I'll post the fix next week.

No problem.
I've made it three hour ago. I'll post soon.

Re: [PATCH 2/3 v2] proc: Export statistics for softirq to /proc

[KOSAKI Motohiro]

> >>> You can calcurate per_irq_sum here.
> >>> Typically, # of possible cpu are very big.
> >>>
> >>> So, I don't like unnecessary twrice looping.
> >>
> >> I was about to send these patches to Linus, but it seems that this
> >> optmisation hasn't been addressed?
> >
> > Thanks for your notice, Andrew.
> > Of course, thanks for your advice, Kosaki-san.
> > I didn't check carefully about this point...
> >
> > Now I am on a business trip.
> > So, I'll post the fix next week.
> 
> No problem.
> I've made it three hour ago. I'll post soon.

Done.


==
Subject: [PATCH] proc: remove redundunt for_each_possible_cpu() loop
Impact: cleanup

Almost distro set NR_CPUS very large number and at that time for_each_possible_cpu() is
a bit costly.

Therefore, To remove unnecessary twice loop is better.


Note. we can remove softirq's redundunt loop, but we can't remove irq's.
because NR_SOFTIRQS ~= 10, NR_IRQS ~= 4000 (in x86 case).


Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
Cc: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Eric Dumazet <dada1@cosmosbay.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 fs/proc/stat.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/proc/stat.c b/fs/proc/stat.c
index f95d73a..cb839e8 100644
--- a/fs/proc/stat.c
+++ b/fs/proc/stat.c
@@ -27,6 +27,7 @@ static int show_stat(struct seq_file *p, void *v)
 	cputime64_t guest;
 	u64 sum = 0;
 	u64 sum_softirq = 0;
+	unsigned int per_softirq_sums[NR_SOFTIRQS] = {0};
 	struct timespec boottime;
 	unsigned int per_irq_sum;
 
@@ -51,8 +52,12 @@ static int show_stat(struct seq_file *p, void *v)
 		}
 		sum += arch_irq_stat_cpu(i);
 
-		for (j = 0; j < NR_SOFTIRQS; j++)
-			sum_softirq += kstat_softirqs_cpu(j, i);
+		for (j = 0; j < NR_SOFTIRQS; j++) {
+			unsigned int softirq_stat = kstat_softirqs_cpu(j, i);
+
+			per_softirq_sums[j] += softirq_stat;
+			sum_softirq += softirq_stat;
+		}
 
 	}
 	sum += arch_irq_stat();
@@ -118,12 +123,7 @@ static int show_stat(struct seq_file *p, void *v)
 	seq_printf(p, "softirq %llu", (unsigned long long)sum_softirq);
 
 	for (i = 0; i < NR_SOFTIRQS; i++) {
-		per_irq_sum = 0;
-
-		for_each_possible_cpu(j)
-			per_irq_sum += kstat_softirqs_cpu(i, j);
-
-		seq_printf(p, " %u", per_irq_sum);
+		seq_printf(p, " %u", per_softirq_sums[i]);
 	}
 	seq_printf(p, "\n");
 

Re: [PATCH 2/3 v2] proc: Export statistics for softirq to /proc

[kobayashi.kk]

2009/1/12 KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>:
>> >>> You can calcurate per_irq_sum here.
>> >>> Typically, # of possible cpu are very big.
>> >>>
>> >>> So, I don't like unnecessary twrice looping.
>> >>
>> >> I was about to send these patches to Linus, but it seems that this
>> >> optmisation hasn't been addressed?
>> >
>> > Thanks for your notice, Andrew.
>> > Of course, thanks for your advice, Kosaki-san.
>> > I didn't check carefully about this point...
>> >
>> > Now I am on a business trip.
>> > So, I'll post the fix next week.
>>
>> No problem.
>> I've made it three hour ago. I'll post soon.
>
> Done.
>
>
> ==
> Subject: [PATCH] proc: remove redundunt for_each_possible_cpu() loop
> Impact: cleanup
>
> Almost distro set NR_CPUS very large number and at that time for_each_possible_cpu() is
> a bit costly.
>
> Therefore, To remove unnecessary twice loop is better.
>
>
> Note. we can remove softirq's redundunt loop, but we can't remove irq's.
> because NR_SOFTIRQS ~= 10, NR_IRQS ~= 4000 (in x86 case).
>
>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
> Cc: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Eric Dumazet <dada1@cosmosbay.com>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  fs/proc/stat.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/proc/stat.c b/fs/proc/stat.c
> index f95d73a..cb839e8 100644
> --- a/fs/proc/stat.c
> +++ b/fs/proc/stat.c
> @@ -27,6 +27,7 @@ static int show_stat(struct seq_file *p, void *v)
>        cputime64_t guest;
>        u64 sum = 0;
>        u64 sum_softirq = 0;
> +       unsigned int per_softirq_sums[NR_SOFTIRQS] = {0};
>        struct timespec boottime;
>        unsigned int per_irq_sum;
>
> @@ -51,8 +52,12 @@ static int show_stat(struct seq_file *p, void *v)
>                }
>                sum += arch_irq_stat_cpu(i);
>
> -               for (j = 0; j < NR_SOFTIRQS; j++)
> -                       sum_softirq += kstat_softirqs_cpu(j, i);
> +               for (j = 0; j < NR_SOFTIRQS; j++) {
> +                       unsigned int softirq_stat = kstat_softirqs_cpu(j, i);
> +
> +                       per_softirq_sums[j] += softirq_stat;
> +                       sum_softirq += softirq_stat;
> +               }
>
>        }
>        sum += arch_irq_stat();
> @@ -118,12 +123,7 @@ static int show_stat(struct seq_file *p, void *v)
>        seq_printf(p, "softirq %llu", (unsigned long long)sum_softirq);
>
>        for (i = 0; i < NR_SOFTIRQS; i++) {
> -               per_irq_sum = 0;
> -
> -               for_each_possible_cpu(j)
> -                       per_irq_sum += kstat_softirqs_cpu(i, j);
> -
> -               seq_printf(p, " %u", per_irq_sum);
> +               seq_printf(p, " %u", per_softirq_sums[i]);
>        }
>        seq_printf(p, "\n");
>

Thank you very much, Kosaki-san!
It looks good to me.

// Keika Kobayashi