2.5.43 CSA, Job, and PAGG

2.5.43 versions of CSA, Job, and PAGG patches are available at:

    ftp://oss.sgi.com/projects/pagg/download/linux-2.5.43-pagg-job.patch
    ftp://oss.sgi.com/projects/csa/download/linux-2.5.43-csa.patch

The CSA and job user-level code is in the same directories.

CSA (Comprehensive System Accounting) provides methods for collecting per-process resource usage data, monitoring disk usage, and charging fees to specific login accounts. CSA provides features which are not available with the other Linux accounting packages. For more information, see:
http://oss.sgi.com/projects/csa/

Linux Jobs is an inescapable process container that is typically created by point of entry processes like login, and inherited by children. PAGG (Process Aggregates) is a generic framework for implementing process containers such as Linux Jobs. For more information, see:
http://oss.sgi.com/projects/pagg/

CSA depends on Linux Jobs, and Linux Jobs depends on PAGG.

diffstat linux-2.5.43-pagg-job.patch

 Documentation/job.txt     |  436 +++++++++
 Documentation/pagg.txt    |  151 +++
 include/linux/init_task.h |    4 
 include/linux/job.h       |  261 +++++
 include/linux/jobctl.h    |  167 +++
 include/linux/pagg.h      |  242 +++++
 include/linux/sched.h     |    4 
 init/Config.help          |   28 
 init/Config.in            |    4 
 kernel/Makefile           |   10 
 kernel/exit.c             |   15 
 kernel/fork.c             |   11 
 kernel/job.c              | 2023 ++++++++++++++++++++++++++++++++++++++++++++++
 kernel/pagg.c             |  418 +++++++++
 14 files changed, 3768 insertions(+), 6 deletions(-)

diffstat linux-2.5.43-csa.patch

 drivers/block/ll_rw_blk.c    |    4 
 fs/exec.c                    |    4 
 fs/read_write.c              |   18 
 include/linux/csa.h          |  522 ++++++++++++++
 include/linux/csa_internal.h |   87 ++
 include/linux/sched.h        |   20 
 init/Config.help             |   19 
 init/Config.in               |    1 
 kernel/Makefile              |    1 
 kernel/csa_job_acct.c        | 1586 +++++++++++++++++++++++++++++++++++++++++++
 kernel/exit.c                |    7 
 kernel/fork.c                |    7 
 kernel/ksyms.c               |    4 
 mm/memory.c                  |   18 
 mm/mmap.c                    |    9 
 mm/mremap.c                  |    8 
 mm/rmap.c                    |    3 
 mm/swapfile.c                |    4 
 18 files changed, 2319 insertions(+), 3 deletions(-)

John Hesterberg and Robin Holt

The first two lines in the csa patch contain an obvious jiffy-wrap bug.

| Dave Jones.        http://www.codemonkey.org.uk

Fixed. The patch file named above is now a link to linux-2.5.43_002-csa.patch. The old file is linux-2.5.43_001-csa.patch

If you could re-review, I would appreciate it.

Thanks,
Robin Holt

Casting it to a ulong won't help you.
Imagine jiffies begins at 0xffffffff

	unsigned long start_wait = jiffies;
	...
	current->bwtime += (unsigned long) jiffies - start_wait;

and when you read it the second time, it's rolled over to 0x00000001

You now increment bwtime by $BIGNUM

| Dave Jones.        http://www.codemonkey.org.uk

Comments:

diff -Naur /dev/null job25p/include/linux/job.h

+/*
+ * Description: This file, include/linux/job.h, contains the data
+ * structure definitions and functions prototypes used
+ * by other kernel bits that communicate with the job
+ * module. One such example is Comprehensive System
+ * Accounting (CSA).
+ *
+ * History:
+ *
+ * 2000.07.15 Sam Watters <[email protected]>
+ * created
+ * 2001.01.30 Sam Watters <[email protected]>
+ * Moved file to include/linux/job.h
+ */

I don't think we need this pre-merging history :)

+/*
+ * Define ioctl request structures for job module
+ */
+
+typedef struct job_create_s {
+ uint64_t r_jid; /* Return value of JID */
+ uint64_t jid; /* Jid value requested */
+ uid_t user; /* UID of user associated with job */
+ int options;/* creation options - unused */
+} job_create_t;

