[PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[Mikael Pettersson]

perfctr-2.7.3 for 2.6.7-rc1-mm1, part 1/6:

- core driver files and kernel changes

 drivers/Makefile          |    1 
 drivers/perfctr/Kconfig   |   45 +++++++++++++
 drivers/perfctr/Makefile  |   16 ++++
 drivers/perfctr/cpumask.h |   24 +++++++
 drivers/perfctr/init.c    |   97 +++++++++++++++++++++++++++++
 drivers/perfctr/version.h |    1 
 include/linux/perfctr.h   |  153 ++++++++++++++++++++++++++++++++++++++++++++++
 kernel/sched.c            |    3 
 kernel/sys.c              |    6 +
 kernel/timer.c            |    2 
 10 files changed, 348 insertions(+)

diff -ruN linux-2.6.7-rc1-mm1/drivers/Makefile linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/drivers/Makefile
--- linux-2.6.7-rc1-mm1/drivers/Makefile	2004-04-04 13:49:10.000000000 +0200
+++ linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/drivers/Makefile	2004-05-31 23:43:34.232821000 +0200
@@ -49,4 +49,5 @@
 obj-$(CONFIG_MCA)		+= mca/
 obj-$(CONFIG_EISA)		+= eisa/
 obj-$(CONFIG_CPU_FREQ)		+= cpufreq/
+obj-$(CONFIG_PERFCTR)		+= perfctr/
 obj-y				+= firmware/
diff -ruN linux-2.6.7-rc1-mm1/drivers/perfctr/Kconfig linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/drivers/perfctr/Kconfig
--- linux-2.6.7-rc1-mm1/drivers/perfctr/Kconfig	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/drivers/perfctr/Kconfig	2004-05-31 23:43:34.232821000 +0200
@@ -0,0 +1,45 @@
+# $Id: Kconfig,v 1.10 2004/05/24 11:00:55 mikpe Exp $
+# Performance-monitoring counters driver configuration
+#
+
+menu "Performance-monitoring counters support"
+
+config PERFCTR
+	bool "Performance monitoring counters support"
+	help
+	  This driver provides access to the performance-monitoring counter
+	  registers available in some (but not all) modern processors.
+	  These special-purpose registers can be programmed to count low-level
+	  performance-related events which occur during program execution,
+	  such as cache misses, pipeline stalls, etc.
+
+	  You can safely say Y here, even if you intend to run the kernel
+	  on a processor without performance-monitoring counters.
+
+config PERFCTR_INIT_TESTS
+	bool "Init-time hardware tests"
+	depends on PERFCTR
+	help
+	  This option makes the driver perform additional hardware tests
+	  during initialisation, and log their results in the kernel's
+	  message buffer. For most supported processors, these tests simply
+	  measure the runtime overheads of performance counter operations.
+
+	  If you have a less well-known processor (one not listed in the
+	  etc/costs/ directory in the user-space package), you should enable
+	  this option and email the results to the perfctr developers.
+
+	  If unsure, say N.
+
+config PERFCTR_VIRTUAL
+	bool "Virtual performance counters support"
+	depends on PERFCTR
+	help
+	  The processor's performance-monitoring counters are special-purpose
+	  global registers. This option adds support for virtual per-process
+	  performance-monitoring counters which only run when the process
+	  to which they belong is executing. This improves the accuracy of
+	  performance measurements by reducing "noise" from other processes.
+
+	  Say Y.
+endmenu
diff -ruN linux-2.6.7-rc1-mm1/drivers/perfctr/Makefile linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/drivers/perfctr/Makefile
--- linux-2.6.7-rc1-mm1/drivers/perfctr/Makefile	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/drivers/perfctr/Makefile	2004-05-31 23:43:34.232821000 +0200
@@ -0,0 +1,16 @@
+# $Id: Makefile,v 1.26 2004/05/30 23:02:14 mikpe Exp $
+# Makefile for the Performance-monitoring counters driver.
+
+# This also covers x86_64.
+perfctr-objs-$(CONFIG_X86) := x86.o
+tests-objs-$(CONFIG_X86) := x86_tests.o
+
+perfctr-objs-$(CONFIG_PPC32) := ppc.o
+tests-objs-$(CONFIG_PPC32) := ppc_tests.o
+
+perfctr-objs-y += init.o
+perfctr-objs-$(CONFIG_PERFCTR_INIT_TESTS) += $(tests-objs-y)
+perfctr-objs-$(CONFIG_PERFCTR_VIRTUAL) += virtual.o
+
+perfctr-objs		:= $(perfctr-objs-y)
+obj-$(CONFIG_PERFCTR)	:= perfctr.o
diff -ruN linux-2.6.7-rc1-mm1/drivers/perfctr/cpumask.h linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/drivers/perfctr/cpumask.h
--- linux-2.6.7-rc1-mm1/drivers/perfctr/cpumask.h	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/drivers/perfctr/cpumask.h	2004-05-31 23:43:34.232821000 +0200
@@ -0,0 +1,24 @@
+/* $Id: cpumask.h,v 1.7 2004/05/12 19:59:01 mikpe Exp $
+ * Performance-monitoring counters driver.
+ * Partial simulation of cpumask_t on non-cpumask_t kernels.
+ * Extension to allow inspecting a cpumask_t as array of ulong.
+ * Appropriate definition of perfctr_cpus_forbidden_mask.
+ *
+ * Copyright (C) 2003-2004  Mikael Pettersson
+ */
+
+#ifdef CPU_ARRAY_SIZE
+#define PERFCTR_CPUMASK_NRLONGS	CPU_ARRAY_SIZE
+#else
+#define PERFCTR_CPUMASK_NRLONGS	1
+#endif
+
+/* `perfctr_cpus_forbidden_mask' used to be defined in <asm/perfctr.h>,
+   but cpumask_t compatibility issues forced it to be moved here. */
+#if PERFCTR_CPUS_FORBIDDEN_MASK_NEEDED
+extern cpumask_t perfctr_cpus_forbidden_mask;
+#define perfctr_cpu_is_forbidden(cpu)	cpu_isset((cpu), perfctr_cpus_forbidden_mask)
+#else
+#define perfctr_cpus_forbidden_mask	CPU_MASK_NONE
+#define perfctr_cpu_is_forbidden(cpu)	0 /* cpu_isset() needs an lvalue :-( */
+#endif
diff -ruN linux-2.6.7-rc1-mm1/drivers/perfctr/init.c linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/drivers/perfctr/init.c
--- linux-2.6.7-rc1-mm1/drivers/perfctr/init.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/drivers/perfctr/init.c	2004-05-31 23:43:34.232821000 +0200
@@ -0,0 +1,97 @@
+/* $Id: init.c,v 1.76 2004/05/31 18:18:55 mikpe Exp $
+ * Performance-monitoring counters driver.
+ * Top-level initialisation code.
+ *
+ * Copyright (C) 1999-2004  Mikael Pettersson
+ */
+#include <linux/config.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/sched.h>
+#include <linux/perfctr.h>
+
+#include <asm/uaccess.h>
+
+#include "cpumask.h"
+#include "virtual.h"
+#include "version.h"
+
+struct perfctr_info perfctr_info = {
+	.abi_version = PERFCTR_ABI_VERSION,
+	.driver_version = VERSION,
+};
+
+char *perfctr_cpu_name __initdata;
+
+static int cpus_copy_to_user(const cpumask_t *cpus, struct perfctr_cpu_mask *argp)
+{
+	const unsigned int k_nrwords = PERFCTR_CPUMASK_NRLONGS*(sizeof(long)/sizeof(int));
+	unsigned int u_nrwords;
+	unsigned int ui, ki, j;
+
+	if (get_user(u_nrwords, &argp->nrwords))
+		return -EFAULT;
+	if (put_user(k_nrwords, &argp->nrwords))
+		return -EFAULT;
+	if (u_nrwords < k_nrwords)
+		return -EOVERFLOW;
+	for(ui = 0, ki = 0; ki < PERFCTR_CPUMASK_NRLONGS; ++ki) {
+		unsigned long mask = cpus_addr(*cpus)[ki];
+		for(j = 0; j < sizeof(long)/sizeof(int); ++j) {
+			if (put_user((unsigned int)mask, &argp->mask[ui]))
+				return -EFAULT;
+			++ui;
+			mask = (mask >> (8*sizeof(int)-1)) >> 1;
+		}
+	}
+	return 0;
+}
+
+asmlinkage long sys_perfctr_info(struct perfctr_info *infop,
+				 struct perfctr_cpu_mask *cpusp,
+				 struct perfctr_cpu_mask *forbiddenp)
+{
+	if (infop && copy_to_user(infop, &perfctr_info, sizeof perfctr_info))
+		return -EFAULT;
+	if (cpusp) {
+		cpumask_t cpus = cpu_online_map;
+		int err = cpus_copy_to_user(&cpus, cpusp);
+		if (err)
+			return err;
+	}
+	if (forbiddenp) {
+		cpumask_t cpus = perfctr_cpus_forbidden_mask;
+		int err = cpus_copy_to_user(&cpus, forbiddenp);
+		if (err)
+			return err;
+	}
+	return 0;
+}
+
+int __init perfctr_init(void)
+{
+	int err;
+
+	err = perfctr_cpu_init();
+	if (err) {
+		printk(KERN_INFO "perfctr: not supported by this processor\n");
+		return err;
+	}
+	err = vperfctr_init();
+	if (err)
+		return err;
+	printk(KERN_INFO "perfctr: driver %s, cpu type %s at %u kHz\n",
+	       perfctr_info.driver_version,
+	       perfctr_cpu_name,
+	       perfctr_info.cpu_khz);
+	return 0;
+}
+
+void __exit perfctr_exit(void)
+{
+	vperfctr_exit();
+	perfctr_cpu_exit();
+}
+
+module_init(perfctr_init)
+module_exit(perfctr_exit)
diff -ruN linux-2.6.7-rc1-mm1/drivers/perfctr/version.h linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/drivers/perfctr/version.h
--- linux-2.6.7-rc1-mm1/drivers/perfctr/version.h	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/drivers/perfctr/version.h	2004-05-31 23:43:34.232821000 +0200
@@ -0,0 +1 @@
+#define VERSION "2.7.3"
diff -ruN linux-2.6.7-rc1-mm1/include/linux/perfctr.h linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/include/linux/perfctr.h
--- linux-2.6.7-rc1-mm1/include/linux/perfctr.h	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/include/linux/perfctr.h	2004-05-31 23:43:34.232821000 +0200
@@ -0,0 +1,153 @@
+/* $Id: perfctr.h,v 1.78 2004/05/31 20:45:51 mikpe Exp $
+ * Performance-Monitoring Counters driver
+ *
+ * Copyright (C) 1999-2004  Mikael Pettersson
+ */
+#ifndef _LINUX_PERFCTR_H
+#define _LINUX_PERFCTR_H
+
+#ifdef CONFIG_PERFCTR	/* don't break archs without <asm/perfctr.h> */
+
+#include <asm/perfctr.h>
+
+struct perfctr_info {
+	unsigned int abi_version;
+	char driver_version[32];
+	unsigned int cpu_type;
+	unsigned int cpu_features;
+	unsigned int cpu_khz;
+	unsigned int tsc_to_cpu_mult;
+	unsigned int _reserved2;
+	unsigned int _reserved3;
+	unsigned int _reserved4;
+};
+
+struct perfctr_cpu_mask {
+	unsigned int nrwords;
+	unsigned int mask[1];	/* actually 'nrwords' */
+};
+
+/* abi_version values: Lower 16 bits contain the CPU data version, upper
+   16 bits contain the API version. Each half has a major version in its
+   upper 8 bits, and a minor version in its lower 8 bits. */
+#define PERFCTR_API_VERSION	0x0600	/* 6.0 */
+#define PERFCTR_ABI_VERSION	((PERFCTR_API_VERSION<<16)|PERFCTR_CPU_VERSION)
+
+/* cpu_features flag bits */
+#define PERFCTR_FEATURE_RDPMC	0x01
+#define PERFCTR_FEATURE_RDTSC	0x02
+#define PERFCTR_FEATURE_PCINT	0x04
+
+/* user's view of mmap:ed virtual perfctr */
+struct vperfctr_state {
+	struct perfctr_cpu_state cpu_state;
+};
+
+/* virtual perfctr control object */
+struct vperfctr_control {
+	int si_signo;
+	struct perfctr_cpu_control cpu_control;
+	unsigned int preserve;
+	unsigned int _reserved1;
+	unsigned int _reserved2;
+	unsigned int _reserved3;
+	unsigned int _reserved4;
+};
+
+#endif	/* CONFIG_PERFCTR */
+
+#ifdef __KERNEL__
+
+/*
+ * The perfctr system calls.
+ */
+asmlinkage long sys_perfctr_info(struct perfctr_info*, struct perfctr_cpu_mask*, struct perfctr_cpu_mask*);
+asmlinkage long sys_vperfctr_open(int tid, int creat);
+asmlinkage long sys_vperfctr_control(int fd, const struct vperfctr_control*);
+asmlinkage long sys_vperfctr_unlink(int fd);
+asmlinkage long sys_vperfctr_iresume(int fd);
+asmlinkage long sys_vperfctr_read(int fd,
+				  struct perfctr_sum_ctrs*,
+				  struct vperfctr_control*);
+
+extern struct perfctr_info perfctr_info;
+
+#ifdef CONFIG_PERFCTR_VIRTUAL
+
+/*
+ * Virtual per-process performance-monitoring counters.
+ */
+struct vperfctr;	/* opaque */
+
+/* process management operations */
+extern struct vperfctr *__vperfctr_copy(struct vperfctr*);
+extern void __vperfctr_exit(struct vperfctr*);
+extern void __vperfctr_suspend(struct vperfctr*);
+extern void __vperfctr_resume(struct vperfctr*);
+extern void __vperfctr_sample(struct vperfctr*);
+extern void __vperfctr_set_cpus_allowed(struct task_struct*, struct vperfctr*, cpumask_t);
+
+static inline void perfctr_copy_thread(struct thread_struct *thread)
+{
+	thread->perfctr = NULL;
+}
+
+static inline void perfctr_exit_thread(struct thread_struct *thread)
+{
+	struct vperfctr *perfctr;
+	perfctr = thread->perfctr;
+	if (perfctr)
+		__vperfctr_exit(perfctr);
+}
+
+static inline void perfctr_suspend_thread(struct thread_struct *prev)
+{
+	struct vperfctr *perfctr;
+	perfctr = prev->perfctr;
+	if (perfctr)
+		__vperfctr_suspend(perfctr);
+}
+
+static inline void perfctr_resume_thread(struct thread_struct *next)
+{
+	struct vperfctr *perfctr;
+	perfctr = next->perfctr;
+	if (perfctr)
+		__vperfctr_resume(perfctr);
+}
+
+static inline void perfctr_sample_thread(struct thread_struct *thread)
+{
+	struct vperfctr *perfctr;
+	perfctr = thread->perfctr;
+	if (perfctr)
+		__vperfctr_sample(perfctr);
+}
+
+static inline void perfctr_set_cpus_allowed(struct task_struct *p, cpumask_t new_mask)
+{
+#if PERFCTR_CPUS_FORBIDDEN_MASK_NEEDED
+	struct vperfctr *perfctr;
+
+	task_lock(p);
+	perfctr = p->thread.perfctr;
+	if (perfctr)
+		__vperfctr_set_cpus_allowed(p, perfctr, new_mask);
+	task_unlock(p);
+#endif
+}
+
+#else	/* !CONFIG_PERFCTR_VIRTUAL */
+
+static inline void perfctr_copy_thread(struct thread_struct *t) { }
+static inline void perfctr_exit_thread(struct thread_struct *t) { }
+static inline void perfctr_suspend_thread(struct thread_struct *t) { }
+static inline void perfctr_resume_thread(struct thread_struct *t) { }
+static inline void perfctr_sample_thread(struct thread_struct *t) { }
+static inline void perfctr_set_cpus_allowed(struct task_struct *p, cpumask_t m) { }
+
+#endif	/* CONFIG_PERFCTR_VIRTUAL */
+
+#endif	/* __KERNEL__ */
+
+#endif	/* _LINUX_PERFCTR_H */
diff -ruN linux-2.6.7-rc1-mm1/kernel/sched.c linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/kernel/sched.c
--- linux-2.6.7-rc1-mm1/kernel/sched.c	2004-05-30 15:59:30.000000000 +0200
+++ linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/kernel/sched.c	2004-05-31 23:43:34.232821000 +0200
@@ -40,6 +40,7 @@
 #include <linux/rcupdate.h>
 #include <linux/cpu.h>
 #include <linux/percpu.h>
+#include <linux/perfctr.h>
 #include <linux/kthread.h>
 #include <linux/seq_file.h>
 #include <linux/times.h>
@@ -3476,6 +3477,8 @@
 	migration_req_t req;
 	runqueue_t *rq;
 
+	perfctr_set_cpus_allowed(p, new_mask);
+
 	rq = task_rq_lock(p, &flags);
 	if (any_online_cpu(new_mask) == NR_CPUS) {
 		ret = -EINVAL;
diff -ruN linux-2.6.7-rc1-mm1/kernel/sys.c linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/kernel/sys.c
--- linux-2.6.7-rc1-mm1/kernel/sys.c	2004-05-30 15:59:30.000000000 +0200
+++ linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/kernel/sys.c	2004-05-31 23:43:34.232821000 +0200
@@ -279,6 +279,12 @@
 cond_syscall(sys_pciconfig_read)
 cond_syscall(sys_pciconfig_write)
 cond_syscall(sys_pciconfig_iobase)
+cond_syscall(sys_perfctr_info)
+cond_syscall(sys_vperfctr_open)
+cond_syscall(sys_vperfctr_control)
+cond_syscall(sys_vperfctr_unlink)
+cond_syscall(sys_vperfctr_iresume)
+cond_syscall(sys_vperfctr_read)
 
 static int set_one_prio(struct task_struct *p, int niceval, int error)
 {
diff -ruN linux-2.6.7-rc1-mm1/kernel/timer.c linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/kernel/timer.c
--- linux-2.6.7-rc1-mm1/kernel/timer.c	2004-05-30 15:59:08.000000000 +0200
+++ linux-2.6.7-rc1-mm1.perfctr-2.7.3.core/kernel/timer.c	2004-05-31 23:43:34.232821000 +0200
@@ -31,6 +31,7 @@
 #include <linux/time.h>
 #include <linux/jiffies.h>
 #include <linux/cpu.h>
+#include <linux/perfctr.h>
 
 #include <asm/uaccess.h>
 #include <asm/unistd.h>
@@ -835,6 +836,7 @@
 	do_process_times(p, user, system);
 	do_it_virt(p, user);
 	do_it_prof(p);
+	perfctr_sample_thread(&p->thread);
 }	
 
 /*

Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[Andrew Morton]

Mikael Pettersson <mikpe@csd.uu.se> wrote:
>
> perfctr-2.7.3 for 2.6.7-rc1-mm1, part 1/6:
>

OK, I spent a couple hours on the perfctr code.  It looks sane, although a
bit hard to follow.  The above changelog (which is _all_ you gave us) makes
this large and complex patch hard to review, hard to understand, hard for
others to support.

One can look at the code, sort-of-see what it's doing, query micro-issues,
but it is hard (and a bad use of one's time) to try and reverse-engineer
the big-picture "how it all hangs together" from the implementation.

We need (especially at this stage in the kernel cycle) at least a couple of
pages of design documentation which describes

- the major data structures

- the relationships between them

- their lifetime rules.

- a general overview of how the whole thing operates (what's
  PERFCTR_INTERRUPT_SUPPORT do?  What interrupts are generated?  Describe
  the backpatching design, etc).  What is "forbidden" on p4 siblings, and
  why and what is the implication of this?  etcetera.

- the thread/process/data structure inheritance/sharing/creation models

- any known shortcomings, rejected design decisions, etc.

- the core<->arch-specific interface


Also there should be a document or a manpage or something which describes,
in detail:

- the user/kernel API  (separate document, probably)

- what's in that filesystem, and how the objects in it are used.

- the sysfs interface

- the relationship with ptrace



Random points:


- In __vperfctr_set_cpus_allowed(), is it possible for a process to
  generate that printk deliberately, thus spamming the logs?

- perfctr_set_cpus_allowed() does task_lock().  Should that be
  vperfctr_task_lock() instead?

  Please update the locking comment over task_lock() to represent this
  new usage.

  Note that printk() can call wake_up(), which takes scheduler locks.  So
  you've introduced a lock ranking here.  Looks to be OK though.

- (What is the thread/process/child inheritance model for perfctr
  anyway?)

- Why does sys_vperfctr_open() call ptrace_check_attach()?  (I suspect
  I'd know that if there was API documentation?)

- cpus_copy_to_user() has the arguments in the wrong order, and should
  have sparse annotation added, please.

- <canofworms>cannot cpus_copy_to_user() share code with
  sys_sched_getaffinity()?</canofworms>

- Sometime all the new syscalls need sparse annotation added, propagated
  into callees.

- Can perfctr_init() and perfctr_exit() have static scope?

- This

	cache = &get_cpu_cache();

  looks cumbersome.  It'd be nice to add the & to get_cpu_cache() itself.

- In perfctr_cpu_init():

	perfctr_cpu_init_one(NULL);
	smp_call_function(perfctr_cpu_init_one, NULL, 1, 1);

  use on_each_cpu() here.  Ditto perfctr_cpu_exit(), other places.

- Why does perfctr_cpu_init() do preempt_disable()?  Needs a comment. 
  Ditto perfctr_cpu_exit().

- Why does vperfctr_alloc() allocate an entire page?  (Needs comment)

- Why is that page reserved?

- The top-level Kconfig help should perhaps contain some handy URLs. 
  (Except there don't seem to be any :()

- stack space.

  struct vperfctr_control is 348 bytes, do_vperfctr_read() uses 500 bytes
  of stack and does copy_to_user(), which can cause tremendously deep
  callchains (think: it can call into XFS and then into the qlogic driver ;))

  These big structures should be dynamically allocated.

  sizeof(struct perfctr_cpu_state) is 708.

- ooh, it has a filesystem, and something can be mmapped.  API documentation? ;)

- why is the filesystem kern_mount()ed?

- why are the inodes initialised to state I_DIRTY?

- In sys_vperfctr_open(), can the `if (!vperfctr_fs_init_done())' test
  actually return true?

- Can the presence of thread_struct.perfctr be dependent upon
  CONFIG_PERFCTR?

- If task A creates itself a node in that new filesystem (what's the
  naming schema there?  What facilities does the filesystem offer?  Why was
  an fs interface chosen?) and task B opens that node, then task A exits,
  what keeps the task_struct at the node's file->f_private_data->owner
  valid?

  What happens in this situation?  Is it valid usage?  Should the read fail?

- Are functions like p5_write_control() preempt-protected?

- Is there much point in making CONFIG_PERFCTR_VIRTUAL optional?

Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[Christoph Hellwig]

On Tue, Jun 22, 2004 at 01:53:11AM -0700, Andrew Morton wrote:
> Also there should be a document or a manpage or something which describes,
> in detail:
> 
> - the user/kernel API  (separate document, probably)

It also needs moving back to /proc/<pid>/ files from the syscall API.

Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[Andrew Morton]

Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Jun 22, 2004 at 01:53:11AM -0700, Andrew Morton wrote:
> > Also there should be a document or a manpage or something which describes,
> > in detail:
> > 
> > - the user/kernel API  (separate document, probably)
> 
> It also needs moving back to /proc/<pid>/ files from the syscall API.

What does this mean?

Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[Christoph Hellwig]

On Tue, Jun 22, 2004 at 02:04:17AM -0700, Andrew Morton wrote:
> Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Tue, Jun 22, 2004 at 01:53:11AM -0700, Andrew Morton wrote:
> > > Also there should be a document or a manpage or something which describes,
> > > in detail:
> > > 
> > > - the user/kernel API  (separate document, probably)
> > 
> > It also needs moving back to /proc/<pid>/ files from the syscall API.
> 
> What does this mean?

Early version of perfctr used files in /proc/<pid>/ for controlling perfctr
instead of the syscalls, and indeed that's a much better fit for most of them.
Let's ressurect that code instead of doing the syscall approach.

Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[Andrew Morton]

Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Jun 22, 2004 at 02:04:17AM -0700, Andrew Morton wrote:
> > Christoph Hellwig <hch@infradead.org> wrote:
> > >
> > > On Tue, Jun 22, 2004 at 01:53:11AM -0700, Andrew Morton wrote:
> > > > Also there should be a document or a manpage or something which describes,
> > > > in detail:
> > > > 
> > > > - the user/kernel API  (separate document, probably)
> > > 
> > > It also needs moving back to /proc/<pid>/ files from the syscall API.
> > 
> > What does this mean?
> 
> Early version of perfctr used files in /proc/<pid>/ for controlling perfctr
> instead of the syscalls, and indeed that's a much better fit for most of them.
> Let's ressurect that code instead of doing the syscall approach.

This appears to have come out of the blue.  Please explain why you think
this change is needed.

Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[Christoph Hellwig]

On Tue, Jun 22, 2004 at 02:14:41AM -0700, Andrew Morton wrote:
> > Let's ressurect that code instead of doing the syscall approach.
> 
> This appears to have come out of the blue.  Please explain why you think
> this change is needed.

Just read through all the old perfctr threads.  This was discussed multiple
times.

Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[Andrew Morton]

Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Jun 22, 2004 at 02:14:41AM -0700, Andrew Morton wrote:
> > > Let's ressurect that code instead of doing the syscall approach.
> > 
> > This appears to have come out of the blue.  Please explain why you think
> > this change is needed.
> 
> Just read through all the old perfctr threads.  This was discussed multiple
> times.

Well it didn't register with me and either it didn't register with Mike, or
he rejected the notion.

Please restate the case.

Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[Andi Kleen]

[few points I just noticed]

Andrew Morton <akpm@osdl.org> writes:
>
> - a general overview of how the whole thing operates (what's
>   PERFCTR_INTERRUPT_SUPPORT do?  What interrupts are generated?  Describe

The hardware can usually generate an interrupt when the performance counter
under or overflows. oprofile uses this too.


>   the backpatching design, etc).  What is "forbidden" on p4 siblings, and

p4 siblings share only a single set of performance counters, so you must
keep them synchronized.

>
> - <canofworms>cannot cpus_copy_to_user() share code with
>   sys_sched_getaffinity()?</canofworms>

The sched_getaffinity code for that is broken, I'm about to submit
a patch for it.  I haven't looked at that one.

>
> - Is there much point in making CONFIG_PERFCTR_VIRTUAL optional?

It slows down an important fast path (context switch) 
But it would be better to use some back patching design like kernel hooks
to keep the cost for the common case (no perfctr getting switched) 
low.  If that common case can be kept at a "no checks at all" level
then it's good. That would be best for distribution kernels too.

-Andi

Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[Mikael Pettersson]

Andrew Morton writes:
 > Random points:
 > 
 > 
 > - In __vperfctr_set_cpus_allowed(), is it possible for a process to
 >   generate that printk deliberately, thus spamming the logs?

On a HT P4, yes that's possible. Should I put in a rate limit?
I _must_ detect when this kernel or user error occurs, otherwise
things can break horribly on HT P4s. The only alternative to the
warning is to force a signal on the target process.

 > - perfctr_set_cpus_allowed() does task_lock().  Should that be
 >   vperfctr_task_lock() instead?

vperfctr_task_lock() _is_ task_lock() when HT P4s are possible.
In other cases vperfctr_task_lock() just disables preemption,
and perfctr_set_cpus_allows() is a no-op.

 >   Please update the locking comment over task_lock() to represent this
 >   new usage.

You mean add a comment there to the effect that set_cpus_allowed()
may do a task_lock()?

 > - (What is the thread/process/child inheritance model for perfctr
 >   anyway?)

Currently they're nuked on fork(). That's because it's difficult
to get child results back without changing wait() et al.
There has been some work in that direction, but it's a low-priority
item as long as basic API issues are being discussed.

Perfctrs are 100% tied to tasks. Whether those tasks choose to
combine themselves into POSIX processes or not is irrelevant.

 > - Why does sys_vperfctr_open() call ptrace_check_attach()?  (I suspect
 >   I'd know that if there was API documentation?)

In the remote-control case, we must check that the opening process
has the right to control the target process. I'm using the same
rules as ptrace(ATTACH) does, hence the ptrace_check_attach() call.

 > - cpus_copy_to_user() has the arguments in the wrong order, and should
 >   have sparse annotation added, please.

Agreed.

 > - <canofworms>cannot cpus_copy_to_user() share code with
 >   sys_sched_getaffinity()?</canofworms>

Not without breaking binary compatibility in both the affinity calls
and perfctr. The affinity calls' ABI is known to be broken and I will
not change my working API to theirs just for the sake of code sharing.

 > - Can perfctr_init() and perfctr_exit() have static scope?

I guess so. This is probably a left-over from pre-module_{init,exit}
days (2.2 kernels) when I had to put an init(9 call in drivers/char/misc.c.

 > - This
 > 
 > 	cache = &get_cpu_cache();
 > 
 >   looks cumbersome.  It'd be nice to add the & to get_cpu_cache() itself.

Agreed.

 > - In perfctr_cpu_init():
 > 
 > 	perfctr_cpu_init_one(NULL);
 > 	smp_call_function(perfctr_cpu_init_one, NULL, 1, 1);
 > 
 >   use on_each_cpu() here.  Ditto perfctr_cpu_exit(), other places.

This is the PPC32 driver. Yes, they can use on_each_cpu(), although
as you noticed below preemption is already disabled.

 > - Why does perfctr_cpu_init() do preempt_disable()?  Needs a comment. 
 >   Ditto perfctr_cpu_exit().

x86 still needs it in perfctr_cpu_init() since it (a) runs init
tests, and (b) goes through the low-level procedures in order to
backpatch call sites with the detected CPU type's function pointers.
These activities must not be context-switched or migrated from
one CPU to another.

The PPC32 driver inherited the preempt_disable() from the x86 driver.
It only needs it for the init_tests thing. I can move the preemption
manipulation into ppc_tests.c, and use on_each_cpu() in _init() and _exit().

 > - Why does vperfctr_alloc() allocate an entire page?  (Needs comment)

mmap()

 > - Why is that page reserved?

mmap() -- without the reservedness things broke horribly in the past.

 > - stack space.
 > 
 >   struct vperfctr_control is 348 bytes, do_vperfctr_read() uses 500 bytes
 >   of stack and does copy_to_user(), which can cause tremendously deep
 >   callchains (think: it can call into XFS and then into the qlogic driver ;))
 > 
 >   These big structures should be dynamically allocated.

There's room for a temp copy in the perfctr state object, which as
you've noticed is an entire page. That should reduce stack usage.

 >   sizeof(struct perfctr_cpu_state) is 708.

You can thank the P4 for most of that. It really is a complex beast.

 > - why is the filesystem kern_mount()ed?

It's an anonymous fs based on fs/pipe.c.

 > - why are the inodes initialised to state I_DIRTY?

See fs/pipe.c.

 > - In sys_vperfctr_open(), can the `if (!vperfctr_fs_init_done())' test
 >   actually return true?

Yes. Since the conversion to multiple system calls, there is no single
entry point which can check if the HW is supported. So now people can
call sys_vperfctr_open() even if HW detection failed. The test you
quoted protects against that.

 > - Can the presence of thread_struct.perfctr be dependent upon
 >   CONFIG_PERFCTR?

Sure, if that's desirable.

 > - If task A creates itself a node in that new filesystem (what's the
 >   naming schema there?  What facilities does the filesystem offer?  Why was
 >   an fs interface chosen?) and task B opens that node, then task A exits,
 >   what keeps the task_struct at the node's file->f_private_data->owner
 >   valid?

The fs has no directories or files, it's just there to ensure that
one can use a file descriptor as a handle, do fs-related things
(mmap(), send over a local socket), and that things like the fd
going out of scope (at close() or exit()) are detected. A perfctr
object can outlive the task it was attached to, that's part of
of how remote-control and the user-space `perfex' application work.

FS was chosen primarily because of mmap(), and secondarily because
an fd is a convenient handle on a potentially shared or non-own object.

 > - Are functions like p5_write_control() preempt-protected?

At the higher-level, yes. <asm-$ARCH/perfctr.h> documents that the
low-level accessor procedures must be called with preemption disabled.

 > - Is there much point in making CONFIG_PERFCTR_VIRTUAL optional?

It adds sizeof(struct*) bytes in the task_struct, and two
"if (tsk->ptr) do_something();" tests to switch_to(). People
may or may not want that.

The switch_to() overhead could perhaps be reduced if all unusual
cases were tested for in a single "if (tsk->unsual_mask != 0)".
I don't know if the other checks are unusual enough to warrant
such a change.

/Mikael

Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[Mikael Pettersson]

Andrew Morton writes:
 > Mikael Pettersson <mikpe@csd.uu.se> wrote:
 > >
 > > perfctr-2.7.3 for 2.6.7-rc1-mm1, part 1/6:
 > >
 > 
 > OK, I spent a couple hours on the perfctr code.  It looks sane, although a
 > bit hard to follow.  The above changelog (which is _all_ you gave us) makes
 > this large and complex patch hard to review, hard to understand, hard for
 > others to support.
 > 
 > One can look at the code, sort-of-see what it's doing, query micro-issues,
 > but it is hard (and a bad use of one's time) to try and reverse-engineer
 > the big-picture "how it all hangs together" from the implementation.
 > 
 > We need (especially at this stage in the kernel cycle) at least a couple of
 > pages of design documentation which describes

Speaking of documentation, I have a strong preference for using
LaTeX for all papers, reports, docs, etc, except man pages.
Is that an acceptable format?

Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[Mikael Pettersson]

Andrew Morton writes:
 > Christoph Hellwig <hch@infradead.org> wrote:
 > >
 > > On Tue, Jun 22, 2004 at 02:14:41AM -0700, Andrew Morton wrote:
 > > > > Let's ressurect that code instead of doing the syscall approach.
 > > > 
 > > > This appears to have come out of the blue.  Please explain why you think
 > > > this change is needed.
 > > 
 > > Just read through all the old perfctr threads.  This was discussed multiple
 > > times.
 > 
 > Well it didn't register with me and either it didn't register with Mike, or
 > he rejected the notion.
 > 
 > Please restate the case.

Swiching to open() on /proc/<pid>/<tid>/perfctr followed by ioctl()s
would be easy to implement. But people @ LKML are sometimes violently
opposed to ioctl()s, that's why the switch to syscalls happended.

One concern is that there doesn't appear to be a user-visible shortcut
to "your own" /proc/<pid>/<tid>/ like there was when /proc/self still
had some useful meaning. This is merely annoying, not a show-stopper.

Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[Christoph Hellwig]

On Tue, Jun 22, 2004 at 04:38:06PM +0200, Mikael Pettersson wrote:
> Swiching to open() on /proc/<pid>/<tid>/perfctr followed by ioctl()s
> would be easy to implement. But people @ LKML are sometimes violently
> opposed to ioctl()s, that's why the switch to syscalls happended.

I don't remember the details anymore, but lots of the syscalls could
really be read/write on special files.  I'll look through the code again
and send out draft API document.

> One concern is that there doesn't appear to be a user-visible shortcut
> to "your own" /proc/<pid>/<tid>/ like there was when /proc/self still
> had some useful meaning. This is merely annoying, not a show-stopper.

Shouldn't be a real problem to add one.

Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[Andrew Morton]

Mikael Pettersson <mikpe@csd.uu.se> wrote:
>
> Andrew Morton writes:
>  > Random points:
>  > 
>  > 
>  > - In __vperfctr_set_cpus_allowed(), is it possible for a process to
>  >   generate that printk deliberately, thus spamming the logs?
> 
> On a HT P4, yes that's possible. Should I put in a rate limit?

Spose so - __printk_ratelimit().  Or simply turn the message off until the
next reboot.

>  > - perfctr_set_cpus_allowed() does task_lock().  Should that be
>  >   vperfctr_task_lock() instead?
> 
> vperfctr_task_lock() _is_ task_lock() when HT P4s are possible.

I know.  I was asking whether that could should have used the macro rather
than open-coding the task_lock().  Whatever.

>  >   Please update the locking comment over task_lock() to represent this
>  >   new usage.
> 
> You mean add a comment there to the effect that set_cpus_allowed()
> may do a task_lock()?

That comment describes what data structures are protected by task_lock(). 
If you're aware of missing info or if you're protecting new data via
task_lock(), please update the comment.

>  > - Why does sys_vperfctr_open() call ptrace_check_attach()?  (I suspect
>  >   I'd know that if there was API documentation?)
> 
> In the remote-control case, we must check that the opening process
> has the right to control the target process. I'm using the same
> rules as ptrace(ATTACH) does, hence the ptrace_check_attach() call.

OK.  The term "remote control" has not been defined by you, so I'm to
assume that it refers to one process initiating perfctr instrumentation
against another, in some fashion.  Please feel my minor frustration ;)

>  >   These big structures should be dynamically allocated.
> 
> There's room for a temp copy in the perfctr state object, which as
> you've noticed is an entire page. That should reduce stack usage.

hm, not sure I understand that, but whatever.  Please fix up the big stack
users, especially those which do copy_*_user, or non-atomic memory
allocations.

Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[Andrew Morton]

Mikael Pettersson <mikpe@csd.uu.se> wrote:
>
>  > We need (especially at this stage in the kernel cycle) at least a couple of
>   > pages of design documentation which describes
> 
>  Speaking of documentation, I have a strong preference for using
>  LaTeX for all papers, reports, docs, etc, except man pages.
>  Is that an acceptable format?

Sounds fine, but excessive for this exercise.  At this stage I'm asking for
sufficient description of the kernel patch to allow other developers to
understand and review the patch, maybe hack on it a bit.  A couple of pages
of ascii should suffice.  I'd consider it compulsory for patches of this
magnitude and scope.

Longer term, yes - having added a bunch of new syscalls to the kernel we
have a responsibility to document the user/kernel API for downstream users,
but that's not a requirement for reviewing the patch.

Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core

[Mikael Pettersson]

Christoph Hellwig writes:
 > On Tue, Jun 22, 2004 at 04:38:06PM +0200, Mikael Pettersson wrote:
 > > Swiching to open() on /proc/<pid>/<tid>/perfctr followed by ioctl()s
 > > would be easy to implement. But people @ LKML are sometimes violently
 > > opposed to ioctl()s, that's why the switch to syscalls happended.
 > 
 > I don't remember the details anymore, but lots of the syscalls could
 > really be read/write on special files.  I'll look through the code again
 > and send out draft API document.

I've thought about this, but the FS with multiple files approach
has several problems:

1. An open file descriptor no longer suffices as a user-space handle.
   This is because we don't have fd = open("%d/file",dirfd) type
   system calls.

2. A mini-fs under /proc/<pid>/<tid>/perfctr/ disappears when the
   process disappears. Currently, a process' perfctr state lives
   as long as references remains, whether via the process task_struct,
   or via some open file descriptor. One use for this is in
   remote-control applications, where it avoids enforcing
   parent/child relationships on monitor/target processes.

Going with a mini-fs is possible (though painful to implement), but
the remote-control feature would have to be constrained to a debugger
like model. In particular, the whole notion of using an fd as a handle
to a state object with its own lifetime would have to be ditched.

/Mikael