Date: Wed, 15 Apr 2015 09:46:01 +0200
From: Ingo Molnar <mingo@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>, Mel Gorman <mel@csn.ul.ie>,
        Rik van Riel <riel@redhat.com>
Cc: Jason Low <jason.low2@hp.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Oleg Nesterov <oleg@redhat.com>,
        Mike Galbraith <umgwanakikbuti@gmail.com>,
        Frederic Weisbecker <fweisbec@gmail.com>, Mel Gorman <mgorman@suse.de>,
        Preeti U Murthy <preeti@linux.vnet.ibm.com>, hideaki.kimura@hp.com,
        Aswin Chandramouleeswaran <aswin@hp.com>,
        Scott J Norton <scott.norton@hp.com>
Subject: Re: [PATCH 1/3] sched, timer: Remove usages of ACCESS_ONCE in the
 scheduler
Message-ID: <20150415074601.GC13449@gmail.com>
References: <1429052986-9420-1-git-send-email-jason.low2@hp.com>
 <1429052986-9420-2-git-send-email-jason.low2@hp.com>
 <20150414195906.3adc89d9@gandalf.local.home>
 <1429063953.7039.88.camel@j-VirtualBox>
 <20150414224059.061ec5bf@grimm.local.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20150414224059.061ec5bf@grimm.local.home>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2000
Lines: 54


* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/