[PATCH] 2.5.43 CSA, Job, and PAGG

[PATCH] 2.5.43 CSA, Job, and PAGG

[John Hesterberg]

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

Re: [PATCH] 2.5.43 CSA, Job, and PAGG

[Dave Jones]

On Thu, Oct 17, 2002 at 10:21:47AM -0500, John Hesterberg wrote:
 > 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 first two lines in the csa patch contain an obvious jiffy-wrap bug.

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

Re: [PATCH] 2.5.43 CSA, Job, and PAGG

[Robin Holt]

On Thu, 17 Oct 2002, Dave Jones wrote:

> On Thu, Oct 17, 2002 at 10:21:47AM -0500, John Hesterberg wrote:
>  > 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 first two lines in the csa patch contain an obvious jiffy-wrap bug.

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

Re: [PATCH] 2.5.43 CSA, Job, and PAGG

[Dave Jones]

On Thu, Oct 17, 2002 at 12:50:52PM -0500, Robin Holt wrote:

 > > The first two lines in the csa patch contain an obvious jiffy-wrap bug.
 > 
 > 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.

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

Re: [PATCH] 2.5.43 CSA, Job, and PAGG

[Christoph Hellwig]

On Thu, Oct 17, 2002 at 10:21:47AM -0500, John Hesterberg wrote:
> 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/

Comments:


diff -Naur /dev/null job25p/include/linux/job.h
--- /dev/null	Thu Aug 30 20:30:55 2001
+++ job25p/include/linux/job.h	Wed Oct 16 00:09:38 2002
+/*
+ * 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 <watters@sgi.com>
+ * 		created
+ * 2001.01.30	Sam Watters <watters@sgi.com>
+ * 		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
--- 2.5.42/include/linux/sched.h	Tue Oct  8 17:52:35 2002
+++ job25p/include/linux/sched.h	Wed Oct 16 15:38:31 2002
@@ -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
--- 2.5.42/kernel/exit.c	Wed Oct  2 15:14:53 2002
+++ job25p/kernel/exit.c	Wed Oct 16 15:39:32 2002
@@ -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.

Re: [PATCH] 2.5.43 CSA, Job, and PAGG

[John Hesterberg]

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

On Thu, Oct 17, 2002 at 10:33:28PM -0400, Christoph Hellwig wrote:
> diff -Naur 2.5.42/include/linux/sched.h job25p/include/linux/sched.h
> --- 2.5.42/include/linux/sched.h	Tue Oct  8 17:52:35 2002
> +++ job25p/include/linux/sched.h	Wed Oct 16 15:38:31 2002
> @@ -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..

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

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

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.

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

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.

> 
> +	/* 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.

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

Re: [PATCH] 2.5.43 CSA, Job, and PAGG

[Christoph Hellwig]

On Fri, Oct 18, 2002 at 02:05:46PM -0500, John Hesterberg wrote:
> > +
> > +/* 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..
> 
> It's not a structure pointer, it's the structure itself.

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

> > +/* Host ID for the localhost */
> > +static uint32_t   jid_hid = DISABLED;
> > +
> > +static char 	   *hid = NULL;	    
> > +MODULE_PARM(hid, "s");
> > 
> > What's this for?
> 
> 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.

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

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

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.

Re: [PATCH] 2.5.43 CSA, Job, and PAGG

[Christoph Hellwig]

On Thu, Oct 17, 2002 at 10:21:47AM -0500, John Hesterberg wrote:
> 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/

Comments:


diff -Naur job25p-linux/fs/read_write.c csa2.5p-linux/fs/read_write.c
--- job25p-linux/fs/read_write.c	Mon Oct 14 14:09:27 2002
+++ csa2.5p-linux/fs/read_write.c	Wed Oct 16 17:05:13 2002
@@ -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
--- job25p-linux/include/linux/sched.h	Wed Oct 16 17:23:22 2002
+++ csa2.5p-linux/include/linux/sched.h	Wed Oct 16 17:15:39 2002
@@ -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.

Re: [PATCH] 2.5.43 CSA, Job, and PAGG

[Andi Kleen]

Christoph Hellwig <hch@sgi.com> writes:

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

__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

Re: [PATCH] 2.5.43 CSA, Job, and PAGG

[Robin Holt]

On 18 Oct 2002, Andi Kleen wrote:

> Christoph Hellwig <hch@sgi.com> writes:
> 
> > 
> > +#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.
> 
> __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

Re: [PATCH] 2.5.43 CSA, Job, and PAGG

[Keith Owens]

On Thu, 17 Oct 2002 22:44:10 -0400, 
Christoph Hellwig <hch@sgi.com> wrote:
>On Thu, Oct 17, 2002 at 10:21:47AM -0500, John Hesterberg wrote:
>> 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
>
>+#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.

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.

Re: [PATCH] 2.5.43 CSA, Job, and PAGG

[Christoph Hellwig]

On Sat, Oct 19, 2002 at 12:37:50AM +1000, Keith Owens wrote:
> On Thu, 17 Oct 2002 22:44:10 -0400, 
> Christoph Hellwig <hch@sgi.com> wrote:
> >On Thu, Oct 17, 2002 at 10:21:47AM -0500, John Hesterberg wrote:
> >> 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
> >
> >+#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.
> 
> The construct #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE) is
> required for all CONFIG_FOO which can be defined as tristate.

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

Re: [PATCH] 2.5.43 CSA, Job, and PAGG

[Keith Owens]

On Fri, 18 Oct 2002 18:00:31 -0400, 
Christoph Hellwig <hch@sgi.com> wrote:
>On Sat, Oct 19, 2002 at 12:37:50AM +1000, Keith Owens wrote:
>> The construct #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE) is
>> required for all CONFIG_FOO which can be defined as tristate.
>
>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?

>> On Thu, 17 Oct 2002 22:44:10 -0400, 
>> Christoph Hellwig <hch@sgi.com> wrote:
>> >+#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.

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.

Re: [PATCH] 2.5.43 CSA, Job, and PAGG

[David Woodhouse]

kaos@ocs.com.au said:
>  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