linux-kernel - Re: [PATCH 04/33] cputime: Allow dynamic switch between tick/virtual based cputime accounting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFTL4hzpgDyHZ9qySLmouNBMgr6jqNd--w=1R4spVHELMt-z2A@mail.gmail.com>
Date:	Wed, 9 Jan 2013 00:22:33 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Alessio Igor Bogani <abogani@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Chris Metcalf <cmetcalf@...era.com>,
	Christoph Lameter <cl@...ux.com>,
	Geoff Levand <geoff@...radead.org>,
	Gilad Ben Yossef <gilad@...yossef.com>,
	Hakan Akkan <hakanakkan@...il.com>,
	Ingo Molnar <mingo@...nel.org>,
	Li Zhong <zhong@...ux.vnet.ibm.com>,
	Namhyung Kim <namhyung.kim@....com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Paul Gortmaker <paul.gortmaker@...driver.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 04/33] cputime: Allow dynamic switch between tick/virtual
 based cputime accounting

2013/1/8 Steven Rostedt <rostedt@...dmis.org>:
> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/