Messages in this thread | | | Date | Sun, 22 Mar 2009 01:55:59 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH v2] perfcounters: record time running and time enabled for each counter |
| |
On Sun, 22 Mar 2009 10:13:35 +1100 Paul Mackerras <paulus@samba.org> wrote:
> Andrew Morton writes: > > > Perhaps one of the reasons why this code is confusing is the blurring > > between the "time" at which an event occured and the "time" between the > > occurrence of two events. A weakness in English, I guess. Using the term > > "interval" in the latter case will help a lot. > > Except that we aren't measuring an "interval", we're measuring the > combined length of a whole series of intervals. What's a good word > for that?
foo_total_time?
It doesn't matter so much if the thing has a comment at the definition site.
> > > + atomic64_t child_time_enabled; > > > + atomic64_t child_time_running; > > > > These read like booleans, but why are they atomic64_t's? > > OK so this file could use more comments, but I did answer that > question in the patch description. > > > > - return put_user(cntval, (u64 __user *) buf) ? -EFAULT : sizeof(cntval); > > > + if (count != n * sizeof(u64)) > > > + return -EINVAL; > > > + > > > + if (!access_ok(VERIFY_WRITE, buf, count)) > > > + return -EFAULT; > > > + > > > > <panics> > > > > Oh. > > > > It would be a lot more reassuring to verify `uptr', rather than `buf' here.
This?
> > The patch adds new trailing whitespace. checkpatch helps. > > > > > + for (i = 0; i < n; ++i) > > > + if (__put_user(values[i], uptr + i)) > > > + return -EFAULT; > > > > And here we iterate across `n', whereas we verified `count'. > > And the fact that we just verified count == n * 8, four lines above, > doesn't give you any comfort?
access_ok(..., uptr, n * sizeof(*uptr))
might be most robust.
Or fix up the types (if needed) and copy the whole thing with copy_to_user()
Is it really so performance-sensitive that we can't use plain old put_user()?
| |