* Steven Rostedt <rost...@goodmis.org> wrote:
> On Tue, 14 Apr 2015 19:12:33 -0700
> Jason Low <jason.l...@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.l...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
