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

Christoph Hellwig
2002-10-17T19:24:02Z

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.