[PATCH 0/2] softirq: redefine the type of kernel_stat.softirqs[] as unsigned long

[PATCH 0/2] softirq: redefine the type of kernel_stat.softirqs[] as unsigned long

[thunder.leizhen]

From: Zhen Lei <thunder.leizhen@huawei.com>

The type of member softirqs in structure kernel_stat is unsigned int, its
accumulated value can easily overflow. Changing to unsigned long can safely 
solve the problem on 64-bit processors.

 struct kernel_stat {
 	unsigned long irqs_sum;
-	unsigned int softirqs[NR_SOFTIRQS];
+	unsigned long softirqs[NR_SOFTIRQS];


Zhen Lei (2):
  softirq: fix integer overflow in function show_stat()
  softirq: redefine the type of kernel_stat.softirqs[] as unsigned long

 fs/proc/softirqs.c          | 2 +-
 fs/proc/stat.c              | 4 ++--
 include/linux/kernel_stat.h | 8 ++++----
 kernel/rcu/tree.h           | 2 +-
 kernel/rcu/tree_stall.h     | 6 +++---
 5 files changed, 11 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH 1/2] softirq: fix integer overflow in function show_stat()

[thunder.leizhen]

From: Zhen Lei <thunder.leizhen@huawei.com>

The statistics function of softirq is supported by commit aa0ce5bbc2db
("softirq: introduce statistics for softirq") in 2009. At that time,
64-bit processors should not have many cores and would not face
significant count overflow problems. Now it's common for a processor to
have hundreds of cores. Assume that there are 100 cores and 10
TIMER_SOFTIRQ are generated per second, then the 32-bit sum will be
overflowed after 50 days.

For example:
seq_put_decimal_ull(p, "softirq ", (unsigned long long)sum_softirq);
for (i = 0; i < NR_SOFTIRQS; i++)
	seq_put_decimal_ull(p, " ", per_softirq_sums[i]);

$ cat /proc/stat | tail -n 1
softirq 22929066124 9 2963267579 4128150 2618598635 546358555 0 \
629391610 1326100278 74637 1956244783

Here, the sum of per_softirq_sums[] is 10044164236 and is not equal to
22929066124. Because integers overflowed.

Therefore, change the type of local variable per_softirq_sums[] to u64.

Fixes: d3d64df21d3d ("proc: export statistics for softirq to /proc")
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 fs/proc/stat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/proc/stat.c b/fs/proc/stat.c
index da60956b2915645..84aac577a50cabb 100644
--- a/fs/proc/stat.c
+++ b/fs/proc/stat.c
@@ -86,7 +86,7 @@ static int show_stat(struct seq_file *p, void *v)
 	u64 guest, guest_nice;
 	u64 sum = 0;
 	u64 sum_softirq = 0;
-	unsigned int per_softirq_sums[NR_SOFTIRQS] = {0};
+	u64 per_softirq_sums[NR_SOFTIRQS] = {0};
 	struct timespec64 boottime;
 
 	user = nice = system = idle = iowait =
-- 
2.25.1

Re: [PATCH 1/2] softirq: fix integer overflow in function show_stat()

[Matthew Wilcox]

On Mon, Jul 24, 2023 at 09:22:23PM +0800, thunder.leizhen@huaweicloud.com wrote:
> From: Zhen Lei <thunder.leizhen@huawei.com>
> 
> The statistics function of softirq is supported by commit aa0ce5bbc2db
> ("softirq: introduce statistics for softirq") in 2009. At that time,
> 64-bit processors should not have many cores and would not face
> significant count overflow problems. Now it's common for a processor to
> have hundreds of cores. Assume that there are 100 cores and 10
> TIMER_SOFTIRQ are generated per second, then the 32-bit sum will be
> overflowed after 50 days.

50 days is long enough to take a snapshot.  You should always be using
difference between, not absolute values, and understand that they can
wrap.  We only tend to change the size of a counter when it can wrap
sufficiently quickly that we might miss a wrap (eg tens of seconds).

Re: [PATCH 1/2] softirq: fix integer overflow in function show_stat()

[Leizhen (ThunderTown)]

On 2023/7/24 21:50, Matthew Wilcox wrote:
> On Mon, Jul 24, 2023 at 09:22:23PM +0800, thunder.leizhen@huaweicloud.com wrote:
>> From: Zhen Lei <thunder.leizhen@huawei.com>
>>
>> The statistics function of softirq is supported by commit aa0ce5bbc2db
>> ("softirq: introduce statistics for softirq") in 2009. At that time,
>> 64-bit processors should not have many cores and would not face
>> significant count overflow problems. Now it's common for a processor to
>> have hundreds of cores. Assume that there are 100 cores and 10
>> TIMER_SOFTIRQ are generated per second, then the 32-bit sum will be
>> overflowed after 50 days.
> 
> 50 days is long enough to take a snapshot.  You should always be using
> difference between, not absolute values, and understand that they can
> wrap.  We only tend to change the size of a counter when it can wrap
> sufficiently quickly that we might miss a wrap (eg tens of seconds).

