[PATCH 0/2] fix dying cpu compare race

[PATCH 0/2] fix dying cpu compare race

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

Ye Bin (2):
  cpu/hotplug: introduce 'num_dying_cpus' to get dying CPUs count
  lib/percpu_counter: fix dying cpu compare race

 include/linux/cpumask.h | 20 ++++++++++++++++----
 kernel/cpu.c            |  2 ++
 lib/percpu_counter.c    | 11 ++++++++++-
 3 files changed, 28 insertions(+), 5 deletions(-)

-- 
2.31.1

[PATCH 1/2] cpu/hotplug: introduce 'num_dying_cpus' to get dying CPUs count

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

Introduce '__num_dying_cpus' variable to cache the number of dying CPUs
in the core and just return the cached variable.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 include/linux/cpumask.h | 20 ++++++++++++++++----
 kernel/cpu.c            |  2 ++
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 2a61ddcf8321..8127fd598f51 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -135,6 +135,8 @@ extern struct cpumask __cpu_dying_mask;
 
 extern atomic_t __num_online_cpus;
 
+extern atomic_t __num_dying_cpus;
+
 extern cpumask_t cpus_booted_once_mask;
 
 static __always_inline void cpu_max_bits_warn(unsigned int cpu, unsigned int bits)
@@ -1018,10 +1020,14 @@ set_cpu_active(unsigned int cpu, bool active)
 static __always_inline void
 set_cpu_dying(unsigned int cpu, bool dying)
 {
-	if (dying)
-		cpumask_set_cpu(cpu, &__cpu_dying_mask);
-	else
-		cpumask_clear_cpu(cpu, &__cpu_dying_mask);
+	if (dying) {
+		if (!cpumask_test_and_set_cpu(cpu, &__cpu_dying_mask))
+			atomic_inc(&__num_dying_cpus);
+	}
+	else {
+		if (cpumask_test_and_clear_cpu(cpu, &__cpu_dying_mask))
+			atomic_dec(&__num_dying_cpus);
+	}
 }
 
 /**
@@ -1073,6 +1079,11 @@ static __always_inline unsigned int num_online_cpus(void)
 {
 	return arch_atomic_read(&__num_online_cpus);
 }
+
+static __always_inline unsigned int num_dying_cpus(void)
+{
+	return arch_atomic_read(&__num_dying_cpus);
+}
 #define num_possible_cpus()	cpumask_weight(cpu_possible_mask)
 #define num_present_cpus()	cpumask_weight(cpu_present_mask)
 #define num_active_cpus()	cpumask_weight(cpu_active_mask)
@@ -1108,6 +1119,7 @@ static __always_inline bool cpu_dying(unsigned int cpu)
 #define num_possible_cpus()	1U
 #define num_present_cpus()	1U
 #define num_active_cpus()	1U
+#define num_dying_cpus()	0U
 
 static __always_inline bool cpu_online(unsigned int cpu)
 {
diff --git a/kernel/cpu.c b/kernel/cpu.c
index f4a2c5845bcb..1c96c04cb259 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -2662,6 +2662,8 @@ EXPORT_SYMBOL(__cpu_dying_mask);
 atomic_t __num_online_cpus __read_mostly;
 EXPORT_SYMBOL(__num_online_cpus);
 
+atomic_t __num_dying_cpus __read_mostly;
+
 void init_cpu_present(const struct cpumask *src)
 {
 	cpumask_copy(&__cpu_present_mask, src);
-- 
2.31.1

[PATCH 2/2] lib/percpu_counter: fix dying cpu compare race

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

In commit 8b57b11cca88 ("pcpcntrs: fix dying cpu summation race") a race
condition between a cpu dying and percpu_counter_sum() iterating online CPUs
was identified.
Acctually, there's the same race condition between a cpu dying and
__percpu_counter_compare(). Here, use 'num_online_cpus()' for quick judgment.
But 'num_online_cpus()' will be decreased before call 'percpu_counter_cpu_dead()',
then maybe return incorrect result.
To solve above issue, also need to add dying CPUs count when do quick judgment
in __percpu_counter_compare().

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 lib/percpu_counter.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
index 5004463c4f9f..399840cb0012 100644
--- a/lib/percpu_counter.c
+++ b/lib/percpu_counter.c
@@ -227,6 +227,15 @@ static int percpu_counter_cpu_dead(unsigned int cpu)
 	return 0;
 }
 
+static __always_inline unsigned int num_count_cpus(void)
+{
+#ifdef CONFIG_HOTPLUG_CPU
+	return (num_online_cpus() + num_dying_cpus());
+#else
+	return num_online_cpus();
+#endif
+}
+
 /*
  * Compare counter against given value.
  * Return 1 if greater, 0 if equal and -1 if less
@@ -237,7 +246,7 @@ int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch)
 
 	count = percpu_counter_read(fbc);
 	/* Check to see if rough count will be sufficient for comparison */
