linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] sched/cpuacct: fix charge percpu cpuusage
@ 2022-02-13 12:01 Chengming Zhou
  2022-02-14 10:09 ` Peter Zijlstra
  0 siblings, 1 reply; 5+ messages in thread
From: Chengming Zhou @ 2022-02-13 12:01 UTC (permalink / raw)
  To: tj, arbn, mingo, peterz; +Cc: linux-kernel, Chengming Zhou, Minye Zhu

The cpuacct_account_field() is always called by the current task
itself, so it's ok to use __this_cpu_add() to charge the tick time.

But cpuacct_charge() maybe called by update_curr() in load_balance()
on a random CPU, different from the CPU on which the task is running.
So __this_cpu_add() will charge that cputime to a random incorrect CPU.

Reported-by: Minye Zhu <zhuminye@bytedance.com>
Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
 kernel/sched/cpuacct.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/cpuacct.c b/kernel/sched/cpuacct.c
index 3d06c5e4220d..75fbc212cb71 100644
--- a/kernel/sched/cpuacct.c
+++ b/kernel/sched/cpuacct.c
@@ -335,11 +335,12 @@ static struct cftype files[] = {
 void cpuacct_charge(struct task_struct *tsk, u64 cputime)
 {
 	struct cpuacct *ca;
+	unsigned int cpu = task_cpu(tsk);
 
 	rcu_read_lock();
 
 	for (ca = task_ca(tsk); ca; ca = parent_ca(ca))
-		__this_cpu_add(*ca->cpuusage, cputime);
+		*per_cpu_ptr(ca->cpuusage, cpu) += cputime;
 
 	rcu_read_unlock();
 }
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] sched/cpuacct: fix charge percpu cpuusage
  2022-02-13 12:01 [PATCH] sched/cpuacct: fix charge percpu cpuusage Chengming Zhou
@ 2022-02-14 10:09 ` Peter Zijlstra
  2022-02-14 18:57   ` Tejun Heo
  2022-02-15 12:19   ` Chengming Zhou
  0 siblings, 2 replies; 5+ messages in thread
From: Peter Zijlstra @ 2022-02-14 10:09 UTC (permalink / raw)
  To: Chengming Zhou; +Cc: tj, arbn, mingo, linux-kernel, Minye Zhu

On Sun, Feb 13, 2022 at 08:01:18PM +0800, Chengming Zhou wrote:
> The cpuacct_account_field() is always called by the current task
> itself, so it's ok to use __this_cpu_add() to charge the tick time.
> 
> But cpuacct_charge() maybe called by update_curr() in load_balance()
> on a random CPU, different from the CPU on which the task is running.
> So __this_cpu_add() will charge that cputime to a random incorrect CPU.
> 
> Reported-by: Minye Zhu <zhuminye@bytedance.com>
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>

Can I get a Fixes: tag for this?