Yes, I think patch 2/2 can be dropped. I reduced the number of soft
interrupts generated in one second, and actually 100+ or 1000 is normal.
But I think patch 1/2 is necessary. The sum of the output scattered values
does not match the output sum. To solve this problem, we only need to
adjust the type of a local variable.


> 
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH 1/2] softirq: fix integer overflow in function show_stat()

[Leizhen (ThunderTown)]

On 2023/7/25 10:00, Leizhen (ThunderTown) wrote:
> 
> 
> On 2023/7/24 21:50, Matthew Wilcox wrote:
>> On Mon, Jul 24, 2023 at 09:22:23PM +0800, thunder.leizhen@huaweicloud.com wrote:
>>> From: Zhen Lei <thunder.leizhen@huawei.com>
>>>
>>> The statistics function of softirq is supported by commit aa0ce5bbc2db
>>> ("softirq: introduce statistics for softirq") in 2009. At that time,
>>> 64-bit processors should not have many cores and would not face
>>> significant count overflow problems. Now it's common for a processor to
>>> have hundreds of cores. Assume that there are 100 cores and 10
>>> TIMER_SOFTIRQ are generated per second, then the 32-bit sum will be
>>> overflowed after 50 days.
>>
>> 50 days is long enough to take a snapshot.  You should always be using
>> difference between, not absolute values, and understand that they can
>> wrap.  We only tend to change the size of a counter when it can wrap
>> sufficiently quickly that we might miss a wrap (eg tens of seconds).

Sometimes it can take a long time to view it again. For example, it is
possible to run a complete business test for hours or even days, and
then calculate the average.

> 
> Yes, I think patch 2/2 can be dropped. I reduced the number of soft
> interrupts generated in one second, and actually 100+ or 1000 is normal.
> But I think patch 1/2 is necessary. The sum of the output scattered values
> does not match the output sum. To solve this problem, we only need to
> adjust the type of a local variable.

However, it is important to consider that when the local variable is changed
to u64, the output string becomes longer. It is not clear if the user-mode
program parses it only by u32.

> 
> 
>>
>> .
>>
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH 1/2] softirq: fix integer overflow in function show_stat()

[Matthew Wilcox]

On Tue, Jul 25, 2023 at 05:09:05PM +0800, Leizhen (ThunderTown) wrote:
> On 2023/7/25 10:00, Leizhen (ThunderTown) wrote:
> > On 2023/7/24 21:50, Matthew Wilcox wrote:
> >> On Mon, Jul 24, 2023 at 09:22:23PM +0800, thunder.leizhen@huaweicloud.com wrote:
> >>> From: Zhen Lei <thunder.leizhen@huawei.com>
> >>>
> >>> The statistics function of softirq is supported by commit aa0ce5bbc2db
> >>> ("softirq: introduce statistics for softirq") in 2009. At that time,
> >>> 64-bit processors should not have many cores and would not face
> >>> significant count overflow problems. Now it's common for a processor to
> >>> have hundreds of cores. Assume that there are 100 cores and 10
> >>> TIMER_SOFTIRQ are generated per second, then the 32-bit sum will be
> >>> overflowed after 50 days.
> >>
> >> 50 days is long enough to take a snapshot.  You should always be using
> >> difference between, not absolute values, and understand that they can
> >> wrap.  We only tend to change the size of a counter when it can wrap
> >> sufficiently quickly that we might miss a wrap (eg tens of seconds).
> 
> Sometimes it can take a long time to view it again. For example, it is
> possible to run a complete business test for hours or even days, and
> then calculate the average.

I've been part of teams which have done such multi-hour tests.  That
isn't how monitoring was performed.  Instead snapshots were taken every
minute or even more frequently, because we wanted to know how these
counters were fluctuating during the test -- were there time periods
when the number of sortirqs spiked, or was it constant during the test?

> > Yes, I think patch 2/2 can be dropped. I reduced the number of soft
> > interrupts generated in one second, and actually 100+ or 1000 is normal.
> > But I think patch 1/2 is necessary. The sum of the output scattered values
> > does not match the output sum. To solve this problem, we only need to
> > adjust the type of a local variable.
> 
> However, it is important to consider that when the local variable is changed
> to u64, the output string becomes longer. It is not clear if the user-mode
> program parses it only by u32.

There's no need for the numbers to add up.  They won't anyway, because
summing them is racy , so they'll always be a little off.

