per-cgroup /proc/stat statistics

include/linux/kernel_stat.h +4 -0

+#else
+#define kstat_cpu(cpu) per_cpu(kstat, cpu)
+#define kstat_this_cpu (&__get_cpu_var(kstat))
+#endif
 
 extern unsigned long long nr_context_switches(void);

(quoted diff) +0 -0

 } 

include/linux/kernel_stat.h struct kernel_stat { · line 50

Quoted 3 lines · Glauber Costa

/* Must have preemption disabled for this to be meaningful. */

include/linux/kernel_stat.h +1 -1

-#define kstat_this_cpu __get_cpu_var(kstat)
+#define kstat_this_cpu this_cpu_ptr(task_group_kstat(current))

This just lost you a debug check, the former would whinge when called without preemption, the new one wont. Its part of the this_cpu feature set to make debugging impossible.

Quoted 7 lines

include/linux/kernel_stat.h +4 -0

+#else
+#define kstat_cpu(cpu) per_cpu(kstat, cpu)
+#define kstat_this_cpu (&__get_cpu_var(kstat))
+#endif
 
  extern unsigned long long nr_context_switches(void);

include/linux/kernel_stat.h struct irq_desc; · line 66

Quoted 8 lines

include/linux/kernel_stat.h +2 -2

@@ -52,8 +62,8 @@ struct irq_desc;
  static inline void kstat_incr_irqs_this_cpu(unsigned int irq,
                                             struct irq_desc *desc)
  {
-       __this_cpu_inc(kstat.irqs[irq]);
-       __this_cpu_inc(kstat.irqs_sum);
+       kstat_this_cpu->irqs[irq]++;
+       kstat_this_cpu->irqs_sum++;

It might be worth looking at the asm output of that, I think you made it worse, but I'm not quite sure how smart gcc is, it might just figure out what you meant.

Quoted 1 line

(quoted diff) +0 -0

  }

Quoted 8 lines · Peter Zijlstra

include/linux/kernel_stat.h struct kernel_stat { · line 50

Quoted 3 lines · Glauber Costa

/* Must have preemption disabled for this to be meaningful. */

include/linux/kernel_stat.h +1 -1

-#define kstat_this_cpu __get_cpu_var(kstat)
+#define kstat_this_cpu this_cpu_ptr(task_group_kstat(current))

This just lost you a debug check, the former would whinge when called without preemption, the new one wont. Its part of the this_cpu feature set to make debugging impossible.

Why is that?

from percpu.h:
#define __get_cpu_var(var) (*this_cpu_ptr(&(var)))

So I don't get it.

include/linux/kernel_stat.h struct kernel_stat { · line 50

Quoted 3 lines · Glauber Costa

/* Must have preemption disabled for this to be meaningful. */

include/linux/kernel_stat.h +1 -1

-#define kstat_this_cpu __get_cpu_var(kstat)
+#define kstat_this_cpu this_cpu_ptr(task_group_kstat(current))

This just lost you a debug check, the former would whinge when called without preemption, the new one wont. Its part of the this_cpu feature set to make debugging impossible.

Quoted 7 lines

include/linux/kernel_stat.h +4 -0

+#else
+#define kstat_cpu(cpu) per_cpu(kstat, cpu)
+#define kstat_this_cpu (&__get_cpu_var(kstat))
+#endif
 
  extern unsigned long long nr_context_switches(void);

include/linux/kernel_stat.h struct irq_desc; · line 66

Quoted 8 lines

include/linux/kernel_stat.h +2 -2

@@ -52,8 +62,8 @@ struct irq_desc;
  static inline void kstat_incr_irqs_this_cpu(unsigned int irq,
                                             struct irq_desc *desc)
  {
-       __this_cpu_inc(kstat.irqs[irq]);
-       __this_cpu_inc(kstat.irqs_sum);
+       kstat_this_cpu->irqs[irq]++;
+       kstat_this_cpu->irqs_sum++;

It might be worth looking at the asm output of that, I think you made it worse, but I'm not quite sure how smart gcc is, it might just figure out what you meant.

Quoted 16 lines · Peter Zijlstra

include/linux/kernel_stat.h struct kernel_stat { · line 50

Quoted 3 lines · Glauber Costa

/* Must have preemption disabled for this to be meaningful. */

include/linux/kernel_stat.h +1 -1

-#define kstat_this_cpu __get_cpu_var(kstat)
+#define kstat_this_cpu this_cpu_ptr(task_group_kstat(current))

This just lost you a debug check, the former would whinge when called without preemption, the new one wont. Its part of the this_cpu feature set to make debugging impossible.

Quoted 7 lines

include/linux/kernel_stat.h +4 -0

+#else
+#define kstat_cpu(cpu) per_cpu(kstat, cpu)
+#define kstat_this_cpu (&__get_cpu_var(kstat))
+#endif
 
  extern unsigned long long nr_context_switches(void);

Quoted 12 lines

include/linux/kernel_stat.h struct irq_desc; · line 66

Quoted 8 lines

include/linux/kernel_stat.h +2 -2

@@ -52,8 +62,8 @@ struct irq_desc;
  static inline void kstat_incr_irqs_this_cpu(unsigned int irq,
                                             struct irq_desc *desc)
  {
-       __this_cpu_inc(kstat.irqs[irq]);
-       __this_cpu_inc(kstat.irqs_sum);
+       kstat_this_cpu->irqs[irq]++;
+       kstat_this_cpu->irqs_sum++;

It might be worth looking at the asm output of that, I think you made it worse, but I'm not quite sure how smart gcc is, it might just figure out what you meant.

I'd say leave it alone.
The biggest difference is that we don't have access to task_group(), or any of the fields in struct task_group. Because of that, we end up having to export a function to do the job of dealing with it.

Users inside sched.c won't have this problem. Outside of it, we'll add a call to some paths. True, mostly handle_irq paths, but I don't think that's what's going to kill us.

Now if we really really want to save it, we'd have to move struct task_group and its friends to a more visible location like a header...

Quoted 35 lines · Glauber Costa

Quoted 16 lines · Peter Zijlstra

include/linux/kernel_stat.h struct kernel_stat { · line 50

Quoted 3 lines · Glauber Costa

   /* Must have preemption disabled for this to be meaningful. */

include/linux/kernel_stat.h +1 -1

-#define kstat_this_cpu __get_cpu_var(kstat)
+#define kstat_this_cpu this_cpu_ptr(task_group_kstat(current))

This just lost you a debug check, the former would whinge when called without preemption, the new one wont. Its part of the this_cpu feature set to make debugging impossible.

Quoted 7 lines

include/linux/kernel_stat.h +4 -0

+#else
+#define kstat_cpu(cpu) per_cpu(kstat, cpu)
+#define kstat_this_cpu (&__get_cpu_var(kstat))
+#endif
 
   extern unsigned long long nr_context_switches(void);

Quoted 12 lines

include/linux/kernel_stat.h struct irq_desc; · line 66

Quoted 8 lines

include/linux/kernel_stat.h +2 -2

@@ -52,8 +62,8 @@ struct irq_desc;
   static inline void kstat_incr_irqs_this_cpu(unsigned int irq,
                                              struct irq_desc *desc)
   {
-       __this_cpu_inc(kstat.irqs[irq]);
-       __this_cpu_inc(kstat.irqs_sum);
+       kstat_this_cpu->irqs[irq]++;
+       kstat_this_cpu->irqs_sum++;

It might be worth looking at the asm output of that, I think you made it worse, but I'm not quite sure how smart gcc is, it might just figure out what you meant.

I'd say leave it alone.
The biggest difference is that we don't have access to task_group(), or any of the fields in struct task_group. Because of that, we end up having to export a function to do the job of dealing with it.

Quoted 6 lines

Users inside sched.c won't have this problem. Outside of it, we'll add a call to some paths. True, mostly handle_irq paths, but I don't think that's what's going to kill us.

Now if we really really want to save it, we'd have to move struct task_group and its friends to a more visible location like a header...

I'm not quite getting how task_group is relevant here.

The above will do something like:

mov gs:$per-cpu-offset-of-kstat, reg
inc reg + idx*8

Quoted 35 lines · Glauber Costa

Quoted 16 lines · Peter Zijlstra

include/linux/kernel_stat.h struct kernel_stat { · line 50

Quoted 3 lines · Glauber Costa

   /* Must have preemption disabled for this to be meaningful. */

include/linux/kernel_stat.h +1 -1

-#define kstat_this_cpu __get_cpu_var(kstat)
+#define kstat_this_cpu this_cpu_ptr(task_group_kstat(current))

This just lost you a debug check, the former would whinge when called without preemption, the new one wont. Its part of the this_cpu feature set to make debugging impossible.

Quoted 7 lines

include/linux/kernel_stat.h +4 -0

+#else
+#define kstat_cpu(cpu) per_cpu(kstat, cpu)
+#define kstat_this_cpu (&__get_cpu_var(kstat))
+#endif
 
   extern unsigned long long nr_context_switches(void);

Quoted 12 lines

include/linux/kernel_stat.h struct irq_desc; · line 66

Quoted 8 lines

include/linux/kernel_stat.h +2 -2

@@ -52,8 +62,8 @@ struct irq_desc;
   static inline void kstat_incr_irqs_this_cpu(unsigned int irq,
                                              struct irq_desc *desc)
   {
-       __this_cpu_inc(kstat.irqs[irq]);
-       __this_cpu_inc(kstat.irqs_sum);
+       kstat_this_cpu->irqs[irq]++;
+       kstat_this_cpu->irqs_sum++;

It might be worth looking at the asm output of that, I think you made it worse, but I'm not quite sure how smart gcc is, it might just figure out what you meant.

I'd say leave it alone.
The biggest difference is that we don't have access to task_group(), or any of the fields in struct task_group. Because of that, we end up having to export a function to do the job of dealing with it.

Quoted 6 lines

Users inside sched.c won't have this problem. Outside of it, we'll add a call to some paths. True, mostly handle_irq paths, but I don't think that's what's going to kill us.

Now if we really really want to save it, we'd have to move struct task_group and its friends to a more visible location like a header...

I'm not quite getting how task_group is relevant here.

The above will do something like:

mov gs:$per-cpu-offset-of-kstat, reg
inc reg + idx*8

whereas __this_cpu_inc() could end up like:

inc gs:$per-cpu-offset-of-kstat + idx*8

or whatnot. Now clearly gcc could be smart and optimize the temporary reg thing away in the earlier case, or it might not, I really don't know how smart that thing is.

kernel/sched.c +3 -0

@@ -623,6 +624,9 @@ static inline struct task_group *task_group(struct
 task_struct *p)
        struct task_group *tg;
        struct cgroup_subsys_state *css;
 
+       if (!p->mm)
+               return &root_task_group;
+
        css = task_subsys_state_check(p, cpu_cgroup_subsys_id,
                        lockdep_is_held(&p->pi_lock) ||

                        lockdep_is_held(&task_rq(p)->lock)); 

Quoted 5 lines · Glauber Costa

kernel/sched.c +0 -0

@@ -623,6 +624,9 @@ static inline struct task_group *task_group(struct
 task_struct *p)
         struct task_group *tg;
         struct cgroup_subsys_state *css;
 

Quoted 6 lines

kernel/sched.c +3 -0

+       if (!p->mm)
+               return&root_task_group;
+
         css = task_subsys_state_check(p, cpu_cgroup_subsys_id,
                         lockdep_is_held(&p->pi_lock) ||

                         lockdep_is_held(&task_rq(p)->lock));

Hmm, why is that? Aren't kthreads part of the cgroup muck just as much as normal tasks are?

Each cgroup has its own file, cpu.proc.stat that will display the exact same format as /proc/stat. Users that want to have access to a per-cgroup version of this information, can query it for that purpose.

Signed-off-by Glauber Costa <[email protected]>

+
 /*
 * Account user cpu time to a process.
 * @p: the process that the cpu time gets accounted to

kernel/sched.c +3 -3

@@ -3763,9 +3777,8 @@ unsigned long long thread_group_sched_runtime(struct task_struct *p)
  * @cputime_scaled: cputime scaled by cpu frequency
  */
  void account_user_time(struct task_struct *p, cputime_t cputime,
-                      cputime_t cputime_scaled)
+               cputime_t cputime_scaled)
  {
-       cputime64_t *cpustat = kstat_this_cpu->cpustat;
        cputime64_t tmp;
 
        /* Add user time to process. */

@@ -3775,10 +3788,11 @@ void account_user_time(struct task_struct *p, cputime_t cputime,
 
        /* Add user time to cpustat. */
        tmp = cputime_to_cputime64(cputime);
+
        if (TASK_NICE(p) > 0)
-               cpustat[NICE] = cputime64_add(cpustat[NICE], tmp);
+               task_cgroup_account_field(p, tmp, NICE);

       else
-               cpustat[USER] = cputime64_add(cpustat[USER], tmp);
+               task_cgroup_account_field(p, tmp, USER);

       cpuacct_update_stats(p, CPUACCT_STAT_USER, cputime); /* Account for user time used */

(quoted diff) +0 -0

@@ -3824,7 +3838,7 @@ static void account_guest_time(struct task_struct *p, cputime_t cputime,
  */

 static inline
 void __account_system_time(struct task_struct *p, cputime_t cputime,
-                       cputime_t cputime_scaled, cputime64_t *target_cputime64)
+                       cputime_t cputime_scaled, int index)
 {
       cputime64_t tmp = cputime_to_cputime64(cputime);

kernel/sched.c +4 -5

@@ -3834,7 +3848,7 @@ void __account_system_time(struct task_struct *p, cputime_t cputime,
        account_group_system_time(p, cputime);
 
        /* Add system time to cpustat. */
-       *target_cputime64 = cputime64_add(*target_cputime64, tmp);
+       task_cgroup_account_field(p, tmp, index);
        cpuacct_update_stats(p, CPUACCT_STAT_SYSTEM, cputime);
 
        /* Account for system time used */

@@ -3851,8 +3865,7 @@ void __account_system_time(struct task_struct *p, cputime_t cputime,
  void account_system_time(struct task_struct *p, int hardirq_offset,
                         cputime_t cputime, cputime_t cputime_scaled)
  {
-       cputime64_t *cpustat = kstat_this_cpu->cpustat;
-       cputime64_t *target_cputime64;
+       int index;
 
        if ((p->flags & PF_VCPU) && (irq_count() - hardirq_offset == 0)) {
                account_guest_time(p, cputime, cputime_scaled);

@@ -3860,13 +3873,13 @@ void account_system_time(struct task_struct *p, int hardirq_offset,
        }
 
        if (hardirq_count() - hardirq_offset)
-               target_cputime64 = &cpustat[IRQ];
+               index = IRQ;
        else if (in_serving_softirq())
-               target_cputime64 = &cpustat[SOFTIRQ];
+               index = SOFTIRQ;

       else
-               target_cputime64 = &cpustat[SYSTEM];
+               index = SYSTEM;

-       __account_system_time(p, cputime, cputime_scaled, target_cputime64);
+       __account_system_time(p, cputime, cputime_scaled, index); }

 /*

kernel/sched.c +11 -8

@@ -3941,27 +3954,29 @@ static __always_inline bool steal_account_process_tick(void)
  * softirq as those do not count in task exec_runtime any more.
  */
  static void irqtime_account_process_tick(struct task_struct *p, int user_tick,
-                                               struct rq *rq)
+                                        struct rq *rq)
  {
        cputime_t one_jiffy_scaled = cputime_to_scaled(cputime_one_jiffy);
        cputime64_t tmp = cputime_to_cputime64(cputime_one_jiffy);
-       cputime64_t *cpustat = kstat_this_cpu->cpustat;
+       struct task_group *tg;
 
        if (steal_account_process_tick())
                return;
 
+       tg = task_group(p);
+
        if (irqtime_account_hi_update()) {
-               cpustat[IRQ] = cputime64_add(cpustat[IRQ], tmp);
+               task_cgroup_account_field(p, tmp, IRQ);
        } else if (irqtime_account_si_update()) {
-               cpustat[SOFTIRQ] = cputime64_add(cpustat[SOFTIRQ], tmp);
+               task_cgroup_account_field(p, tmp, SOFTIRQ);
        } else if (this_cpu_ksoftirqd() == p) {
                /*
                 * ksoftirqd time do not get accounted in cpu_softirq_time.
                 * So, we have to handle it separately here.
                 * Also, p->stime needs to be updated for ksoftirqd.
                 */
-               __account_system_time(p, cputime_one_jiffy, one_jiffy_scaled,
-                                       &cpustat[SOFTIRQ]);
+               __account_system_time(p, cputime_one_jiffy,
+                                     one_jiffy_scaled, SOFTIRQ);
        } else if (user_tick) {
                account_user_time(p, cputime_one_jiffy, one_jiffy_scaled);
        } else if (p == rq->idle) {

@@ -3969,8 +3984,8 @@ static void irqtime_account_process_tick(struct task_struct *p, int user_tick,
        } else if (p->flags & PF_VCPU) { /* System time or guest time */
                account_guest_time(p, cputime_one_jiffy, one_jiffy_scaled);
        } else {
-               __account_system_time(p, cputime_one_jiffy, one_jiffy_scaled,
-                                       &cpustat[SYSTEM]);
+               __account_system_time(p, cputime_one_jiffy,
+                                     one_jiffy_scaled, SYSTEM);
        }
  }
 

@@ -8038,6 +8053,7 @@ void __init sched_init(void)
  {
        int i, j;
        unsigned long alloc_size = 0, ptr;
+       struct kernel_stat *kstat;
 
  #ifdef CONFIG_FAIR_GROUP_SCHED
        alloc_size += 2 * nr_cpu_ids * sizeof(void **);

kernel/sched.c +26 -0

+               kstat = per_cpu_ptr(root_task_group.cpustat, i);
+               kstat->cpustat[IDLE] = 0;
+               kstat->cpustat[IDLE_BASE] = 0;
+               kstat->cpustat[IOWAIT_BASE] = 0;
+               kstat->cpustat[IOWAIT] = 0;
+       }
+
  #endif /* CONFIG_CGROUP_SCHED */
 
        for_each_possible_cpu(i) {

@@ -8526,6 +8554,7 @@ static void free_sched_group(struct task_group *tg)
        free_fair_sched_group(tg);
        free_rt_sched_group(tg);
        autogroup_free(tg);
+       free_percpu(tg->cpustat);
        kfree(tg);
  }
 

@@ -8534,6 +8563,7 @@ struct task_group *sched_create_group(struct task_group *parent)
  {
        struct task_group *tg;
        unsigned long flags;
+       int i;
 
        tg = kzalloc(sizeof(*tg), GFP_KERNEL);
        if (!tg)

@@ -8545,6 +8575,19 @@ struct task_group *sched_create_group(struct task_group *parent)
        if (!alloc_rt_sched_group(tg, parent))
                goto err;
 
+       tg->cpustat = alloc_percpu(struct kernel_stat);
+       if (!tg->cpustat)
+               goto err;
+
+       for_each_possible_cpu(i) {
+               struct kernel_stat *kstat, *root_kstat;
+
+               kstat = per_cpu_ptr(tg->cpustat, i);
+               root_kstat = per_cpu_ptr(root_task_group.cpustat, i);
+               kstat->cpustat[IDLE_BASE]  = root_kstat->cpustat[IDLE];
+               kstat->cpustat[IOWAIT_BASE] = root_kstat->cpustat[IOWAIT];
+       }
+
        spin_lock_irqsave(&task_group_lock, flags);
        list_add_rcu(&tg->list, &task_groups);
 

@@ -9092,7 +9135,9 @@ int cpu_cgroup_proc_stat(struct cgroup *cgrp, struct cftype *cft, struct seq_fil
                system = cputime64_add(system, kstat->cpustat[SYSTEM]);
                idle = cputime64_add(idle, root_kstat->cpustat[IDLE]);
                idle = cputime64_add(idle, arch_idle_time(i));
+               idle = cputime64_sub(idle, kstat->cpustat[IDLE_BASE]);
                iowait = cputime64_add(iowait, root_kstat->cpustat[IOWAIT]);
+               iowait = cputime64_sub(iowait, kstat->cpustat[IOWAIT_BASE]);
                irq = cputime64_add(irq, kstat->cpustat[IRQ]);
                softirq = cputime64_add(softirq, kstat->cpustat[SOFTIRQ]);
                steal = cputime64_add(steal, kstat->cpustat[STEAL]);

@@ -9134,7 +9179,9 @@ int cpu_cgroup_proc_stat(struct cgroup *cgrp, struct cftype *cft, struct seq_fil
                system = kstat->cpustat[SYSTEM];
                idle = root_kstat->cpustat[IDLE];
                idle = cputime64_add(idle, arch_idle_time(i));
+               idle = cputime64_sub(idle, kstat->cpustat[IDLE_BASE]);
                iowait = root_kstat->cpustat[IOWAIT];
+               iowait = cputime64_sub(iowait, kstat->cpustat[IOWAIT_BASE]);
                irq = kstat->cpustat[IRQ];
                softirq = kstat->cpustat[SOFTIRQ];
                steal = kstat->cpustat[STEAL];

Quoted 6 lines · Glauber Costa

Each cgroup has its own file, cpu.proc.stat that will display the exact same format as /proc/stat. Users that want to have access to a per-cgroup version of this information, can query it for that purpose.

Signed-off-by Glauber Costa<[email protected]>

...

Quoted 7 lines

kernel/sched.c +7 -0

+static inline void task_cgroup_account_field(struct task_struct *p,
+                                            cputime64_t tmp, int index)
+{
+       struct kernel_stat *kstat;
+       struct task_group *tg = task_group(p);
+
+       do {

kernel/sched.c unsigned long long thread_group_sched_runtime(struct task_struct *p) · line 3771

Quoted 6 lines

kernel/sched.c +6 -0

+               kstat = this_cpu_ptr(tg->cpustat);
+               kstat->cpustat[index] = cputime64_add(kstat->cpustat[index],
+                                                     tmp);
+               tg = tg->parent;
+       } while (tg);
+}

What protects the walk (tg = tg->parent)? Could you please document it

Quoted 131 lines

kernel/sched.c +23 -20

+
  /*
  * Account user cpu time to a process.
  * @p: the process that the cpu time gets accounted to

@@ -3763,9 +3777,8 @@ unsigned long long thread_group_sched_runtime(struct task_struct *p)
  * @cputime_scaled: cputime scaled by cpu frequency
  */
  void account_user_time(struct task_struct *p, cputime_t cputime,
-                      cputime_t cputime_scaled)
+               cputime_t cputime_scaled)
  {
-       cputime64_t *cpustat = kstat_this_cpu->cpustat;
        cputime64_t tmp;
 
        /* Add user time to process. */

@@ -3775,10 +3788,11 @@ void account_user_time(struct task_struct *p, cputime_t cputime,
 
        /* Add user time to cpustat. */
        tmp = cputime_to_cputime64(cputime);
+
        if (TASK_NICE(p)>  0)
-               cpustat[NICE] = cputime64_add(cpustat[NICE], tmp);
+               task_cgroup_account_field(p, tmp, NICE);
        else
-               cpustat[USER] = cputime64_add(cpustat[USER], tmp);
+               task_cgroup_account_field(p, tmp, USER);
 
        cpuacct_update_stats(p, CPUACCT_STAT_USER, cputime);
        /* Account for user time used */

@@ -3824,7 +3838,7 @@ static void account_guest_time(struct task_struct *p, cputime_t cputime,
  */
  static inline
  void __account_system_time(struct task_struct *p, cputime_t cputime,
-                       cputime_t cputime_scaled, cputime64_t *target_cputime64)
+                       cputime_t cputime_scaled, int index)
  {
        cputime64_t tmp = cputime_to_cputime64(cputime);
 

@@ -3834,7 +3848,7 @@ void __account_system_time(struct task_struct *p, cputime_t cputime,
        account_group_system_time(p, cputime);
 
        /* Add system time to cpustat. */
-       *target_cputime64 = cputime64_add(*target_cputime64, tmp);
+       task_cgroup_account_field(p, tmp, index);
        cpuacct_update_stats(p, CPUACCT_STAT_SYSTEM, cputime);
 
        /* Account for system time used */

@@ -3851,8 +3865,7 @@ void __account_system_time(struct task_struct *p, cputime_t cputime,
  void account_system_time(struct task_struct *p, int hardirq_offset,
                         cputime_t cputime, cputime_t cputime_scaled)
  {
-       cputime64_t *cpustat = kstat_this_cpu->cpustat;
-       cputime64_t *target_cputime64;
+       int index;
 
        if ((p->flags&  PF_VCPU)&&  (irq_count() - hardirq_offset == 0)) {
                account_guest_time(p, cputime, cputime_scaled);

@@ -3860,13 +3873,13 @@ void account_system_time(struct task_struct *p, int hardirq_offset,
        }
 
        if (hardirq_count() - hardirq_offset)
-               target_cputime64 =&cpustat[IRQ];
+               index = IRQ;
        else if (in_serving_softirq())
-               target_cputime64 =&cpustat[SOFTIRQ];
+               index = SOFTIRQ;
        else
-               target_cputime64 =&cpustat[SYSTEM];
+               index = SYSTEM;
 
-       __account_system_time(p, cputime, cputime_scaled, target_cputime64);
+       __account_system_time(p, cputime, cputime_scaled, index);
  }
 
  /*

@@ -3941,27 +3954,29 @@ static __always_inline bool steal_account_process_tick(void)
  * softirq as those do not count in task exec_runtime any more.
  */
  static void irqtime_account_process_tick(struct task_struct *p, int user_tick,
-                                               struct rq *rq)
+                                        struct rq *rq)
  {
        cputime_t one_jiffy_scaled = cputime_to_scaled(cputime_one_jiffy);
        cputime64_t tmp = cputime_to_cputime64(cputime_one_jiffy);
-       cputime64_t *cpustat = kstat_this_cpu->cpustat;
+       struct task_group *tg;
 
        if (steal_account_process_tick())
                return;
 
+       tg = task_group(p);
+
        if (irqtime_account_hi_update()) {
-               cpustat[IRQ] = cputime64_add(cpustat[IRQ], tmp);
+               task_cgroup_account_field(p, tmp, IRQ);
        } else if (irqtime_account_si_update()) {
-               cpustat[SOFTIRQ] = cputime64_add(cpustat[SOFTIRQ], tmp);
+               task_cgroup_account_field(p, tmp, SOFTIRQ);
        } else if (this_cpu_ksoftirqd() == p) {
                /*
                 * ksoftirqd time do not get accounted in cpu_softirq_time.
                 * So, we have to handle it separately here.
                 * Also, p->stime needs to be updated for ksoftirqd.
                 */
-               __account_system_time(p, cputime_one_jiffy, one_jiffy_scaled,
-&cpustat[SOFTIRQ]);
+               __account_system_time(p, cputime_one_jiffy,
+                                     one_jiffy_scaled, SOFTIRQ);
        } else if (user_tick) {
                account_user_time(p, cputime_one_jiffy, one_jiffy_scaled);
        } else if (p == rq->idle) {

@@ -3969,8 +3984,8 @@ static void irqtime_account_process_tick(struct task_struct *p, int user_tick,
        } else if (p->flags&  PF_VCPU) { /* System time or guest time */
                account_guest_time(p, cputime_one_jiffy, one_jiffy_scaled);
        } else {
-               __account_system_time(p, cputime_one_jiffy, one_jiffy_scaled,
-&cpustat[SYSTEM]);
+               __account_system_time(p, cputime_one_jiffy,
+                                     one_jiffy_scaled, SYSTEM);
        }
  }
 

@@ -8038,6 +8053,7 @@ void __init sched_init(void)
  {
        int i, j;
        unsigned long alloc_size = 0, ptr;
+       struct kernel_stat *kstat;
 
  #ifdef CONFIG_FAIR_GROUP_SCHED
        alloc_size += 2 * nr_cpu_ids * sizeof(void **);

kernel/sched.c void __init sched_init(void) · line 8115

Quoted 8 lines

kernel/sched.c +5 -0

@@ -8092,6 +8108,18 @@ void __init sched_init(void)
        INIT_LIST_HEAD(&root_task_group.children);
        INIT_LIST_HEAD(&root_task_group.siblings);
        autogroup_init(&init_task);
+
+       root_task_group.cpustat = alloc_percpu(struct kernel_stat);
+       /* Failing that early an allocation means we're screwed anyway */
+       BUG_ON(!root_task_group.cpustat);
+       for_each_possible_cpu(i) {

for_each_possible_cpu might be an overkill, no?

Quoted 67 lines

kernel/sched.c +26 -0

+               kstat = per_cpu_ptr(root_task_group.cpustat, i);
+               kstat->cpustat[IDLE] = 0;
+               kstat->cpustat[IDLE_BASE] = 0;
+               kstat->cpustat[IOWAIT_BASE] = 0;
+               kstat->cpustat[IOWAIT] = 0;
+       }
+
  #endif /* CONFIG_CGROUP_SCHED */
 
        for_each_possible_cpu(i) {

@@ -8526,6 +8554,7 @@ static void free_sched_group(struct task_group *tg)
        free_fair_sched_group(tg);
        free_rt_sched_group(tg);
        autogroup_free(tg);
+       free_percpu(tg->cpustat);
        kfree(tg);
  }
 

@@ -8534,6 +8563,7 @@ struct task_group *sched_create_group(struct task_group *parent)
  {
        struct task_group *tg;
        unsigned long flags;
+       int i;
 
        tg = kzalloc(sizeof(*tg), GFP_KERNEL);
        if (!tg)

@@ -8545,6 +8575,19 @@ struct task_group *sched_create_group(struct task_group *parent)
        if (!alloc_rt_sched_group(tg, parent))
                goto err;
 
+       tg->cpustat = alloc_percpu(struct kernel_stat);
+       if (!tg->cpustat)
+               goto err;
+
+       for_each_possible_cpu(i) {
+               struct kernel_stat *kstat, *root_kstat;
+
+               kstat = per_cpu_ptr(tg->cpustat, i);
+               root_kstat = per_cpu_ptr(root_task_group.cpustat, i);
+               kstat->cpustat[IDLE_BASE]  = root_kstat->cpustat[IDLE];
+               kstat->cpustat[IOWAIT_BASE] = root_kstat->cpustat[IOWAIT];
+       }
+
        spin_lock_irqsave(&task_group_lock, flags);
        list_add_rcu(&tg->list,&task_groups);
 

@@ -9092,7 +9135,9 @@ int cpu_cgroup_proc_stat(struct cgroup *cgrp, struct cftype *cft, struct seq_fil
                system = cputime64_add(system, kstat->cpustat[SYSTEM]);
                idle = cputime64_add(idle, root_kstat->cpustat[IDLE]);
                idle = cputime64_add(idle, arch_idle_time(i));
+               idle = cputime64_sub(idle, kstat->cpustat[IDLE_BASE]);
                iowait = cputime64_add(iowait, root_kstat->cpustat[IOWAIT]);
+               iowait = cputime64_sub(iowait, kstat->cpustat[IOWAIT_BASE]);
                irq = cputime64_add(irq, kstat->cpustat[IRQ]);
                softirq = cputime64_add(softirq, kstat->cpustat[SOFTIRQ]);
                steal = cputime64_add(steal, kstat->cpustat[STEAL]);

@@ -9134,7 +9179,9 @@ int cpu_cgroup_proc_stat(struct cgroup *cgrp, struct cftype *cft, struct seq_fil
                system = kstat->cpustat[SYSTEM];
                idle = root_kstat->cpustat[IDLE];
                idle = cputime64_add(idle, arch_idle_time(i));
+               idle = cputime64_sub(idle, kstat->cpustat[IDLE_BASE]);
                iowait = root_kstat->cpustat[IOWAIT];
+               iowait = cputime64_sub(iowait, kstat->cpustat[IOWAIT_BASE]);
                irq = kstat->cpustat[IRQ];
                softirq = kstat->cpustat[SOFTIRQ];
                steal = kstat->cpustat[STEAL];

kernel/sched.c static struct cftype cpu_files[] = { · line 9257

Quoted 6 lines

kernel/sched.c +3 -0

@@ -9205,6 +9252,10 @@ static struct cftype cpu_files[] = {
                .write_u64 = cpu_rt_period_write_uint,
        },
  #endif
+       {
+               .name = "proc.stat",
+               .read_seq_string = cpu_cgroup_proc_stat,

Looks fine to me

Balbir Singh

Quoted 7 lines

Quoted 7 lines

kernel/sched.c +7 -0

+static inline void task_cgroup_account_field(struct task_struct *p,
+                                            cputime64_t tmp, int index)
+{
+       struct kernel_stat *kstat;
+       struct task_group *tg = task_group(p);
+
+       do {

Quoted 8 lines

kernel/sched.c unsigned long long thread_group_sched_runtime(struct task_struct *p) · line 3771

Quoted 6 lines

kernel/sched.c +6 -0

+               kstat = this_cpu_ptr(tg->cpustat);
+               kstat->cpustat[index] = cputime64_add(kstat->cpustat[index],
+                                                     tmp);
+               tg = tg->parent;
+       } while (tg);
+}

What protects the walk (tg = tg->parent)? Could you please document it

I think that the fact that the hierarchy only grows down, thus parent never changes (or am I wrong?)

And since we run all this with preempt disabled and with the runqueue locked, we should have no problems.

Do you agree?

Quoted 15 lines · Glauber Costa

Quoted 7 lines

Quoted 7 lines

kernel/sched.c +7 -0

+static inline void task_cgroup_account_field(struct task_struct *p,
+                                            cputime64_t tmp, int index)
+{
+       struct kernel_stat *kstat;
+       struct task_group *tg = task_group(p);
+
+       do {

Quoted 8 lines

kernel/sched.c unsigned long long thread_group_sched_runtime(struct task_struct *p) · line 3771

Quoted 6 lines

kernel/sched.c +6 -0

+               kstat = this_cpu_ptr(tg->cpustat);
+               kstat->cpustat[index] = cputime64_add(kstat->cpustat[index],
+                                                     tmp);
+               tg = tg->parent;
+       } while (tg);
+}

What protects the walk (tg = tg->parent)? Could you please document it

Quoted 7 lines

I think that the fact that the hierarchy only grows down, thus parent never changes (or am I wrong?)

And since we run all this with preempt disabled and with the runqueue locked, we should have no problems.

Do you agree?

Right, so the tg can't be destroyed unless its empty, us finding this task in it means its not empty, we require rq->lock or p->pi_lock to move the task.

However, afaict we don't actually have any of those locks.

That said, it should be sufficient to wrap the whole thing in rcu_read_lock(), accounting one tick funny because a cgroup move race isn't the end of the world and its really no different than moving the task a little later anyway.

task_group() should complain about this if you compile a kernel with CONFIG_PROVE_RCU.

Quoted 15 lines · Glauber Costa

Quoted 7 lines

Quoted 7 lines

kernel/sched.c +7 -0

+static inline void task_cgroup_account_field(struct task_struct *p,
+                                            cputime64_t tmp, int index)
+{
+       struct kernel_stat *kstat;
+       struct task_group *tg = task_group(p);
+
+       do {

Quoted 8 lines

kernel/sched.c unsigned long long thread_group_sched_runtime(struct task_struct *p) · line 3771

Quoted 6 lines

kernel/sched.c +6 -0

+               kstat = this_cpu_ptr(tg->cpustat);
+               kstat->cpustat[index] = cputime64_add(kstat->cpustat[index],
+                                                     tmp);
+               tg = tg->parent;
+       } while (tg);
+}

What protects the walk (tg = tg->parent)? Could you please document it

Quoted 7 lines

I think that the fact that the hierarchy only grows down, thus parent never changes (or am I wrong?)

And since we run all this with preempt disabled and with the runqueue locked, we should have no problems.

Do you agree?

Right, so the tg can't be destroyed unless its empty, us finding this task in it means its not empty, we require rq->lock or p->pi_lock to move the task.

However, afaict we don't actually have any of those locks.

That said, it should be sufficient to wrap the whole thing in rcu_read_lock(), accounting one tick funny because a cgroup move race isn't the end of the world and its really no different than moving the task a little later anyway.

task_group() should complain about this if you compile a kernel with CONFIG_PROVE_RCU.

kernel/sched.c +3 -0

+               tg = tg->parent;
+       } while (tg);
+} 

kernel/sched.c unsigned long long thread_group_sched_runtime(struct task_struct *p) · line 3768

Quoted 10 lines · Glauber Costa

kernel/sched.c +10 -0

+static inline void task_cgroup_account_field(struct task_struct *p,
+                                            cputime64_t tmp, int index)
+{
+       struct kernel_stat *kstat;
+       struct task_group *tg = task_group(p);
+
+       do {
+               kstat = this_cpu_ptr(tg->cpustat);
+               kstat->cpustat[index] = cputime64_add(kstat->cpustat[index],
+                                                     tmp);

So aside from the cputime64_t nonsense you could actually write that as:

		__this_cpu_add(tg->cpustat[index], tmp);

which should yield better asm I think, a quick grep seems to confirm cputime64_t is indeed a u64 all over so its fair to just ignore that.

+ }

kernel/sched.c void __init sched_init(void) · line 8120

Quoted 9 lines · Glauber Costa

kernel/sched.c +9 -0

+       root_task_group.cpustat = alloc_percpu(struct kernel_stat);
+       /* Failing that early an allocation means we're screwed anyway */
+       BUG_ON(!root_task_group.cpustat);
+       for_each_possible_cpu(i) {
+               kstat = per_cpu_ptr(root_task_group.cpustat, i);
+               kstat->cpustat[IDLE] = 0;
+               kstat->cpustat[IDLE_BASE] = 0;
+               kstat->cpustat[IOWAIT_BASE] = 0;
+               kstat->cpustat[IOWAIT] = 0;

so alloc_percpu() will actually zero your memory for you, which you rely on for all other fields as well, so why reset these ones?

Quoted 2 lines

+ }

kernel/fork.c static void posix_cpu_timers_init(struct task_struct *tsk) · line 1038

Quoted 5 lines · Glauber Costa

kernel/fork.c +1 -0

@@ -1039,6 +1035,8 @@ static void posix_cpu_timers_init(struct task_struct *tsk)
        INIT_LIST_HEAD(&tsk->cpu_timers[2]);
 }
 
+struct task_group *task_group(struct task_struct *p);

That doesn't appear to be actually used in this file..

Also, since there's already a for_each_possible_cpu() loop in that proc/stat function, would it yield some code improvement to make total_forks a cpu_usage_stat?

I guess the whole cputime64_t crap gets in the way of that being natural...

We could of course kill off the cputime64_t thing, its pretty pointless and its a u64 all over the board. I think Martin or Heiko created this stuff (although I might be wrong, my git tree doesn't go back that far).

Quoted 13 lines · On Wed, 28 Sep 2011 00:00:37 +0200 Peter Zijlstra <[email protected]>…

kernel/fork.c static void posix_cpu_timers_init(struct task_struct *tsk) · line 1038

Quoted 5 lines · Glauber Costa

kernel/fork.c +1 -0

@@ -1039,6 +1035,8 @@ static void posix_cpu_timers_init(struct task_struct *tsk)
        INIT_LIST_HEAD(&tsk->cpu_timers[2]);
 }
 
+struct task_group *task_group(struct task_struct *p);

That doesn't appear to be actually used in this file..

Also, since there's already a for_each_possible_cpu() loop in that proc/stat function, would it yield some code improvement to make total_forks a cpu_usage_stat?

Quoted 6 lines

I guess the whole cputime64_t crap gets in the way of that being natural...

We could of course kill off the cputime64_t thing, its pretty pointless and its a u64 all over the board. I think Martin or Heiko created this stuff (although I might be wrong, my git tree doesn't go back that far).

The reason to introduce cputime_t has been that different architecture needed differently sized integers for their respective representation of cputime. On x86-32 the number of ticks is recorded in a u32, on s390 we needed a u64 for the cpu timer values. cputime64_t is needed for cpustat and other sums of cputime that would overflow a cputime_t (in particular on x86-32 with the u32 cputime_t and the u64 cputime64_t).

Now we would convert everything to u64 but that would cause x86-32 to use 64-bit arithmetic for the tick counter. If that is acceptable I can't say.

Quoted 30 lines · Martin Schwidefsky

Quoted 13 lines · On Wed, 28 Sep 2011 00:00:37 +0200 Peter Zijlstra <[email protected]>…

kernel/fork.c static void posix_cpu_timers_init(struct task_struct *tsk) · line 1038

Quoted 5 lines · Glauber Costa

kernel/fork.c +1 -0

@@ -1039,6 +1035,8 @@ static void posix_cpu_timers_init(struct task_struct *tsk)
        INIT_LIST_HEAD(&tsk->cpu_timers[2]);
 }
 
+struct task_group *task_group(struct task_struct *p);

That doesn't appear to be actually used in this file..

Also, since there's already a for_each_possible_cpu() loop in that proc/stat function, would it yield some code improvement to make total_forks a cpu_usage_stat?

Quoted 6 lines

I guess the whole cputime64_t crap gets in the way of that being natural...

We could of course kill off the cputime64_t thing, its pretty pointless and its a u64 all over the board. I think Martin or Heiko created this stuff (although I might be wrong, my git tree doesn't go back that far).

The reason to introduce cputime_t has been that different architecture needed differently sized integers for their respective representation of cputime. On x86-32 the number of ticks is recorded in a u32, on s390 we needed a u64 for the cpu timer values. cputime64_t is needed for cpustat and other sums of cputime that would overflow a cputime_t (in particular on x86-32 with the u32 cputime_t and the u64 cputime64_t).

Quoted 3 lines

Now we would convert everything to u64 but that would cause x86-32 to use 64-bit arithmetic for the tick counter. If that is acceptable I can't say.

Right, so the main point was about cputime64_t, we might as well use a u64 for that throughout and ditch the silly cputime64_$op() accessors and write normal code.

But even if cputime_t differs between 32 and 64 bit machines, there is no reason actually use cputime_add(), C can do this.

The only reason to use things like cputime_add() is if you use a non simple type, but that doesn't seem to be the case.

So I think we can simplify the code lots by doing away with cputime64_t and all the cputime_*() functions. We can keep cputime_t, or we can use unsigned long, which I think will end up doing pretty much the same.

That is, am I missing some added value of all this cputime*() foo?

Quoted 1 line

That is, am I missing some added value of all this cputime*() foo?

C can do the math as long as the encoding of the cputime is simple enough. Can we demand that a cputime value needs to be an integral type ?

What I did when I wrote all that stuff is to define cputime_t as a struct that contains a single u64. That way I found all the places in the kernel that used a cputime and could convert the code accordingly.

My fear is that if the cputime_xxx operations are removed, code will sneak in again that just uses an unsigned long instead of a cputime_t. That would break any arch that requires something bigger than a u32 for its cputime.

Quoted 4 lines · Martin Schwidefsky

Quoted 1 line

That is, am I missing some added value of all this cputime*() foo?

C can do the math as long as the encoding of the cputime is simple enough. Can we demand that a cputime value needs to be an integral type ?

I'd like to think we can ;-)

Quoted 3 lines

What I did when I wrote all that stuff is to define cputime_t as a struct that contains a single u64. That way I found all the places in the kernel that used a cputime and could convert the code accordingly.

Indeed, that makes it a non-simple type and breaks all the C arith bits.

Quoted 4 lines

My fear is that if the cputime_xxx operations are removed, code will sneak in again that just uses an unsigned long instead of a cputime_t. That would break any arch that requires something bigger than a u32 for its cputime.

Which is only a problem for 32bit archs, of which s390 is the only one that matters, right? Hurm,. could we do something with sparse? Lots of people run sparse.

Quoted 20 lines · Peter Zijlstra

Quoted 4 lines · Martin Schwidefsky

Quoted 1 line

That is, am I missing some added value of all this cputime*() foo?

C can do the math as long as the encoding of the cputime is simple enough. Can we demand that a cputime value needs to be an integral type ?

I'd like to think we can ;-)

Quoted 3 lines

What I did when I wrote all that stuff is to define cputime_t as a struct that contains a single u64. That way I found all the places in the kernel that used a cputime and could convert the code accordingly.

Indeed, that makes it a non-simple type and breaks all the C arith bits.

Quoted 4 lines

My fear is that if the cputime_xxx operations are removed, code will sneak in again that just uses an unsigned long instead of a cputime_t. That would break any arch that requires something bigger than a u32 for its cputime.

Quoted 4 lines

Which is only a problem for 32bit archs, of which s390 is the only one that matters, right? Hurm,. could we do something with sparse? Lots of people run sparse.

Well, I think x86-32 is unlikely to ever really go away.

Quoted 21 lines · Glauber Costa

Quoted 20 lines · Peter Zijlstra

Quoted 4 lines · Martin Schwidefsky

Quoted 1 line

That is, am I missing some added value of all this cputime*() foo?

C can do the math as long as the encoding of the cputime is simple enough. Can we demand that a cputime value needs to be an integral type ?

I'd like to think we can ;-)

Quoted 3 lines

What I did when I wrote all that stuff is to define cputime_t as a struct that contains a single u64. That way I found all the places in the kernel that used a cputime and could convert the code accordingly.

Indeed, that makes it a non-simple type and breaks all the C arith bits.

Quoted 4 lines

My fear is that if the cputime_xxx operations are removed, code will sneak in again that just uses an unsigned long instead of a cputime_t. That would break any arch that requires something bigger than a u32 for its cputime.

Quoted 5 lines

Quoted 4 lines

Which is only a problem for 32bit archs, of which s390 is the only one that matters, right? Hurm,. could we do something with sparse? Lots of people run sparse.

Well, I think x86-32 is unlikely to ever really go away.

Sadly I'd agree with you, but that's not really the point, the only 32 bit arch that has !32 bit cputime_t is s390.

But yeah, death to ia32 (and everything else 32bit fwiw)!

So let me get this straight: The proposal here is really to get rid of all cputime_t , not only cputime64_t ?

Quoted 3 lines · Glauber Costa

So let me get this straight: The proposal here is really to get rid of all cputime_t , not only cputime64_t ?

For now cputime64_t would be sufficient, just wanted to extend the argument with Martin to see if we really need the cputime*() trickery at all.

Quoted 42 lines · On Wed, 28 Sep 2011 12:35:24 +0200 Peter Zijlstra<[email protected]>…

Quoted 30 lines · Martin Schwidefsky

Quoted 13 lines · On Wed, 28 Sep 2011 00:00:37 +0200 Peter Zijlstra<[email protected]>…

kernel/fork.c static void posix_cpu_timers_init(struct task_struct *tsk) · line 1038

Quoted 5 lines · Glauber Costa

kernel/fork.c +1 -0

@@ -1039,6 +1035,8 @@ static void posix_cpu_timers_init(struct task_struct *tsk)
         INIT_LIST_HEAD(&tsk->cpu_timers[2]);
  }
 
+struct task_group *task_group(struct task_struct *p);

That doesn't appear to be actually used in this file..

Also, since there's already a for_each_possible_cpu() loop in that proc/stat function, would it yield some code improvement to make total_forks a cpu_usage_stat?

Quoted 6 lines

I guess the whole cputime64_t crap gets in the way of that being natural...

We could of course kill off the cputime64_t thing, its pretty pointless and its a u64 all over the board. I think Martin or Heiko created this stuff (although I might be wrong, my git tree doesn't go back that far).

The reason to introduce cputime_t has been that different architecture needed differently sized integers for their respective representation of cputime. On x86-32 the number of ticks is recorded in a u32, on s390 we needed a u64 for the cpu timer values. cputime64_t is needed for cpustat and other sums of cputime that would overflow a cputime_t (in particular on x86-32 with the u32 cputime_t and the u64 cputime64_t).

Quoted 3 lines

Now we would convert everything to u64 but that would cause x86-32 to use 64-bit arithmetic for the tick counter. If that is acceptable I can't say.

Right, so the main point was about cputime64_t, we might as well use a u64 for that throughout and ditch the silly cputime64_$op() accessors and write normal code.

But even if cputime_t differs between 32 and 64 bit machines, there is no reason actually use cputime_add(), C can do this.

Quoted 8 lines

The only reason to use things like cputime_add() is if you use a non simple type, but that doesn't seem to be the case.

So I think we can simplify the code lots by doing away with cputime64_t and all the cputime_*() functions. We can keep cputime_t, or we can use unsigned long, which I think will end up doing pretty much the same.

That is, am I missing some added value of all this cputime*() foo?

C can do the math as long as the encoding of the cputime is simple enough. Can we demand that a cputime value needs to be an integral type ?

What I did when I wrote all that stuff is to define cputime_t as a struct that contains a single u64. That way I found all the places in the kernel that used a cputime and could convert the code accordingly.

My fear is that if the cputime_xxx operations are removed, code will sneak in again that just uses an unsigned long instead of a cputime_t. That would break any arch that requires something bigger than a u32 for its cputime. I really have to find my old debugging patch and see if we already have bit rot in regard to cputime_t.

Quoted 30 lines · Martin Schwidefsky

Quoted 13 lines · On Wed, 28 Sep 2011 00:00:37 +0200 Peter Zijlstra<[email protected]>…

kernel/fork.c static void posix_cpu_timers_init(struct task_struct *tsk) · line 1038

Quoted 5 lines · Glauber Costa

kernel/fork.c +1 -0

@@ -1039,6 +1035,8 @@ static void posix_cpu_timers_init(struct task_struct *tsk)
         INIT_LIST_HEAD(&tsk->cpu_timers[2]);
  }
 
+struct task_group *task_group(struct task_struct *p);

That doesn't appear to be actually used in this file..

Also, since there's already a for_each_possible_cpu() loop in that proc/stat function, would it yield some code improvement to make total_forks a cpu_usage_stat?

Quoted 6 lines

I guess the whole cputime64_t crap gets in the way of that being natural...

We could of course kill off the cputime64_t thing, its pretty pointless and its a u64 all over the board. I think Martin or Heiko created this stuff (although I might be wrong, my git tree doesn't go back that far).

The reason to introduce cputime_t has been that different architecture needed differently sized integers for their respective representation of cputime. On x86-32 the number of ticks is recorded in a u32, on s390 we needed a u64 for the cpu timer values. cputime64_t is needed for cpustat and other sums of cputime that would overflow a cputime_t (in particular on x86-32 with the u32 cputime_t and the u64 cputime64_t).

Quoted 3 lines

Now we would convert everything to u64 but that would cause x86-32 to use 64-bit arithmetic for the tick counter. If that is acceptable I can't say.

Right, so the main point was about cputime64_t, we might as well use a u64 for that throughout and ditch the silly cputime64_$op() accessors and write normal code.

But even if cputime_t differs between 32 and 64 bit machines, there is no reason actually use cputime_add(), C can do this.

The only reason to use things like cputime_add() is if you use a non simple type, but that doesn't seem to be the case.

So I think we can simplify the code lots by doing away with cputime64_t and all the cputime_*() functions. We can keep cputime_t, or we can use unsigned long, which I think will end up doing pretty much the same.

That is, am I missing some added value of all this cputime*() foo?

Quoted 13 lines · On Wed, 28 Sep 2011 00:00:37 +0200 Peter Zijlstra<[email protected]>…

kernel/fork.c static void posix_cpu_timers_init(struct task_struct *tsk) · line 1038

Quoted 5 lines · Glauber Costa

kernel/fork.c +1 -0

@@ -1039,6 +1035,8 @@ static void posix_cpu_timers_init(struct task_struct *tsk)
         INIT_LIST_HEAD(&tsk->cpu_timers[2]);
  }
 
+struct task_group *task_group(struct task_struct *p);

That doesn't appear to be actually used in this file..

Also, since there's already a for_each_possible_cpu() loop in that proc/stat function, would it yield some code improvement to make total_forks a cpu_usage_stat?

Quoted 6 lines

I guess the whole cputime64_t crap gets in the way of that being natural...

We could of course kill off the cputime64_t thing, its pretty pointless and its a u64 all over the board. I think Martin or Heiko created this stuff (although I might be wrong, my git tree doesn't go back that far).

The reason to introduce cputime_t has been that different architecture needed differently sized integers for their respective representation of cputime. On x86-32 the number of ticks is recorded in a u32, on s390 we needed a u64 for the cpu timer values. cputime64_t is needed for cpustat and other sums of cputime that would overflow a cputime_t (in particular on x86-32 with the u32 cputime_t and the u64 cputime64_t).

Now we would convert everything to u64 but that would cause x86-32 to use 64-bit arithmetic for the tick counter. If that is acceptable I can't say.

Hi,

Since I've sent already a RFC about it, I am sending now a RFD. If you eager for meaning, this can then be a "Request for Doctors", since Peter is likely to have a heart attack now.

From a conceptual pov this patch-set is a lot saner than the previous

So here's the deal:

* My main goal here was to demonstrate that we can avoid double accounting in a lot of places. So what I did was getting rid of the original and first kstat mechanism, and use only cgroups accounting for that. Since the parent is always updated, the original stats are the one for the root cgroup.

* I believe that all those cpu cgroups are confusing and should be unified. Not that we can simply get rid of it, but my goal here is to provide all the information they do, in cpu cgroup. If the set of tasks needed for accounting is not independent of the ones in cpu cgroup, we can avoid double accounting for that. I default cpuacct to n, but leave it to people that wants to use it alone.

* Well, I'm also doing what I was doing originally: Providing a per-cgroup version of the /proc/stat file.

Quoted 5 lines · Glauber Costa

Hi,

Since I've sent already a RFC about it, I am sending now a RFD. If you eager for meaning, this can then be a "Request for Doctors", since Peter is likely to have a heart attack now.

:-)

All we need is to ensure the case of cgroups enabled but not used isn't actually more expensive that what we have now, after that, if people create a 100 deep cgroup hierarchy they get what they asked.

From a conceptual pov this patch-set is a lot saner than the previous
one, doesn't duplicate nearly as much and actually tries to improve the code (although I suspect simply killing off cputime64_t as a whole will get us even more).

Quoted 7 lines

So here's the deal:

* My main goal here was to demonstrate that we can avoid double accounting in a lot of places. So what I did was getting rid of the original and first kstat mechanism, and use only cgroups accounting for that. Since the parent is always updated, the original stats are the one for the root cgroup.

Right, current patch-set won't compile for those who have CGROUP=n

kernels though, need to find something for that. Shouldn't be too hard though. It looks like you only need to provide static per-cpu storage and a custom version of task_cgroup_account_field().

Quoted 7 lines

* I believe that all those cpu cgroups are confusing and should be unified. Not that we can simply get rid of it, but my goal here is to provide all the information they do, in cpu cgroup. If the set of tasks needed for accounting is not independent of the ones in cpu cgroup, we can avoid double accounting for that. I default cpuacct to n, but leave it to people that wants to use it alone.

Amen! Ideally we place cpuacct on the deprecated list or somesuch..

Quoted 3 lines

* Well, I'm also doing what I was doing originally: Providing a per-cgroup version of the /proc/stat file.

Right, so how much sense does it make to keep calling it proc.stat?

Quoted 4 lines

From a conceptual pov this patch-set is a lot saner than the previous
one, doesn't duplicate nearly as much and actually tries to improve the code (although I suspect simply killing off cputime64_t as a whole will get us even more).

Are you actually planning to do that by yourself ?