> ---
>  kernel/sched/cpuacct.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/cpuacct.c b/kernel/sched/cpuacct.c
> index 3d06c5e4220d..75fbc212cb71 100644
> --- a/kernel/sched/cpuacct.c
> +++ b/kernel/sched/cpuacct.c
> @@ -335,11 +335,12 @@ static struct cftype files[] = {
>  void cpuacct_charge(struct task_struct *tsk, u64 cputime)
>  {
>  	struct cpuacct *ca;
> +	unsigned int cpu = task_cpu(tsk);
>  
>  	rcu_read_lock();
>  
>  	for (ca = task_ca(tsk); ca; ca = parent_ca(ca))
> -		__this_cpu_add(*ca->cpuusage, cputime);
> +		*per_cpu_ptr(ca->cpuusage, cpu) += cputime;
>  
>  	rcu_read_unlock();
>  }

Also, while we there, what about this as an additional patch?

--- a/kernel/sched/cpuacct.c
+++ b/kernel/sched/cpuacct.c
@@ -334,15 +334,13 @@ static struct cftype files[] = {
  */
 void cpuacct_charge(struct task_struct *tsk, u64 cputime)
 {
-	struct cpuacct *ca;
 	unsigned int cpu = task_cpu(tsk);
+	struct cpuacct *ca;
 
-	rcu_read_lock();
+	lockdep_assert_rq_held(cpu_rq(cpu));
 
 	for (ca = task_ca(tsk); ca; ca = parent_ca(ca))
 		*per_cpu_ptr(ca->cpuusage, cpu) += cputime;
-
-	rcu_read_unlock();
 }
 
 /*

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] sched/cpuacct: fix charge percpu cpuusage
  2022-02-14 10:09 ` Peter Zijlstra
@ 2022-02-14 18:57   ` Tejun Heo
  2022-02-15 12:23     ` [Phishing Risk] [External] " Chengming Zhou
  2022-02-15 12:19   ` Chengming Zhou
  1 sibling, 1 reply; 5+ messages in thread
From: Tejun Heo @ 2022-02-14 18:57 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: Chengming Zhou, arbn, mingo, linux-kernel, Minye Zhu

On Mon, Feb 14, 2022 at 11:09:01AM +0100, Peter Zijlstra wrote:
> --- a/kernel/sched/cpuacct.c
> +++ b/kernel/sched/cpuacct.c
> @@ -334,15 +334,13 @@ static struct cftype files[] = {
>   */
>  void cpuacct_charge(struct task_struct *tsk, u64 cputime)
>  {
> -	struct cpuacct *ca;
>  	unsigned int cpu = task_cpu(tsk);
> +	struct cpuacct *ca;
>  
> -	rcu_read_lock();
> +	lockdep_assert_rq_held(cpu_rq(cpu));
>  
>  	for (ca = task_ca(tsk); ca; ca = parent_ca(ca))
>  		*per_cpu_ptr(ca->cpuusage, cpu) += cputime;
> -
> -	rcu_read_unlock();

And probably expand the same to cgroup_account_cputime[_field]() too.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [External] Re: [PATCH] sched/cpuacct: fix charge percpu cpuusage
  2022-02-14 10:09 ` Peter Zijlstra
  2022-02-14 18:57   ` Tejun Heo
@ 2022-02-15 12:19   ` Chengming Zhou
  1 sibling, 0 replies; 5+ messages in thread
From: Chengming Zhou @ 2022-02-15 12:19 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: tj, arbn, mingo, linux-kernel, Minye Zhu

On 2022/2/14 6:09 下午, Peter Zijlstra wrote:
> On Sun, Feb 13, 2022 at 08:01:18PM +0800, Chengming Zhou wrote:
>> The cpuacct_account_field() is always called by the current task
>> itself, so it's ok to use __this_cpu_add() to charge the tick time.
>>
>> But cpuacct_charge() maybe called by update_curr() in load_balance()
>> on a random CPU, different from the CPU on which the task is running.
>> So __this_cpu_add() will charge that cputime to a random incorrect CPU.
>>
>> Reported-by: Minye Zhu <zhuminye@bytedance.com>
>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> 
> Can I get a Fixes: tag for this?

Yes, Fixes: 73e6aafd9ea8 ("sched/cpuacct: Simplify the cpuacct code")
I will send a patch v2 to add it.

> 
>> ---
>>  kernel/sched/cpuacct.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/sched/cpuacct.c b/kernel/sched/cpuacct.c
>> index 3d06c5e4220d..75fbc212cb71 100644
>> --- a/kernel/sched/cpuacct.c
>> +++ b/kernel/sched/cpuacct.c
>> @@ -335,11 +335,12 @@ static struct cftype files[] = {
>>  void cpuacct_charge(struct task_struct *tsk, u64 cputime)
>>  {
>>  	struct cpuacct *ca;
>> +	unsigned int cpu = task_cpu(tsk);
>>  
>>  	rcu_read_lock();
>>  
>>  	for (ca = task_ca(tsk); ca; ca = parent_ca(ca))
>> -		__this_cpu_add(*ca->cpuusage, cputime);
>> +		*per_cpu_ptr(ca->cpuusage, cpu) += cputime;
>>  
>>  	rcu_read_unlock();
>>  }
> 
> Also, while we there, what about this as an additional patch?
> 
> --- a/kernel/sched/cpuacct.c
> +++ b/kernel/sched/cpuacct.c
> @@ -334,15 +334,13 @@ static struct cftype files[] = {
>   */
>  void cpuacct_charge(struct task_struct *tsk, u64 cputime)
>  {
> -	struct cpuacct *ca;
>  	unsigned int cpu = task_cpu(tsk);
> +	struct cpuacct *ca;
>  
> -	rcu_read_lock();
> +	lockdep_assert_rq_held(cpu_rq(cpu));
>  
>  	for (ca = task_ca(tsk); ca; ca = parent_ca(ca))
>  		*per_cpu_ptr(ca->cpuusage, cpu) += cputime;
> -
> -	rcu_read_unlock();
>  }
>  
>  /*

This is much better, I will send an additional patch to include this.

Thanks.


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [Phishing Risk] [External] Re: [PATCH] sched/cpuacct: fix charge percpu cpuusage
  2022-02-14 18:57   ` Tejun Heo
@ 2022-02-15 12:23     ` Chengming Zhou
  0 siblings, 0 replies; 5+ messages in thread
From: Chengming Zhou @ 2022-02-15 12:23 UTC (permalink / raw)
  To: Tejun Heo, Peter Zijlstra; +Cc: arbn, mingo, linux-kernel, Minye Zhu

On 2022/2/15 2:57 上午, Tejun Heo wrote:
> On Mon, Feb 14, 2022 at 11:09:01AM +0100, Peter Zijlstra wrote:
>> --- a/kernel/sched/cpuacct.c
>> +++ b/kernel/sched/cpuacct.c
>> @@ -334,15 +334,13 @@ static struct cftype files[] = {
>>   */
>>  void cpuacct_charge(struct task_struct *tsk, u64 cputime)
>>  {
>> -	struct cpuacct *ca;
>>  	unsigned int cpu = task_cpu(tsk);
>> +	struct cpuacct *ca;
>>  
>> -	rcu_read_lock();
>> +	lockdep_assert_rq_held(cpu_rq(cpu));
>>  
>>  	for (ca = task_ca(tsk); ca; ca = parent_ca(ca))
>>  		*per_cpu_ptr(ca->cpuusage, cpu) += cputime;
>> -
>> -	rcu_read_unlock();
> 
> And probably expand the same to cgroup_account_cputime[_field]() too.
> 
> Thanks.
> 

Good suggestion, will do.

Thanks.

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-02-15 12:23 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-13 12:01 [PATCH] sched/cpuacct: fix charge percpu cpuusage Chengming Zhou
2022-02-14 10:09 ` Peter Zijlstra
2022-02-14 18:57   ` Tejun Heo
2022-02-15 12:23     ` [Phishing Risk] [External] " Chengming Zhou
2022-02-15 12:19   ` Chengming Zhou

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).