Re: [PATCH 1/3] sched, timer: Remove usages of ACCESS_ONCE in the scheduler

[Ingo Molnar <mingo@kernel.org>]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 14 Apr 2015 19:12:33 -0700
> Jason Low <jason.low2@hp.com> wrote:
> 
> > Hi Steven,
> > 
> > On Tue, 2015-04-14 at 19:59 -0400, Steven Rostedt wrote:
> > > On Tue, 14 Apr 2015 16:09:44 -0700
> > > Jason Low <jason.low2@hp.com> wrote:
> > > 
> > > 
> > > > @@ -2088,7 +2088,7 @@ void task_numa_fault(int last_cpupid, int mem_node, int pages, int flags)
> > > >  
> > > >  static void reset_ptenuma_scan(struct task_struct *p)
> > > >  {
> > > > -	ACCESS_ONCE(p->mm->numa_scan_seq)++;
> > > > +	WRITE_ONCE(p->mm->numa_scan_seq, READ_ONCE(p->mm->numa_scan_seq) + 1);
> > > 
> > > Is the READ_ONCE() inside the WRITE_ONCE() really necessary?
> > 
> > Yeah, I think so to be safe, otherwise, the access of
> > p->mm->numa_scan_seq in the 2nd parameter doesn't have the volatile
> > cast.
> 
> You are correct. Now I'm thinking that the WRITE_ONCE() is not needed,
> and just a:
> 
> 	p->mm->numa_scan_seq = READ_ONCE(p->numa_scan_seq) + 1;
> 
> Can be done. But I'm still trying to wrap my head around why this is
> needed here. Comments would have been really helpful. We should make
> all READ_ONCE() WRITE_ONCE and obsolete ACCESS_ONCE() have mandatory
> comments just like we do with memory barriers.

So the original ACCESS_ONCE() barriers were misguided to begin with: I 
think they tried to handle races with the scheduler balancing softirq 
and tried to avoid having to use atomics for the sequence counter 
(which would be overkill), but things like ACCESS_ONCE(x)++ never 
guaranteed atomicity (or even coherency) of the update.

But since in reality this is only statistical sampling code, all these 
compiler barriers can be removed I think. Peter, Mel, Rik, do you 
agree?

Thanks,

	Ingo