lkml.org 
[lkml]   [2009]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] perfcounters: record time running and time enabled for each counter
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?

> > + 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.
>
> 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?

Paul.


\
 
 \ /
  Last update: 2009-03-22 00:23    [W:0.059 / U:34.300 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site