(all those comments apply for the other structs either)

Don't use typedefs or _s struct - linus just explained the issue to Ingo recently. You shouldn't use uid_t in a kernel/userspace interface. And use uXX/sXX types in core kernel code.

It should look something like this:

struct job_create_s {
	__u64	r_jid;	/* Return value of JID */
	__u64	jid;	/* Jid value requested */
	__u32 	user;	/* UID of user associated with job */
	__s32 	options;/* creation options - unused */
};

+/*
+ * The job start/stop events: These will identify the
+ * the reason the do_jobstart and do_jobend callbacks are being
+ * called.
+ */
+#define JOB_EVENT_IGNORE 0
+#define JOB_EVENT_START 1
+#define JOB_EVENT_RESTART 2
+#define JOB_EVENT_END 3

enum?

+/*
+ * Subscriber type: Each module that registers as a accounting data
+ * "subscriber" has to have a type. This type will identify the
+ * the appropriate structs and macros to use when exchanging data.
+ */
+#define JOB_ACCT_CSA 0
+#define JOB_ACCT_COUNT 1 /* Number of entries available */
+
+#if defined(CONFIG_CSA_JOB_ACCT) || defined(CONFIG_CSA_JOB_ACCT_MODULE)

no need for ifdefs to just protect struct definitions.

+#ifdef __KERNEL__
+int job_register_acct(job_acctmod_t *);
+int job_unregister_acct(job_acctmod_t *);
+uint64_t job_getjid(struct task_struct *);
+int job_getacct(uint64_t, int, void *);
+int job_setacct(uint64_t, int, int, void *);
+#endif

Remove the __KERNEL__ - kernel headers shouldn't be included by userspace anyway.

+ *     do_paggctl:     Function pointer to paggctl() system call handler
+ *                     for the pagg module.  This is optional and may be set
+ *                     to NULL if it is not needed by the pagg module.

doesn't seem to be actually implemented..

+struct pagg_hook_s {

+       /* Name Key  - restricted to 32 characters */
+       char                    *name;
+       /* Function pointers */
+       int                     (*do_attach)(struct task_struct *, 

+ struct pagg_s *,
+ void *);
+ int (*do_detach)(struct task_struct *,
+ struct pagg_s *);
+ int (*do_init)(struct task_struct *,
+ struct pagg_s *);

Please remove the do_ prefixes.

+       /* Opaque module specific data */
+       void                    *data;
+       /* Module reference */
+       struct module           *module;

Maybe the first entry as in file_operations, etc..?

+ /* Up the write semaphore for the task's pagg_list */
+#define write_unlock_pagg_list(t) up_write(&t->pagg_list.sem)
+
+
+
+
+
+#else /* CONFIG_PAGG */

Maybe a few whitespaces less? :)

diff -Naur 2.5.42/include/linux/sched.h job25p/include/linux/sched.h
@@ -29,6 +29,7 @@
 #include <linux/compiler.h>
 #include <linux/completion.h>
 #include <linux/pid.h>
+#include <linux/pagg.h>
 
 struct exec_domain;
 

@@ -401,6 +402,9 @@
 	void *journal_info;
 	struct dentry *proc_dentry;
 	struct backing_dev_info *backing_dev_info;
+
+/* List of pagg (process aggregate) attachments */
+	struct pagg_list_s pagg_list;
 };
 

You don't need the header for an unreferences structure pointer. And sched.h already includes far to many other headers..

diff -Naur 2.5.42/kernel/exit.c job25p/kernel/exit.c
@@ -19,6 +19,8 @@
 #include <linux/file.h>
 #include <linux/binfmts.h>
 #include <linux/ptrace.h>
+#include <linux/profile.h>
 

What do you need this for?

+#include <linux/pagg.h>

#include <asm/uaccess.h>
#include <asm/pgtable.h>
@@ -59,11 +61,12 @@
{
struct dentry *proc_dentry;
task_t *leader;
-
- if (p->state < TASK_ZOMBIE)
- BUG();
+
+ BUG_ON(p->state < TASK_ZOMBIE);
+
if (p != current)
wait_task_inactive(p);
+

Change looks unrelated

@@ -635,6 +638,8 @@
current->comm, current->pid, preempt_count());