-	if (abs(count - rhs) > (batch * num_online_cpus())) {
+	if (abs(count - rhs) > (batch * num_count_cpus())) {
 		if (count > rhs)
 			return 1;
 		else
-- 
2.31.1

Re: [PATCH 0/2] fix dying cpu compare race

[Yury Norov]

On Tue, Apr 04, 2023 at 09:42:04AM +0800, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>

Here should be a description of your series.
 
> Ye Bin (2):
>   cpu/hotplug: introduce 'num_dying_cpus' to get dying CPUs count
>   lib/percpu_counter: fix dying cpu compare race
> 
>  include/linux/cpumask.h | 20 ++++++++++++++++----
>  kernel/cpu.c            |  2 ++
>  lib/percpu_counter.c    | 11 ++++++++++-
>  3 files changed, 28 insertions(+), 5 deletions(-)
> 
> -- 
> 2.31.1

Re: [PATCH 1/2] cpu/hotplug: introduce 'num_dying_cpus' to get dying CPUs count

[Yury Norov]

On Tue, Apr 04, 2023 at 09:42:05AM +0800, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> Introduce '__num_dying_cpus' variable to cache the number of dying CPUs
> in the core and just return the cached variable.
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
>  include/linux/cpumask.h | 20 ++++++++++++++++----
>  kernel/cpu.c            |  2 ++
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 2a61ddcf8321..8127fd598f51 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -135,6 +135,8 @@ extern struct cpumask __cpu_dying_mask;
>  
>  extern atomic_t __num_online_cpus;
>  
> +extern atomic_t __num_dying_cpus;
> +
>  extern cpumask_t cpus_booted_once_mask;
>  
>  static __always_inline void cpu_max_bits_warn(unsigned int cpu, unsigned int bits)
> @@ -1018,10 +1020,14 @@ set_cpu_active(unsigned int cpu, bool active)
>  static __always_inline void
>  set_cpu_dying(unsigned int cpu, bool dying)
>  {
> -	if (dying)
> -		cpumask_set_cpu(cpu, &__cpu_dying_mask);
> -	else
> -		cpumask_clear_cpu(cpu, &__cpu_dying_mask);
> +	if (dying) {
> +		if (!cpumask_test_and_set_cpu(cpu, &__cpu_dying_mask))
> +			atomic_inc(&__num_dying_cpus);
> +	}
> +	else {
> +		if (cpumask_test_and_clear_cpu(cpu, &__cpu_dying_mask))
> +			atomic_dec(&__num_dying_cpus);
> +	}
>  }

Corresponding set_cpu_online() is implemented in C-file probably for a
reason. Are you sure that similar function for dying mask should
reside in a header? If so, can you share your reasoning?

Regardless, now that you added the identical function to
set_cpu_online, I think it's worth to make it a general approach:

void set_cpu_counted(unsigned int cpu, bool set,
                        struct cpumask *mask, atomic_t *cnt);

void __always_inline set_cpu_online(unsigned int cpu, bool online)
{
      set_cpu_counted(cpu, online, &__cpu_online_mask, &__num_online_cpus);  
}

void __always_inline set_cpu_dying(unsigned int cpu, bool dying)
{
      set_cpu_counted(cpu, dying, &__cpu_dying_mask, &__num_dying_cpus);  
}

Re: [PATCH 2/2] lib/percpu_counter: fix dying cpu compare race

[Yury Norov]

On Tue, Apr 04, 2023 at 09:42:06AM +0800, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> In commit 8b57b11cca88 ("pcpcntrs: fix dying cpu summation race") a race
> condition between a cpu dying and percpu_counter_sum() iterating online CPUs
> was identified.
> Acctually, there's the same race condition between a cpu dying and
> __percpu_counter_compare(). Here, use 'num_online_cpus()' for quick judgment.
> But 'num_online_cpus()' will be decreased before call 'percpu_counter_cpu_dead()',
> then maybe return incorrect result.
> To solve above issue, also need to add dying CPUs count when do quick judgment
> in __percpu_counter_compare().

Not sure I completely understood the race you are describing. All CPU
accounting is protected with percpu_counters_lock. Is it a real race
that you've faced, or hypothetical? If it's real, can you share stack
traces?
 
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
>  lib/percpu_counter.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
> index 5004463c4f9f..399840cb0012 100644
> --- a/lib/percpu_counter.c
> +++ b/lib/percpu_counter.c
> @@ -227,6 +227,15 @@ static int percpu_counter_cpu_dead(unsigned int cpu)
>  	return 0;
>  }
>  
> +static __always_inline unsigned int num_count_cpus(void)

This doesn't look like a good name. Maybe num_offline_cpus?

> +{
> +#ifdef CONFIG_HOTPLUG_CPU
> +	return (num_online_cpus() + num_dying_cpus());

               ^                                    ^ 
         'return' is not a function. Braces are not needed

Generally speaking, a sequence of atomic operations is not an atomic
operation, so the above doesn't look correct. I don't think that it
would be possible to implement raceless accounting based on 2 separate
counters.

Most probably, you'd have to use the same approach as in 8b57b11cca88:

        lock();
        for_each_cpu_or(cpu, cpu_online_mask, cpu_dying_mask)
                cnt++;
        unlock();

And if so, I'd suggest to implement cpumask_weight_or() for that.

> +#else
> +	return num_online_cpus();
> +#endif
> +}
> +
>  /*
>   * Compare counter against given value.
>   * Return 1 if greater, 0 if equal and -1 if less
> @@ -237,7 +246,7 @@ int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch)
>  
>  	count = percpu_counter_read(fbc);
>  	/* Check to see if rough count will be sufficient for comparison */
> -	if (abs(count - rhs) > (batch * num_online_cpus())) {
> +	if (abs(count - rhs) > (batch * num_count_cpus())) {
>  		if (count > rhs)
>  			return 1;
>  		else
> -- 
> 2.31.1

Re: [PATCH 2/2] lib/percpu_counter: fix dying cpu compare race

[Dave Chinner]

On Tue, Apr 04, 2023 at 09:42:06AM +0800, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> In commit 8b57b11cca88 ("pcpcntrs: fix dying cpu summation race") a race
> condition between a cpu dying and percpu_counter_sum() iterating online CPUs
> was identified.
> Acctually, there's the same race condition between a cpu dying and
> __percpu_counter_compare(). Here, use 'num_online_cpus()' for quick judgment.
> But 'num_online_cpus()' will be decreased before call 'percpu_counter_cpu_dead()',
> then maybe return incorrect result.
> To solve above issue, also need to add dying CPUs count when do quick judgment
> in __percpu_counter_compare().
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
>  lib/percpu_counter.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
> index 5004463c4f9f..399840cb0012 100644
> --- a/lib/percpu_counter.c
> +++ b/lib/percpu_counter.c
> @@ -227,6 +227,15 @@ static int percpu_counter_cpu_dead(unsigned int cpu)
>  	return 0;
>  }
>  
> +static __always_inline unsigned int num_count_cpus(void)
> +{
> +#ifdef CONFIG_HOTPLUG_CPU
> +	return (num_online_cpus() + num_dying_cpus());
> +#else
> +	return num_online_cpus();
> +#endif
> +}
> +
>  /*
>   * Compare counter against given value.
>   * Return 1 if greater, 0 if equal and -1 if less
> @@ -237,7 +246,7 @@ int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch)
>  
>  	count = percpu_counter_read(fbc);
>  	/* Check to see if rough count will be sufficient for comparison */
> -	if (abs(count - rhs) > (batch * num_online_cpus())) {
> +	if (abs(count - rhs) > (batch * num_count_cpus())) {

What problem is this actually fixing? You haven't explained how the
problem you are fixing manifests in the commit message or the cover
letter.

We generally don't care about the accuracy of the comparison here
because we've used percpu_counter_read() which is completely racy
against on-going updates. e.g. we can get preempted between
percpu_counter_read() and the check and so the value can be
completely wrong by the time we actually check it. Hence checking
online vs online+dying really doesn't fix any of the common race
conditions that occur here.

Even if we fall through to using percpu_counter_sum() for the
comparison value, that is still not accurate in the face of racing
updates to the counter because percpu_counter_sum only prevents
the percpu counter from being folded back into the global sum
while it is running. The comparison is still not precise or accurate.

IOWs, the result of this whole function is not guaranteed to be
precise or accurate; percpu counters cannot ever be relied on for
exact threshold detection unless there is some form of external
global counter synchronisation being used for those comparisons
(e.g. a global spinlock held around all the percpu_counter_add()
modifications as well as the __percpu_counter_compare() call).

That's always been the issue with unsynchronised percpu counters -
cpus dying just don't matter here because there are many other more
common race conditions that prevent accurate, race free comparison
of per-cpu counters.

Cheers,

Dave.
-- 
Dave Chinner
dchinner@redhat.com

Re: [PATCH 2/2] lib/percpu_counter: fix dying cpu compare race

[yebin (H)]

On 2023/4/4 14:01, Dave Chinner wrote:
> On Tue, Apr 04, 2023 at 09:42:06AM +0800, Ye Bin wrote:
>> From: Ye Bin <yebin10@huawei.com>
>>
>> In commit 8b57b11cca88 ("pcpcntrs: fix dying cpu summation race") a race
>> condition between a cpu dying and percpu_counter_sum() iterating online CPUs
>> was identified.
>> Acctually, there's the same race condition between a cpu dying and
>> __percpu_counter_compare(). Here, use 'num_online_cpus()' for quick judgment.
>> But 'num_online_cpus()' will be decreased before call 'percpu_counter_cpu_dead()',
>> then maybe return incorrect result.
>> To solve above issue, also need to add dying CPUs count when do quick judgment
>> in __percpu_counter_compare().
>>
>> Signed-off-by: Ye Bin <yebin10@huawei.com>
>> ---
>>   lib/percpu_counter.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
>> index 5004463c4f9f..399840cb0012 100644
>> --- a/lib/percpu_counter.c
>> +++ b/lib/percpu_counter.c
>> @@ -227,6 +227,15 @@ static int percpu_counter_cpu_dead(unsigned int cpu)
>>   	return 0;
>>   }
>>   
>> +static __always_inline unsigned int num_count_cpus(void)
>> +{
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +	return (num_online_cpus() + num_dying_cpus());
>> +#else
>> +	return num_online_cpus();
>> +#endif
>> +}
>> +
>>   /*
>>    * Compare counter against given value.
>>    * Return 1 if greater, 0 if equal and -1 if less
>> @@ -237,7 +246,7 @@ int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch)
>>   
>>   	count = percpu_counter_read(fbc);
>>   	/* Check to see if rough count will be sufficient for comparison */
>> -	if (abs(count - rhs) > (batch * num_online_cpus())) {
>> +	if (abs(count - rhs) > (batch * num_count_cpus())) {
> What problem is this actually fixing? You haven't explained how the
> problem you are fixing manifests in the commit message or the cover
> letter.
Before commit 5825bea05265("xfs: __percpu_counter_compare() inode count 
debug too expensive").
I got issue as follows when do cpu online/offline test:

smpboot: CPU 1 is now offline
XFS: Assertion failed: percpu_counter_compare(&mp->m_ifree, 0) >= 0, file: fs/xfs/xfs_trans.c, line: 622
------------[ cut here ]------------
kernel BUG at fs/xfs/xfs_message.c:110!
invalid opcode: 0000 [#1] SMP KASAN PTI
CPU: 3 PID: 25512 Comm: fsstress Not tainted 5.10.0-04288-gcb31bdc8c65d #8
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
RIP: 0010:assfail+0x77/0x8b fs/xfs/xfs_message.c:110
Code: 7f 10 84 d2 74 0c 48 c7 c7 0c dc e6 ab e8 e8 1e 52 fd 8a 1d 5e 04 5b 01 31 ff 89 de e8 e9 37 14 fd 84 db 74 07 e8 60 36 14 fd <0f> 0b e8 59 36 14 fd 0f 0b 5b 5d 41 5c 41 5d c3 cc cc cc cc e8 47
RSP: 0018:ffff88810a5df5c0 EFLAGS: 00010293
RAX: ffff88810f3a8000 RBX: 0000000000000201 RCX: ffffffffaa8bd7c0
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000001
RBP: 0000000000000000 R08: ffff88810f3a8000 R09: ffffed103edf71cd
R10: ffff8881f6fb8e67 R11: ffffed103edf71cc R12: ffffffffab0108c0
R13: ffffffffab010220 R14: ffffffffffffffff R15: 0000000000000000
FS:  00007f8536e16b80(0000) GS:ffff8881f6f80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00005617e1115f44 CR3: 000000015873a005 CR4: 0000000000370ee0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
  xfs_trans_unreserve_and_mod_sb+0x833/0xca0 fs/xfs/xfs_trans.c:622
  xlog_cil_commit+0x1169/0x29b0 fs/xfs/xfs_log_cil.c:1325
  __xfs_trans_commit+0x2c0/0xe20 fs/xfs/xfs_trans.c:889
  xfs_create_tmpfile+0x6a6/0x9a0 fs/xfs/xfs_inode.c:1320
  xfs_rename_alloc_whiteout fs/xfs/xfs_inode.c:3193 [inline]
  xfs_rename+0x58a/0x1e00 fs/xfs/xfs_inode.c:3245
  xfs_vn_rename+0x28e/0x410 fs/xfs/xfs_iops.c:436
  vfs_rename+0x10b5/0x1dd0 fs/namei.c:4329
  do_renameat2+0xa19/0xb10 fs/namei.c:4474
  __do_sys_renameat2 fs/namei.c:4512 [inline]
  __se_sys_renameat2 fs/namei.c:4509 [inline]
  __x64_sys_renameat2+0xe4/0x120 fs/namei.c:4509
  do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
  entry_SYSCALL_64_after_hwframe+0x61/0xc6
RIP: 0033:0x7f853623d91d

I can reproduce above issue by injecting kernel latency to invalidate 
the quick
judgment of “__percpu_counter_compare()”.
For quick judgment logic, the number of CPUs may have decreased before 
calling
percpu_counter_cpu_dead() when concurrent with CPU offline. That leads to
calculation errors. For example:
Assumption:
(1) batch = 32
(2) The final count is 2
(3) The number of CPUs is 4
If the number of percpu variables on each CPU is as follows when CPU3 is 
offline
cpu0 cpu1 cpu2 cpu3
   31     31     31    31
fbc->count = -122
So at this point, add a check to determine if fbc is greater than 0
abs(count - rhs) = -122
batch * num_ online_ cpus() = 32 * 3 = 96
That is: abs (count rhs)>batch * num_online_cpus() conditions met. The 
actual
value is 2, but the fact that count<0 returns -1 is the opposite.

> We generally don't care about the accuracy of the comparison here
> because we've used percpu_counter_read() which is completely racy
> against on-going updates. e.g. we can get preempted between
> percpu_counter_read() and the check and so the value can be
> completely wrong by the time we actually check it. Hence checking
> online vs online+dying really doesn't fix any of the common race
> conditions that occur here.
>
> Even if we fall through to using percpu_counter_sum() for the
> comparison value, that is still not accurate in the face of racing
> updates to the counter because percpu_counter_sum only prevents
> the percpu counter from being folded back into the global sum
> while it is running. The comparison is still not precise or accurate.
>
> IOWs, the result of this whole function is not guaranteed to be
> precise or accurate; percpu counters cannot ever be relied on for
> exact threshold detection unless there is some form of external
> global counter synchronisation being used for those comparisons
> (e.g. a global spinlock held around all the percpu_counter_add()
> modifications as well as the __percpu_counter_compare() call).
>
> That's always been the issue with unsynchronised percpu counters -
> cpus dying just don't matter here because there are many other more
> common race conditions that prevent accurate, race free comparison
> of per-cpu counters.
>
> Cheers,
>
> Dave.

Re: [PATCH 2/2] lib/percpu_counter: fix dying cpu compare race

[yebin (H)]

On 2023/4/4 10:50, Yury Norov wrote:
> On Tue, Apr 04, 2023 at 09:42:06AM +0800, Ye Bin wrote:
>> From: Ye Bin <yebin10@huawei.com>
>>
>> In commit 8b57b11cca88 ("pcpcntrs: fix dying cpu summation race") a race
>> condition between a cpu dying and percpu_counter_sum() iterating online CPUs
>> was identified.
>> Acctually, there's the same race condition between a cpu dying and
>> __percpu_counter_compare(). Here, use 'num_online_cpus()' for quick judgment.
>> But 'num_online_cpus()' will be decreased before call 'percpu_counter_cpu_dead()',
>> then maybe return incorrect result.
>> To solve above issue, also need to add dying CPUs count when do quick judgment
>> in __percpu_counter_compare().
> Not sure I completely understood the race you are describing. All CPU
> accounting is protected with percpu_counters_lock. Is it a real race
> that you've faced, or hypothetical? If it's real, can you share stack
> traces?
>   
>> Signed-off-by: Ye Bin <yebin10@huawei.com>
>> ---
>>   lib/percpu_counter.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
>> index 5004463c4f9f..399840cb0012 100644
>> --- a/lib/percpu_counter.c
>> +++ b/lib/percpu_counter.c
>> @@ -227,6 +227,15 @@ static int percpu_counter_cpu_dead(unsigned int cpu)
>>   	return 0;
>>   }
>>   
>> +static __always_inline unsigned int num_count_cpus(void)
> This doesn't look like a good name. Maybe num_offline_cpus?
>
>> +{
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +	return (num_online_cpus() + num_dying_cpus());
>                 ^                                    ^
>           'return' is not a function. Braces are not needed
>
> Generally speaking, a sequence of atomic operations is not an atomic
> operation, so the above doesn't look correct. I don't think that it
> would be possible to implement raceless accounting based on 2 separate
> counters.
Yes, there is indeed a concurrency issue with doing so here. But I saw 
that the process was first
set up dying_mask and then reduce the number of online CPUs. The total 
quantity maybe is larger
than the actual value and may fall back to a slow path.But this won't 
cause any problems.

>
> Most probably, you'd have to use the same approach as in 8b57b11cca88:
>
>          lock();
>          for_each_cpu_or(cpu, cpu_online_mask, cpu_dying_mask)
>                  cnt++;
>          unlock();
>
> And if so, I'd suggest to implement cpumask_weight_or() for that.
>
>> +#else
>> +	return num_online_cpus();
>> +#endif
>> +}
>> +
>>   /*
>>    * Compare counter against given value.
>>    * Return 1 if greater, 0 if equal and -1 if less
>> @@ -237,7 +246,7 @@ int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch)
>>   
>>   	count = percpu_counter_read(fbc);
>>   	/* Check to see if rough count will be sufficient for comparison */
>> -	if (abs(count - rhs) > (batch * num_online_cpus())) {
>> +	if (abs(count - rhs) > (batch * num_count_cpus())) {
>>   		if (count > rhs)
>>   			return 1;
>>   		else
>> -- 
>> 2.31.1
> .
>

Re: [PATCH 2/2] lib/percpu_counter: fix dying cpu compare race

[yebin (H)]

On 2023/4/4 10:50, Yury Norov wrote:
> On Tue, Apr 04, 2023 at 09:42:06AM +0800, Ye Bin wrote:
>> From: Ye Bin <yebin10@huawei.com>
>>
>> In commit 8b57b11cca88 ("pcpcntrs: fix dying cpu summation race") a race
>> condition between a cpu dying and percpu_counter_sum() iterating online CPUs
>> was identified.
>> Acctually, there's the same race condition between a cpu dying and
>> __percpu_counter_compare(). Here, use 'num_online_cpus()' for quick judgment.
>> But 'num_online_cpus()' will be decreased before call 'percpu_counter_cpu_dead()',
>> then maybe return incorrect result.
>> To solve above issue, also need to add dying CPUs count when do quick judgment
>> in __percpu_counter_compare().
> Not sure I completely understood the race you are describing. All CPU
> accounting is protected with percpu_counters_lock. Is it a real race
> that you've faced, or hypothetical? If it's real, can you share stack
> traces?
>   
>> Signed-off-by: Ye Bin <yebin10@huawei.com>
>> ---
>>   lib/percpu_counter.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
>> index 5004463c4f9f..399840cb0012 100644
>> --- a/lib/percpu_counter.c
>> +++ b/lib/percpu_counter.c
>> @@ -227,6 +227,15 @@ static int percpu_counter_cpu_dead(unsigned int cpu)
>>   	return 0;
>>   }
>>   
>> +static __always_inline unsigned int num_count_cpus(void)
> This doesn't look like a good name. Maybe num_offline_cpus?
num_count_cpus() include online CPUs and offline CPUs, use 
num_offline_cpus() doesn't seem appropriate either.
>
>> +{
>> +#ifdef CONFIG_HOTPLUG_CPU
Perhaps we need to add a memory barrier to setting and reading 
__num_dying_cpu.

+	return (num_online_cpus() + num_dying_cpus());

>                 ^                                    ^
>           'return' is not a function. Braces are not needed
>
> Generally speaking, a sequence of atomic operations is not an atomic
> operation, so the above doesn't look correct. I don't think that it
> would be possible to implement raceless accounting based on 2 separate
> counters.
>
> Most probably, you'd have to use the same approach as in 8b57b11cca88:
>
>          lock();
>          for_each_cpu_or(cpu, cpu_online_mask, cpu_dying_mask)
>                  cnt++;
>          unlock();
>
> And if so, I'd suggest to implement cpumask_weight_or() for that.
>
>> +#else
>> +	return num_online_cpus();
>> +#endif
>> +}
>> +
>>   /*
>>    * Compare counter against given value.
>>    * Return 1 if greater, 0 if equal and -1 if less
>> @@ -237,7 +246,7 @@ int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch)
>>   
>>   	count = percpu_counter_read(fbc);
>>   	/* Check to see if rough count will be sufficient for comparison */
>> -	if (abs(count - rhs) > (batch * num_online_cpus())) {
>> +	if (abs(count - rhs) > (batch * num_count_cpus())) {
>>   		if (count > rhs)
>>   			return 1;
>>   		else
>> -- 
>> 2.31.1
> .
>

Re: [PATCH 2/2] lib/percpu_counter: fix dying cpu compare race

[Yury Norov]

On Tue, Apr 04, 2023 at 02:54:25PM +0800, yebin (H) wrote:
> 
> 
> On 2023/4/4 10:50, Yury Norov wrote:
> > On Tue, Apr 04, 2023 at 09:42:06AM +0800, Ye Bin wrote:
> > > From: Ye Bin <yebin10@huawei.com>
> > > 
> > > In commit 8b57b11cca88 ("pcpcntrs: fix dying cpu summation race") a race
> > > condition between a cpu dying and percpu_counter_sum() iterating online CPUs
> > > was identified.
> > > Acctually, there's the same race condition between a cpu dying and
> > > __percpu_counter_compare(). Here, use 'num_online_cpus()' for quick judgment.
> > > But 'num_online_cpus()' will be decreased before call 'percpu_counter_cpu_dead()',
> > > then maybe return incorrect result.
> > > To solve above issue, also need to add dying CPUs count when do quick judgment
> > > in __percpu_counter_compare().
> > Not sure I completely understood the race you are describing. All CPU
> > accounting is protected with percpu_counters_lock. Is it a real race
> > that you've faced, or hypothetical? If it's real, can you share stack
> > traces?
> > > Signed-off-by: Ye Bin <yebin10@huawei.com>
> > > ---
> > >   lib/percpu_counter.c | 11 ++++++++++-
> > >   1 file changed, 10 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
> > > index 5004463c4f9f..399840cb0012 100644
> > > --- a/lib/percpu_counter.c
> > > +++ b/lib/percpu_counter.c
> > > @@ -227,6 +227,15 @@ static int percpu_counter_cpu_dead(unsigned int cpu)
> > >   	return 0;
> > >   }
> > > +static __always_inline unsigned int num_count_cpus(void)
> > This doesn't look like a good name. Maybe num_offline_cpus?
> > 
> > > +{
> > > +#ifdef CONFIG_HOTPLUG_CPU
> > > +	return (num_online_cpus() + num_dying_cpus());
> >                 ^                                    ^
> >           'return' is not a function. Braces are not needed
> > 
> > Generally speaking, a sequence of atomic operations is not an atomic
> > operation, so the above doesn't look correct. I don't think that it
> > would be possible to implement raceless accounting based on 2 separate
> > counters.
> Yes, there is indeed a concurrency issue with doing so here. But I saw that
> the process was first
> set up dying_mask and then reduce the number of online CPUs. The total
> quantity maybe is larger
> than the actual value and may fall back to a slow path.But this won't cause
> any problems.

This sounds like an implementation detail. If it will change in
future, your accounting will get broken.

If you think it's a consistent behavior and will be preserved in
future, then it must be properly commented in your patch.

Thanks,
Yury