Re: [PATCH 1/2] softirq: fix integer overflow in function show_stat()

[Leizhen (ThunderTown)]

On 2023/7/25 23:26, Matthew Wilcox wrote:
> On Tue, Jul 25, 2023 at 05:09:05PM +0800, Leizhen (ThunderTown) wrote:
>> On 2023/7/25 10:00, Leizhen (ThunderTown) wrote:
>>> On 2023/7/24 21:50, Matthew Wilcox wrote:
>>>> On Mon, Jul 24, 2023 at 09:22:23PM +0800, thunder.leizhen@huaweicloud.com wrote:
>>>>> From: Zhen Lei <thunder.leizhen@huawei.com>
>>>>>
>>>>> The statistics function of softirq is supported by commit aa0ce5bbc2db
>>>>> ("softirq: introduce statistics for softirq") in 2009. At that time,
>>>>> 64-bit processors should not have many cores and would not face
>>>>> significant count overflow problems. Now it's common for a processor to
>>>>> have hundreds of cores. Assume that there are 100 cores and 10
>>>>> TIMER_SOFTIRQ are generated per second, then the 32-bit sum will be
>>>>> overflowed after 50 days.
>>>>
>>>> 50 days is long enough to take a snapshot.  You should always be using
>>>> difference between, not absolute values, and understand that they can
>>>> wrap.  We only tend to change the size of a counter when it can wrap
>>>> sufficiently quickly that we might miss a wrap (eg tens of seconds).
>>
>> Sometimes it can take a long time to view it again. For example, it is
>> possible to run a complete business test for hours or even days, and
>> then calculate the average.
> 
> I've been part of teams which have done such multi-hour tests.  That
> isn't how monitoring was performed.  Instead snapshots were taken every
> minute or even more frequently, because we wanted to know how these
> counters were fluctuating during the test -- were there time periods
> when the number of sortirqs spiked, or was it constant during the test?
> 
>>> Yes, I think patch 2/2 can be dropped. I reduced the number of soft
>>> interrupts generated in one second, and actually 100+ or 1000 is normal.
>>> But I think patch 1/2 is necessary. The sum of the output scattered values
>>> does not match the output sum. To solve this problem, we only need to
>>> adjust the type of a local variable.
>>
>> However, it is important to consider that when the local variable is changed
>> to u64, the output string becomes longer. It is not clear if the user-mode
>> program parses it only by u32.
> 
> There's no need for the numbers to add up.  They won't anyway, because
> summing them is racy , so they'll always be a little off.

Okay, thanks for the reply. I got it. I just summed it up temporarily to
prove that integer overflow is possible, and there's no actual requirement.

> 
> .
> 

-- 
Regards,
  Zhen Lei

[PATCH 2/2] softirq: redefine the type of kernel_stat.softirqs[] as unsigned long

[thunder.leizhen]

From: Zhen Lei <thunder.leizhen@huawei.com>

As commit aa0ce5bbc2db ("softirq: introduce statistics for softirq")
mentioned, the number of one softirq can exceed 100 per second. At this
rate, the 32-bit sum will overflow after 1.5 years. This problem can be
avoided if the type of 'softirqs[]' is changed to u64, but the atomicity
of the counting operation cannot be guaranteed on 32-bit processors.
Changing to unsigned long can safely solve the problem on 64-bit
processors, and it is the same as the type of 'irqs_sum'.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 fs/proc/softirqs.c          | 2 +-
 fs/proc/stat.c              | 2 +-
 include/linux/kernel_stat.h | 8 ++++----
 kernel/rcu/tree.h           | 2 +-
 kernel/rcu/tree_stall.h     | 6 +++---
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/proc/softirqs.c b/fs/proc/softirqs.c
index f4616083faef3bb..985be6904d748ab 100644
--- a/fs/proc/softirqs.c
+++ b/fs/proc/softirqs.c
@@ -20,7 +20,7 @@ static int show_softirqs(struct seq_file *p, void *v)
 	for (i = 0; i < NR_SOFTIRQS; i++) {
 		seq_printf(p, "%12s:", softirq_to_name[i]);
 		for_each_possible_cpu(j)
-			seq_printf(p, " %10u", kstat_softirqs_cpu(i, j));
+			seq_printf(p, " %10lu", kstat_softirqs_cpu(i, j));
 		seq_putc(p, '\n');
 	}
 	return 0;