+ profile_exit_task(tsk);
+
fake_volatile:
acct_process(code);
__exit_mm(tsk);

Change look unrelated.

@@ -653,6 +658,10 @@
__MOD_DEC_USE_COUNT(tsk->binfmt->module);

tsk->exit_code = code;
+
+ /* call pagg modules to detach from any attached process aggregates */
+ detach_pagg_list_chk(tsk);
+

Comments seems rather superflous.

@@ -173,6 +174,9 @@

init_task.rlim[RLIMIT_NPROC].rlim_cur = max_threads/2; init_task.rlim[RLIMIT_NPROC].rlim_max = max_threads/2;
+
+ /* Initialize the pagg list in pid 0 before it can clone itself. */
+ INIT_PAGG_LIST(&current->pagg_list);

Use a tab instead of eight spaces please.

+/* Host ID for the localhost */
+static uint32_t jid_hid = DISABLED;
+
+static char *hid = NULL;
+MODULE_PARM(hid, "s");

What's this for?

+
+/* Function prototypes */
+int job_sys_create(job_create_t *);
+int job_sys_getjid(job_getjid_t *);
+int job_sys_waitjid(job_waitjid_t *);
+int job_sys_killjid(job_killjid_t *);
+int job_sys_getjidcnt(job_jidcnt_t *);
+int job_sys_getjidlst(job_jidlst_t *);
+int job_sys_getpidcnt(job_pidcnt_t *);
+int job_sys_getpidlst(job_pidlst_t *);
+int job_sys_getuser(job_user_t *);
+int job_sys_getprimepid(job_primepid_t *);
+int job_sys_sethid(job_sethid_t *);
+int job_sys_detachjid(job_detachjid_t *);
+int job_sys_detachpid(job_detachpid_t *);
+int job_attach(struct task_struct *, struct pagg_s *, void *); +int job_detach(struct task_struct *, struct pagg_s *); +job_entry_t *job_getjob(uint64_t jid);

Make static?

+/* Job container kernel pagg entry */
+static struct pagg_hook_s pagg_hook = {
+ PAGG_JOB,
+ job_attach,
+ job_detach,
+ NULL,
+ &job_table,
+ THIS_MODULE,
+ LIST_HEAD_INIT(pagg_hook.entry)
+};

Use named intializers?

