linux-kernel - Re: Performance Stats: Kernel patch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 04 Apr 2007 17:50:50 -0400
From:	Valdis.Kletnieks@...edu
To:	Maxim Uvarov <muvarov@...mvista.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: Performance Stats: Kernel patch

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@...mvista.com>

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

Content of type "application/pgp-signature" skipped