* [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: [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
* 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
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).