+#if defined(CONFIG_CSA_JOB_ACCT) || defined(CONFIG_CSA_JOB_ACCT_MODULE)
+ /* - CSA accounting */
+ if (acct_list[JOB_ACCT_CSA]) {
+ job_acctmod_t *acct = acct_list[JOB_ACCT_CSA];
+ if (try_inc_mod_count(acct->module)) {
+ if (acct->do_jobend) {
+ int res = 0;
+ job_csa_t csa;

Shouldn't this use a proper interface instead of the ugly conditional?

+/*
+ * init_module
+ *
+ * This function is called when a module is inserted into a kernel. This
+ * function allocates any necessary structures and sets initial values for
+ * module data.
+ *
+ * If the function succeeds, then 0 is returned. On failure, -1 is returned.
+ */
+static int __init
+init_job(void)
+{
+ int i,rc;
+
+
+ /* Initialize the job table chains */
+ for (i = 0; i < HASH_SIZE; i++) {
+ INIT_LIST_HEAD(&job_table[i]);
+ }
+
+ /* Initialize the list for accounting subscribers */
+ for (i = 0; i < JOB_ACCT_COUNT; i++) {
+ acct_list[i] = NULL;
+ }

It's in .bss, no need to initialize to zero.

+ /* Setup our /proc entry file */
+ job_proc_entry = create_proc_entry(JOB_PROC_ENTRY,
+ S_IFREG | S_IRUGO, &proc_root);
+
+ if (!job_proc_entry) {
+ unregister_pagg_hook(&pagg_hook);
+ return -1;
+ }
+
+ job_proc_entry->proc_fops = &job_file_ops;
+ job_proc_entry->proc_iops = NULL;

Umm, no, ioctl on procfs is not the proper interfaces. It looks like all ioctl subcalls were syscalls initially (on unicos?), so doing the as actual syscalls would at least be better. Defining a proper ascii file interface (i.e. echo start <arg1> <arg2> <etc.. > file) sounds better.

+#include <linux/config.h>
+
+#if defined(CONFIG_PAGG)

Just compile the file conditional on CONFIG_PAGG instead.

Christoph,
Thanks for your comments!
I'm making all the changes, except the items noted below, which might need more discussion.

It's not a structure pointer, it's the structure itself.

A host id (hid) is used as the top 32 bits of the job id. It gets initialized when you load the module, and can be changed with the JOB_SETHID call.

We agree, but haven't had the time to fix this yet.

On IRIX, where this is ported from, this is how CSA is hooked into Job. When it was ported over, they kept it the same way.

What we *really* want would be a new syscall, but we can't lock down the syscall number in an external patch. If we get integrated by Linus, then we'd love to cut over to a new syscall. For now, the ioctl on /proc behaves almost identically to how a new syscall would, in that we pass down arguments in binary format, can copyout results, etc. This is what the code on both ends is expecting.

I don't like the ascii file interface in this case, as it would require the code on both sides to munge the arguments to/from ascii, and would break the interfaces that are not just passing in arguments.

John

Umm, of course your right - sorry for the thinko (seeo? :))

Any reason it's a char, not some interger?

Well, no. It's not one syscall but many - and we really don't like adding new multiplexer syscalls to linux, even less then normal ones.

You might want to look at fs/nfsd/nfsctl.c in 2.5 for a proper fs-based interface for this.

Comments:

diff -Naur job25p-linux/fs/read_write.c csa2.5p-linux/fs/read_write.c
@@ -209,8 +209,11 @@
 				ret = file->f_op->read(file, buf, count, pos);
 			else
 				ret = do_sync_read(file, buf, count, pos);
-			if (ret > 0)
+			if (ret > 0) {
 				dnotify_parent(file->f_dentry, DN_ACCESS);
+				current->rchar += ret;
+			}
+			current->syscr++;
 

You miss e.g. AIO, sendfile/sendmsg/recmsg when updating current->syscr.

+#ifndef _LINUX_CSA_H
+#define _LINUX_CSA_H
+
+#ifndef __KERNEL__
+#include <stdint.h>
+#include <sys/types.h>
+#endif

Umm, userspace shouldn't include kernel headers again.. Also for the types the same comments as for the pagg patch applies.

+
+/*
+ * accounting flags per-process
+ */
+#define AFORK 0x01 /* fork, but did not exec */
+#define ASU 0x02 /* super-user privileges */
+#define ACKPT 0x04 /* process has been checkpointed */
+#define ACORE 0x08 /* produced corefile */
+#define AXSIG 0x10 /* killed by a signal */
+#define AMORE 0x20 /* more CSA acct records for this process */
+#define AINC 0x40 /* incremental accounting record */

enum?

+ * Job Accounting for Linux
+ *
+ * This header file contains the definitions needed for communication
+ * between the kernel and the CSA module.
+ */
+
+#ifndef _LINUX_CSA_INTERNAL_H
+#define _LINUX_CSA_INTERNAL_H
+
+#include <linux/config.h>
+
+#if defined (CONFIG_CSA_JOB_ACCT) || defined (CONFIG_CSA_JOB_ACCT_MODULE)

Umm, stubbing stuff out based on _MODULE is a bad, bad idea. Just make it a bool instead.

diff -Naur job25p-linux/include/linux/sched.h csa2.5p-linux/include/linux/sched.h
@@ -214,6 +214,8 @@
 	struct kioctx		*ioctx_list;
 
 	struct kioctx		default_kioctx;
+
+	unsigned long hiwater_rss, hiwater_vm;
 

Make conditional on CONFIG_CSA?

};

extern int mmlist_nr;
@@ -405,6 +407,11 @@

/* List of pagg (process aggregate) attachments */
struct pagg_list_s pagg_list;
+
+/* i/o counters(bytes read/written, blocks read/written, #syscalls, waittime */
+ unsigned long rchar, wchar, rblk, wblk, syscr, syscw, bwtime;
+ unsigned long csa_rss_mem1, csa_vm_mem1;
+ clock_t csa_stimexpd;

Move into a sub-structure and make conditional on CONFIG_CSA?

+int csa_jstart(int, void *);
+int csa_jexit(int, void *);
+int csa_ioctl(struct inode *, struct file *, unsigned int,
+ unsigned long);
+void csa_acct_eop(int, struct task_struct *);

All static?

+static int csa_modify_buf(char *, struct acctcsa *, struct acctmem *,
+ struct acctio *, int, int);
+static int csa_write(char *, int, int, uint64_t, int, job_csa_t *);
+static void csa_config_make(ac_eventtype, struct acctcfg *);
+static int csa_config_write(ac_eventtype,struct file *);
+static void csa_header(struct achead *, int, int, int);
+static long int sc_CLK(long int);

+#if defined __ia64__
+#define JID_ERR1 "do_csa_acct: No job table entry for jid 0x%lx.\n" +#define JID_ERR2 "csa user job accounting write error %d, jid 0x%lx\n" +#define JID_ERR3 "Can't disable csa user job accounting jid 0x%lx\n" +#define JID_ERR4 "csa user job accounting disabled, jid 0x%lx\n" +#else
+#define JID_ERR1 "do_csa_acct: No job table entry for jid 0x%llx.\n" +#define JID_ERR2 "csa user job accounting write error %d, jid 0x%llx\n" +#define JID_ERR3 "Can't disable csa user job accounting jid 0x%llx\n" +#define JID_ERR4 "csa user job accounting disabled, jid 0x%llx\n" +#endif

Umm, this would be #if __LP64__. But please just always cast to long long instead.

+static struct file *csa_acctvp = (struct file *)NULL;
+static time_t boottime = 0;

No need to inintialize stuff in .bss.

+static int csa_flag = 0; /* accounting start state flag */
+char csa_path[ACCT_PATH] = ""; /* current accounting file path name */
+char new_path[ACCT_PATH] = ""; /* new accounting file path name */

Again..

+static job_acctmod_t csa_job_callbacks = {
+ JOB_ACCT_CSA,
+ csa_jstart,
+ csa_jexit,
+ THIS_MODULE
+};

named initializers..

+static int __init
+init_csa(void)
+{
+ int retval = 0;
+
+ /*
+ * Create the /proc entries used to communicate/control this
+ * module.
+ */
+ csa_proc_entry = create_proc_entry(CSA_PROC_ENTRY,
+ S_IFREG | S_IRUGO, &proc_root);
+
+ if (!csa_proc_entry) {
+ return -1;
+ }
+
+ csa_proc_entry->proc_fops = &csa_file_ops;
+ csa_proc_entry->proc_iops = NULL;

Again the interface is rather horrible.

__LP64__ is a HP/IA64ism, which is not necessarily defined on other 64bit platforms. The proper way to test for 64bit in the kernel is

#include <linux/types.h>

...

#if BITS_PER_LONG==64

#endif

-Andi

I did the cast to long long instead. Cleans things up nicely.

Robin

The construct #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE) is required for all CONFIG_FOO which can be defined as tristate.

.config Source code
CONFIG_FOO CONFIG_FOO CONFIG_FOO_MODULE
n undef undef
y 1 undef
m undef 1

If CSA_JOB_ACCT can be a module or builtin then the code is correct. OTOH, if CSA_JOB_ACCT is a boolean subset of CSA then it should be a bool, not a tristate. Given

if [ "$CONFIG_PAGG" = "y" ]; then
tristate ' Process aggregate based jobs' CONFIG_PAGG_JOB
dep_tristate ' CSA Job Accounting' CONFIG_CSA_JOB_ACCT $CONFIG_PAGG_JOB
fi

and assuming that CONFIG_PAGG_JOB=y, CONFIG_CSA_JOB_ACCT=m is a valid combination, then the code is correct.

Keith, I know that it works. Which doesn't make it a good idea.

With the current config system you have to code that way if FOO can be built in or a module. What do you mean by this?

With
dep_tristate ' CSA Job Accounting' CONFIG_CSA_JOB_ACCT $CONFIG_PAGG_JOB
there is no single bool which defines if CONFIG_CSA_JOB_ACCT is set or not in .config.

There is even source code that needs to know if CONFIG_FOO was set to y or m, that code needs the existing separation between CONFIG_FOO and CONFIG_FOO_MODULE.

No. That code needs to be fixed so it doesn't ever look at CONFIG_FOO_MODULE.

--
dwmw2