lkml.org 
[lkml]   [2009]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/9] perf_counter: fix update_userpage()
From
Date
On Thu, 2009-04-02 at 10:50 +0200, Peter Zijlstra wrote:
> On Sun, 2009-03-29 at 11:24 +1100, Paul Mackerras wrote:
>
> > > --- linux-2.6.orig/include/linux/perf_counter.h
> > > +++ linux-2.6/include/linux/perf_counter.h
> > > @@ -160,10 +160,45 @@ struct perf_counter_hw_event {
> > > struct perf_counter_mmap_page {
> > > __u32 version; /* version number of this structure */
> > > __u32 compat_version; /* lowest version this is compat with */
> > > +
> > > + /*
> > > + * Bits needed to read the hw counters in user-space.
> > > + *
> > > + * The index and offset should be read atomically using the seqlock:
> > > + *
> > > + * __u32 seq, index;
> > > + * __s64 offset;
> > > + *
> > > + * again:
> > > + * rmb();
> > > + * seq = pc->lock;
> > > + *
> > > + * if (unlikely(seq & 1)) {
> > > + * cpu_relax();
> > > + * goto again;
> > > + * }
> > > + *
> > > + * index = pc->index;
> > > + * offset = pc->offset;
> > > + *
> > > + * rmb();
> > > + * if (pc->lock != seq)
> > > + * goto again;
> > > + *
> > > + * After this, index contains architecture specific counter index + 1,
> > > + * so that 0 means unavailable, offset contains the value to be added
> > > + * to the result of the raw timer read to obtain this counter's value.
> > > + */
> > > __u32 lock; /* seqlock for synchronization */
> > > __u32 index; /* hardware counter identifier */
> > > __s64 offset; /* add to hardware counter value */
> >
> > I think we can simplify this (in a follow-on patch).
> >
> > It has occurred to me that we don't need to do all this on the
> > userspace side, because we are necessarily reading and writing these
> > fields on the same CPU. If the reader and writer were on different
> > CPUs, that would make no sense since they would be accessing different
> > hardware counter registers.
> >
> > That means that we don't need any CPU memory barriers on either side.
> > All the kernel needs to do is to increment `lock' when it updates
> > things, and the user side can be:
> >
> > do {
> > seq = pc->lock;
> > index = pc->index;
> > offset = pc->offset;
> > barrier();
> > } while (pc->lock != seq);
> >
> > and all that's needed is a compiler barrier to stop the compiler from
> > optimizing too much.
>
> Can this work at all?
>
> I mean, user-space could get preempted/rescheduled after we read the
> mmap() data using that seqlock and before we actually did the read-pmc
> bit.
>
> In that case, the counter can have changed underneath us and we're
> reading rubbish.

The below might work:

u32 seq;
s64 count;

again:
seq = pc->lock;

if (unlikely(seq & 1)) {
cpu_relax();
goto again;
}

count = pmc_read(pc->index);
counter += pc->offset;

barrier();
if (pc->lock != seq)
goto again;






\
 
 \ /
  Last update: 2009-04-02 11:05    [W:0.085 / U:0.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site