Re: Performance Stats: Kernel patch

[Valdis . Kletnieks]

On Wed, 04 Apr 2007 17:15:43 +0400, Maxim Uvarov said:

> New version of this patch. Please flay it.
> 
> 
> Signed-off-by: Max Uvarov <muvarov@xxxxxxxxxxxxx>

> Index: linux-2.6.18/fs/proc/array.c
> ===================================================================
> --- linux-2.6.18.orig/fs/proc/array.c
> +++ linux-2.6.18/fs/proc/array.c
> @@ -295,6 +295,20 @@ static inline char *task_cap(struct task
>                             cap_t(p->cap_effective));
>  }
> 
> +#ifdef CONFIG_THREAD_PERF_STAT
> +static inline char *task_perf(struct task_struct *p, char *buffer)
> +{
> +#ifdef THREAD_PERF_STAT_SYSC

Missed the CONFIG_ here.

> +       return buffer + sprintf(buffer, "Nvcsw:\t%lu\n"
> +                           "Nivcsw:\t%lu\n",
> +                           cap_t(p->nvcsw),
> +                           cap_t(p->nivcsw));

cap_t()??!?  That's from include/linux/capability.h, and looks something like:

#ifdef STRICT_CAP_T_TYPECHECKS
#define cap_t(x) (x).cap
#else
#define cap_t(x) (x)
#endif

and you're probably picking up the second part, making it a no-op.  And if you
ever hit the first part of that ifdef, you'll throw a compile error.

Somebody else can comment on the use of #ifdef - we tend to frown on it inside
open C code, but I'm not seeing a really brilliant way to avoid them entirely
(the 'static inline task_perf' can probably move to a .h, but it's hard to
find a clean way to avoid the ifdefs given that we have Kconfig variables
to select it. array.c already has a CONFIG_S390 in it, anyhow. :)

There's a mostly-hypothetical race between inc_syscall() and the places that
increment the context switch counters, and where we read the values - but at
worst, we'll output a stale off-by-one-ish value.  Certainly not worth
grabbing a lock on the task struct for *this* usage, but the sort of thing you
want to keep in mind as you write other code.

Other random comments:

1) You probably want to rebase against something more recent (2.6.21-rc<mumble>
or the final .21 when it's released).

2) It arrived here with some line-wrapping damage, most likely to the fact
that you posted it with Thunderbird.  There's a mystic Thunderbird incantation
to make it not do that, but I have no idea what it is - it's in the list
archives someplace.

Attachment: pgp00000.pgp
Description: PGP signature