LinuxLists.cc - [PATCH] 2.5.43 CSA, Job, and PAGG

2002-10-17 15:15:55

Subject: [PATCH] 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

2002-10-17 16:13:27

by Dave Jones

[permalink] [raw]

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

2002-10-17 17:45:08

by Robin Holt

[permalink] [raw]

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

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

2002-10-17 18:44:16

by Dave Jones

[permalink] [raw]

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

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

2002-10-17 19:13:26

by Christoph Hellwig

[permalink] [raw]

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

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

2002-10-17 19:24:02

by Christoph Hellwig

[permalink] [raw]

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

2002-10-18 01:01:19

by Andi Kleen

[permalink] [raw]

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

Christoph Hellwig <[email protected]> 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

2002-10-18 01:17:24

by Robin Holt

[permalink] [raw]

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

On 18 Oct 2002, Andi Kleen wrote:

> Christoph Hellwig <[email protected]> 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

2002-10-18 14:32:03

by Keith Owens

[permalink] [raw]

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

On Thu, 17 Oct 2002 22:44:10 -0400,
Christoph Hellwig <[email protected]> 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.

2002-10-18 14:41:01

by Christoph Hellwig

[permalink] [raw]

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

On Sat, Oct 19, 2002 at 12:37:50AM +1000, Keith Owens wrote:
> On Thu, 17 Oct 2002 22:44:10 -0400,
> Christoph Hellwig <[email protected]> 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.

2002-10-18 15:38:00

by Keith Owens

[permalink] [raw]

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

On Fri, 18 Oct 2002 18:00:31 -0400,
Christoph Hellwig <[email protected]> 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 <[email protected]> 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.

2002-10-18 16:13:21

by David Woodhouse

[permalink] [raw]

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

[email protected] 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

2002-10-18 19:02:14

by John Hesterberg

[permalink] [raw]

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

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

2002-10-21 12:22:22

by Christoph Hellwig

[permalink] [raw]

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

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.