Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1569747

Re: [PATCH 08/37] cputime: Convert task/group cputime to nsecs

Show all headers | View raw

On Sat, Jan 28, 2017 at 04:28:13PM +0100, Frederic Weisbecker wrote:
> On Sat, Jan 28, 2017 at 12:57:40PM +0100, Stanislaw Gruszka wrote:
> > On 32 bit architectures 64bit store/load is not atomic and if not
> > protected - 64bit variables can be mangled. I do not see any protection
> > (lock) between utime/stime store and load in the patch and seems that
> > {u/s}time store/load can be performed at the same time. Though problem
> > is very very improbable it still can happen at least theoretically when
> > lower and upper 32 bits are changed at the same time i.e. process
> > {u,s}time become near to multiple of 2**32 nsec (aprox: 4sec) and
> > 64bit {u,s}time is stored and loaded at the same time on different
> > cpus. As said this is very improbable situation, but eventually could
> > be possible on long lived processes.
> 
> "Improbable situation" doesn't appply to Linux. With millions (billion?)
> of machines using it, a rare issue in the core turns into likely to happen
> somewhere in the planet every second.
> 
> So it's definetly a race we want to consider. Note it goes beyond the scope
> of this patchset as the issue was already there before since cputime_t can already
> map to u64 on 32 bits systems upstream. But this patchset definetly extends
> the issue on all 32 bits configs.
> 
> kcpustat has the same issue upstream. It's is made of u64 on all configs.

I would like to add what are possible consequences if value will be
mangled. For sum_exec_runtime, utime and stime we could get wrong values
on cpu-clock related syscalls like clock_gettime() or clock_nanosleep()
and cpu-clock timers like timer_create(CLOCK_PROCESS_CPUTIME_ID) can be
triggered before or long after expected. For kcpustat this seems to be
wrong values read by procfs and 3 drivers (cpufreq, appldata, macintosh).

> > I considering fixing problem of sum_exec_runtime possible mangling
> > by using prev_sum_exec_runtime:
> > 
> > u64 read_sum_exec_runtime(struct task_struct *t)
> > {
> >        u64 ns, prev_ns;
> >  
> >        do {
> >                prev_ns = READ_ONCE(t->se.prev_sum_exec_runtime);
> >                ns = READ_ONCE(t->se.sum_exec_runtime);
> >        } while (ns < prev_ns || ns > (prev_ns + U32_MAX));
> >  
> >        return ns;
> > }
> > 
> > This should work based on fact that prev_sum_exec_runtime and
> > sum_exec_runtime are not modified and stored at the same time, so only
> > one of those variabled can be mangled. Though I need to think about 
> > correctnes of that a bit more.
> 
> I'm not sure that would be enough. READ_ONCE prevents from reordering by the
> compiler but not by the CPU. You'd need memory barriers between reads and
> writes of prev_ns and ns.

It will not be enough, this _suppose_ to work based on that sum_exec_runtime
and prev_sum_exec_runtime are not written at the same time. i.e. only
one variable can be mangled as another one sits already in the memory.
However "not written at the same time" is weak part of reasoning. Even
if those variables are stored at different part of code (sum_exec_runtime
on update_curr() and prev_sum_exec_runtime on set_next_entity()) we can
not assume store of one variable is finished before another one starts.

>    WRITE ns                READ prev_ns
>    smp_wmb()               smp_rmb()
>    WRITE prev_ns           READ ns
>    smp_wmb()               smp_rmb()
>
> It seems to be the only way to make sure that at least one of the reads
> (prev_ns or ns) is correct.

I think you have right, but seems on much code paths we have scenario:

	WRITE ns		READ prev_ns
	smp_wmb()		smp_rmb()
	WRITE prev_ns		READ ns

and we have already smp_wmb() after write of sum_exec_runtime on
update_min_vruntime().

Stanislaw

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 00/37] cputime: Convert core use of cputime_t to nsecs v3 Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 12/37] binfmt: Convert obsolete cputime type to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 16/37] signal: Convert obsolete cputime type to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 26/37] cputime: Complete nsec conversion of tick based accounting Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 36/37] cputime: Remove asm generic headers Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 09/37] alpha: Convert obsolete cputime_t to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 30/37] ia64: Convert vtime to use nsec units directly Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 04/37] cputime: Convert kcpustat to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 08/37] cputime: Convert task/group cputime to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
    Re: [PATCH 08/37] cputime: Convert task/group cputime to nsecs Stanislaw Gruszka <sgruszka@redhat.com> - 2017-01-28 13:20 +0100
      Re: [PATCH 08/37] cputime: Convert task/group cputime to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-28 16:40 +0100
        Re: [PATCH 08/37] cputime: Convert task/group cputime to nsecs Stanislaw Gruszka <sgruszka@redhat.com> - 2017-01-30 15:00 +0100
          Re: [PATCH 08/37] cputime: Convert task/group cputime to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-30 16:40 +0100
  [PATCH 33/37] powerpc: Remove unused cputime definitions Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 07/37] cputime: Special API to return old-typed cputime Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 28/37] cputime: Remove jiffies based cputime Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 03/37] sched: Remove unused INIT_CPUTIME macro Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 10/37] x86: Convert obsolete cputime type to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 20/37] itimer: Convert internal cputime_t units to nsec Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 13/37] acct: Convert obsolete cputime type to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 05/37] macintosh/rack-meter: Remove cputime_t internal use Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 24/37] cputime: Push time to account_idle_time() in nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 34/37] s390: Remove unused cputime definitions Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 01/37] jiffies: Reuse TICK_NSEC instead of NSEC_PER_JIFFY Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 29/37] ia64: Move nsecs based cputime headers to the last arch using it Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 37/37] s390: Prevent from cputime leaks Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
    Re: [PATCH 37/37] s390: Prevent from cputime leaks Martin Schwidefsky <schwidefsky@de.ibm.com> - 2017-01-23 10:50 +0100
      Re: [PATCH 37/37] s390: Prevent from cputime leaks Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-25 16:30 +0100
        Re: [PATCH 37/37] s390: Prevent from cputime leaks Martin Schwidefsky <schwidefsky@de.ibm.com> - 2017-01-25 16:50 +0100
  [PATCH 27/37] vtime: Return nsecs instead of cputime_t to account Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 32/37] s390: Make arch_cpu_idle_time() to return nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 31/37] ia64: Remove unused cputime definitions Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 18/37] posix-timers: Use TICK_NSEC instead of a dynamically ad-hoc calculated version Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 02/37] time: Introduce jiffies64_to_nsecs() Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 15/37] tsacct: Convert obsolete cputime type to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 14/37] delaycct: Convert obsolete cputime type to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 25/37] cputime: Push time to account_system_time() in nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 23/37] cputime: Push time to account_steal_time() in nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 35/37] cputime: Remove unused nsec_to_cputime Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 19/37] posix-timers: Convert internals to use nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 17/37] cputime: Increment kcpustat directly on irqtime account Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100
  [PATCH 06/37] cputime: Convert guest time accounting to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2017-01-22 19:30 +0100

csiph-web