diff --git a/fs/proc/stat.c b/fs/proc/stat.c
index 84aac577a50cabb..7d40d98471d5ab2 100644
--- a/fs/proc/stat.c
+++ b/fs/proc/stat.c
@@ -116,7 +116,7 @@ static int show_stat(struct seq_file *p, void *v)
 		sum		+= arch_irq_stat_cpu(i);
 
 		for (j = 0; j < NR_SOFTIRQS; j++) {
-			unsigned int softirq_stat = kstat_softirqs_cpu(j, i);
+			unsigned long softirq_stat = kstat_softirqs_cpu(j, i);
 
 			per_softirq_sums[j] += softirq_stat;
 			sum_softirq += softirq_stat;
diff --git a/include/linux/kernel_stat.h b/include/linux/kernel_stat.h
index 9935f7ecbfb9e31..b6c5723a1149cd6 100644
--- a/include/linux/kernel_stat.h
+++ b/include/linux/kernel_stat.h
@@ -40,7 +40,7 @@ struct kernel_cpustat {
 
 struct kernel_stat {
 	unsigned long irqs_sum;
-	unsigned int softirqs[NR_SOFTIRQS];
+	unsigned long softirqs[NR_SOFTIRQS];
 };
 
 DECLARE_PER_CPU(struct kernel_stat, kstat);
@@ -63,15 +63,15 @@ static inline void kstat_incr_softirqs_this_cpu(unsigned int irq)
 	__this_cpu_inc(kstat.softirqs[irq]);
 }
 
-static inline unsigned int kstat_softirqs_cpu(unsigned int irq, int cpu)
+static inline unsigned long kstat_softirqs_cpu(unsigned int irq, int cpu)
 {
        return kstat_cpu(cpu).softirqs[irq];
 }
 
-static inline unsigned int kstat_cpu_softirqs_sum(int cpu)
+static inline unsigned long kstat_cpu_softirqs_sum(int cpu)
 {
 	int i;
-	unsigned int sum = 0;
+	unsigned long sum = 0;
 
 	for (i = 0; i < NR_SOFTIRQS; i++)
 		sum += kstat_softirqs_cpu(i, cpu);
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index 192536916f9a607..ce51640cd34f66b 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -268,7 +268,7 @@ struct rcu_data {
 	unsigned long rcuc_activity;
 
 	/* 7) Diagnostic data, including RCU CPU stall warnings. */
-	unsigned int softirq_snap;	/* Snapshot of softirq activity. */
+	unsigned long softirq_snap;	/* Snapshot of softirq activity. */
 	/* ->rcu_iw* fields protected by leaf rcu_node ->lock. */
 	struct irq_work rcu_iw;		/* Check for non-irq activity. */
 	bool rcu_iw_pending;		/* Is ->rcu_iw pending? */
diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h
index 0ade7f9dcaa18da..d5b22700abcc385 100644
--- a/kernel/rcu/tree_stall.h
+++ b/kernel/rcu/tree_stall.h
@@ -446,7 +446,7 @@ static void print_cpu_stat_info(int cpu)
 	rsr.cputime_system  = kcpustat_field(kcsp, CPUTIME_SYSTEM, cpu);
 
 	pr_err("\t         hardirqs   softirqs   csw/system\n");
-	pr_err("\t number: %8ld %10d %12lld\n",
+	pr_err("\t number: %8ld %10ld %12lld\n",
 		kstat_cpu_irqs_sum(cpu) - rsrp->nr_hardirqs,
 		kstat_cpu_softirqs_sum(cpu) - rsrp->nr_softirqs,
 		nr_context_switches_cpu(cpu) - rsrp->nr_csw);
@@ -498,7 +498,7 @@ static void print_cpu_stall_info(int cpu)
 	rcuc_starved = rcu_is_rcuc_kthread_starving(rdp, &j);
 	if (rcuc_starved)
 		sprintf(buf, " rcuc=%ld jiffies(starved)", j);
-	pr_err("\t%d-%c%c%c%c: (%lu %s) idle=%04x/%ld/%#lx softirq=%u/%u fqs=%ld%s%s\n",
+	pr_err("\t%d-%c%c%c%c: (%lu %s) idle=%04x/%ld/%#lx softirq=%lu/%lu fqs=%ld%s%s\n",
 	       cpu,
 	       "O."[!!cpu_online(cpu)],
 	       "o."[!!(rdp->grpmask & rdp->mynode->qsmaskinit)],
@@ -575,7 +575,7 @@ static void rcu_check_gp_kthread_expired_fqs_timer(void)
 		       data_race(rcu_state.gp_flags),
 		       gp_state_getname(RCU_GP_WAIT_FQS), RCU_GP_WAIT_FQS,
 		       data_race(READ_ONCE(gpk->__state)));
-		pr_err("\tPossible timer handling issue on cpu=%d timer-softirq=%u\n",
+		pr_err("\tPossible timer handling issue on cpu=%d timer-softirq=%lu\n",
 		       cpu, kstat_softirqs_cpu(TIMER_SOFTIRQ, cpu));
 	}
 }
-- 
2.25.1