linux-kernel - Re: [RFC PATCH] sched/cputime: drop local_irq_safe() in vtime_init

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]

Date:	Thu, 14 Apr 2016 15:18:30 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:	Ingo Molnar <mingo@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	linux-kernel@...r.kernel.org,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Rik van Riel <riel@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [RFC PATCH] sched/cputime: drop local_irq_safe() in
 vtime_init_idle()

On Thu, Apr 14, 2016 at 02:29:14PM +0200, Sebastian Andrzej Siewior wrote:
> A while ago vtime_init_idle() used to invoke sched_clock_cpu() and had
> write_seqlock_irqsave() because sched_clock_cpu() required to be called
> with interrupts off.
> This requirement is written before the body of the function and was
> introduced in 2010 via c676329abb2b ("sched_clock: Add local_clock() API
> and improve documentation"). This requirement has been dropped in 2013
> via ef08f0fff876 ("sched/clock: Remove local_irq_disable() from the clocks")
> but the body of the function still has the comment.
> 
> Now, vtime_init_idle() was converted from seqlock to seqcount via
> b7ce2277f087 ("sched/cputime: Convert vtime_seqlock to seqcount") and
> while doing so the IRQ-off region around sched_clock_cpu() was preserved
> while not strictly required (given the first part of this commit).
> A little later, sched_clock_cpu() was replaced with jiffies via
> ff9a9b4c4334 ("sched, time: Switch VIRT_CPU_ACCOUNTING_GEN to jiffy
> granularity").
> Based on this events I assume it is safe to drop the local_irq_safe()
> section.

I think I tried this but I got lockdep warnings because the other updaters
happen on IRQ.

In practive we can't be interrupted since the idle task hasn't even started.

Now we can probably get rid of the write_seqcount_stuff here because what
is initialized there is supposed to be visible by the idle task once it runs.

But I'm more worried about readers.

> 
> Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> Cc: Frederic Weisbecker <fweisbec@...il.com>
> Cc: Rik van Riel <riel@...hat.com>
> Cc: Thomas Gleixner <tglx@...utronix.de
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> ---
>  kernel/sched/cputime.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index 75f98c5498d5..816cd4ef5873 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -785,14 +785,10 @@ void arch_vtime_task_switch(struct task_struct *prev)
>  
>  void vtime_init_idle(struct task_struct *t, int cpu)
>  {
> -	unsigned long flags;
> -
> -	local_irq_save(flags);
>  	write_seqcount_begin(&t->vtime_seqcount);
>  	t->vtime_snap_whence = VTIME_SYS;
>  	t->vtime_snap = jiffies;
>  	write_seqcount_end(&t->vtime_seqcount);
> -	local_irq_restore(flags);
>  }
>  
>  cputime_t task_gtime(struct task_struct *t)
> -- 
> 2.8.0.rc3
>