Re: [PATCH 04/33] cputime: Allow dynamic switch between tick/virtualbased cputime accounting

[Frederic Weisbecker]

2013/1/8 Steven Rostedt <rostedt@xxxxxxxxxxx>:
> On Tue, 2013-01-08 at 03:08 +0100, Frederic Weisbecker wrote:
>
>> @@ -439,29 +443,13 @@ void account_idle_ticks(unsigned long ticks)
>>
>>       account_idle_time(jiffies_to_cputime(ticks));
>>  }
>> -
>>  #endif
>>
>> +
>
> Spurious newline.

There may be even more around on the other patches :)

>> +#ifdef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
>> +void task_cputime_adjusted(struct task_struct *p, cputime_t *ut, cputime_t *st)
>> +{
>> +     *ut = p->utime;
>> +     *st = p->stime;
>> +}
>
> Why not keep this out in the open like:
>
> static void __task_cputime_adjusted() {
>         ...
> }
>
> #ifdef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
> void task_cputime_adjusted(...)
> {
>         __task_cputime_adjusted(p, ut, st);
> }
>
>> +
>> +void thread_group_cputime_adjusted(struct task_struct *p, cputime_t *ut, cputime_t *st)
>> +{
>> +     struct task_cputime cputime;
>> +
>> +     thread_group_cputime(p, &cputime);
>> +
>> +     *ut = cputime.utime;
>> +     *st = cputime.stime;
>> +}
>> +#else /* !CONFIG_VIRT_CPU_ACCOUNTING_NATIVE */
>>
>>  #ifndef nsecs_to_cputime
>>  # define nsecs_to_cputime(__nsecs)   nsecs_to_jiffies(__nsecs)
>> @@ -548,6 +553,12 @@ static void cputime_adjust(struct task_cputime *curr,
>>  {
>>       cputime_t rtime, utime, total;
>>
>> +     if (vtime_accounting_enabled()) {
>> +             *ut = curr->utime;
>> +             *st = curr->stime;
>
> Then here, we can do:
>
>                 __task_cputime_adjusted(curr, ut, st);
>> +             return;

Note curr above is not a task but a struct task_cputime. But it could be:

__task_cputime_adjusted(curr->utime, curr->stime, ut, st);

But thinking more about it, I should just remove this:

+     if (vtime_accounting_enabled()) {
+             *ut = curr->utime;
+             *st = curr->stime;

It concerns CONFIG_VIRT_CPU_ACCOUNTING_GEN which is actually not
enough precise (it's jiffies based) and thus needs the same adjusting
performed on normal tick based cputime.

>
> Isn't suppose to do basically the same thing, when
> vtime_accounting_enabled() is set?
>
>> +     }
>> +
>>       utime = curr->utime;
>>       total = utime + curr->stime;
>>
>> @@ -601,7 +612,7 @@ void thread_group_cputime_adjusted(struct task_struct *p, cputime_t *ut, cputime
>>       thread_group_cputime(p, &cputime);
>>       cputime_adjust(&cputime, &p->signal->prev_cputime, ut, st);
>>  }
>> -#endif
>> +#endif /* !CONFIG_VIRT_CPU_ACCOUNTING_NATIVE */
>>
>>  #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
>>  static DEFINE_PER_CPU(long, last_jiffies) = INITIAL_JIFFIES;
>> @@ -643,6 +654,11 @@ void vtime_account_idle(struct task_struct *tsk)
>>       account_idle_time(delta_cpu);
>>  }
>>
>> +bool vtime_accounting_enabled(void)
>> +{
>> +     return context_tracking_active();
>> +}
>> +
>>  static int __cpuinit vtime_cpu_notify(struct notifier_block *self,
>>                                     unsigned long action, void *hcpu)
>>  {
>> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
>> index fb8e5e4..314b9ee 100644
>> --- a/kernel/time/tick-sched.c
>> +++ b/kernel/time/tick-sched.c
>> @@ -632,8 +632,11 @@ static void tick_nohz_restart_sched_tick(struct tick_sched *ts, ktime_t now)
>>
>>  static void tick_nohz_account_idle_ticks(struct tick_sched *ts)
>>  {
>> -#ifndef CONFIG_VIRT_CPU_ACCOUNTING
>> +#ifndef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
>>       unsigned long ticks;
>> +
>> +     if (vtime_accounting_enabled())
>> +             return;
>
> If this can be dynamically changed at runtime, wouldn't some of these
> accounting variables get corrupted? Like the last_jiffies per_cpu
> variable?

It can't yet be dynamically changed at runtime because full dynticks
CPUs are defined through boot parameters. But it's indeed something
we'll need to care about when we'll have a runtime interface.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/