2005-05-09 08:08:11

by Guillaume Thouvenin

[permalink] [raw]
Subject: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector

Hello,

This patch adds a fork connector in the do_fork() routine. It sends a
netlink datagram when enabled. The message can be read by a user space
application. By this way, the user space application is alerted when a
fork occurs. Informations sent by the kernel are the processor ID where
the fork occurred, the pid of the parent and the pid of the child.

It uses the userspace <-> kernelspace connector that works on top of
the netlink protocol. The fork connector can be enable or disable by
sending a message to the connector.

The fork connector is used by the Enhanced Linux System Accounting
project http://elsa.sourceforge.net

This patch applies to 2.6.12-rc3-mm3

Best Regards,
Guillaume

Signed-off-by: Guillaume Thouvenin <[email protected]>

---

drivers/connector/Kconfig | 11 ++++
drivers/connector/Makefile | 1
drivers/connector/cn_fork.c | 114 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/cn_fork.h | 86 +++++++++++++++++++++++++++++++++
include/linux/connector.h | 2
kernel/fork.c | 11 ++++
6 files changed, 225 insertions(+)


Index: linux-2.6.12-rc3-mm3/drivers/connector/Kconfig
===================================================================
--- linux-2.6.12-rc3-mm3.orig/drivers/connector/Kconfig 2005-05-09 07:45:52.000000000 +0200
+++ linux-2.6.12-rc3-mm3/drivers/connector/Kconfig 2005-05-09 08:03:15.000000000 +0200
@@ -10,4 +10,15 @@ config CONNECTOR
Connector support can also be built as a module. If so, the module
will be called cn.ko.

+config FORK_CONNECTOR
+ bool "Enable fork connector"
+ select CONNECTOR
+ default y
+ ---help---
+ It adds a connector in kernel/fork.c:do_fork() function. When a fork
+ occurs, netlink is used to transfer information about the parent and
+ its child. This information can be used by a user space application.
+ The fork connector can be enable/disable by sending a message to the
+ connector with the corresponding group id.
+
endmenu
Index: linux-2.6.12-rc3-mm3/drivers/connector/Makefile
===================================================================
--- linux-2.6.12-rc3-mm3.orig/drivers/connector/Makefile 2005-05-09 07:45:52.000000000 +0200
+++ linux-2.6.12-rc3-mm3/drivers/connector/Makefile 2005-05-09 08:03:15.000000000 +0200
@@ -1,2 +1,3 @@
obj-$(CONFIG_CONNECTOR) += cn.o
+obj-$(CONFIG_FORK_CONNECTOR) += cn_fork.o
cn-objs := cn_queue.o connector.o
Index: linux-2.6.12-rc3-mm3/drivers/connector/cn_fork.c
===================================================================
--- linux-2.6.12-rc3-mm3.orig/drivers/connector/cn_fork.c 2003-01-30 11:24:37.000000000 +0100
+++ linux-2.6.12-rc3-mm3/drivers/connector/cn_fork.c 2005-05-09 08:03:15.000000000 +0200
@@ -0,0 +1,114 @@
+/*
+ * cn_fork.c - Fork connector
+ *
+ * Copyright (C) 2005 BULL SA.
+ * Written by Guillaume Thouvenin <[email protected]>
+ *
+ * This module implements the fork connector. It allows to send a
+ * netlink datagram, when enabled, from the do_fork() routine. The
+ * message can be read by a user space application. By this way,
+ * the user space application is alerted when a fork occurs.
+ *
+ * It uses the userspace <-> kernelspace connector that works on top of
+ * the netlink protocol. The fork connector is enabled or disabled by
+ * sending a message to the connector. The unique sequence number of
+ * messages can be used to check if a message is lost.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+
+#include <linux/cn_fork.h>
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Guillaume Thouvenin <[email protected]>");
+MODULE_DESCRIPTION("Enable or disable the usage of the fork connector");
+
+int cn_fork_enable = 0;
+struct cb_id cb_fork_id = { CN_IDX_FORK, CN_VAL_FORK };
+
+static inline void cn_fork_send_status(void)
+{
+ /* TODO: An informational line in log is maybe not enough... */
+ printk(KERN_INFO "cn_fork_enable == %d\n", cn_fork_enable);
+}
+
+/**
+ * cn_fork_callback - enable or disable the fork connector
+ * @data: message send by the connector
+ *
+ * The callback allows to enable or disable the sending of information
+ * about fork in the do_fork() routine. To enable the fork, the user
+ * space application must send the integer 1 in the data part of the
+ * message. To disable the fork connector, it must send the integer 0.
+ */
+static void cn_fork_callback(void *data)
+{
+ struct cn_msg *msg = data;
+ int action;
+
+ if (cn_already_initialized && (msg->len == sizeof(cn_fork_enable))) {
+ memcpy(&action, msg->data, sizeof(cn_fork_enable));
+ switch(action) {
+ case FORK_CN_START:
+ cn_fork_enable = 1;
+ break;
+ case FORK_CN_STOP:
+ cn_fork_enable = 0;
+ break;
+ case FORK_CN_STATUS:
+ cn_fork_send_status();
+ break;
+ }
+ }
+}
+
+/**
+ * cn_fork_init - initialization entry point
+ *
+ * This routine will be run at kernel boot time because this driver is
+ * built in the kernel. It adds the connector callback to the connector
+ * driver.
+ */
+static int cn_fork_init(void)
+{
+ int err;
+
+ err = cn_add_callback(&cb_fork_id, "cn_fork", &cn_fork_callback);
+ if (err) {
+ printk(KERN_WARNING "Failed to register cn_fork\n");
+ return -EINVAL;
+ }
+
+ printk(KERN_NOTICE "cn_fork is registered\n");
+ return 0;
+}
+
+/**
+ * cn_fork_exit - exit entry point
+ *
+ * As this driver is always statically compiled into the kernel the
+ * cn_fork_exit has no effect.
+ */
+static void cn_fork_exit(void)
+{
+ cn_del_callback(&cb_fork_id);
+}
+
+module_init(cn_fork_init);
+module_exit(cn_fork_exit);
Index: linux-2.6.12-rc3-mm3/include/linux/cn_fork.h
===================================================================
--- linux-2.6.12-rc3-mm3.orig/include/linux/cn_fork.h 2003-01-30 11:24:37.000000000 +0100
+++ linux-2.6.12-rc3-mm3/include/linux/cn_fork.h 2005-05-09 09:50:28.000000000 +0200
@@ -0,0 +1,86 @@
+/*
+ * cn_fork.h - Fork connector
+ *
+ * Copyright (C) 2005 Nguyen Anh Quynh <[email protected]>
+ * Copyright (C) 2005 Guillaume Thouvenin <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef CN_FORK_H
+#define CN_FORK_H
+
+#include <linux/connector.h>
+
+#define FORK_CN_STOP 0
+#define FORK_CN_START 1
+#define FORK_CN_STATUS 2
+
+struct cn_fork_msg
+{
+ int cpu; /* ID of the cpu where the fork occured */
+ pid_t ppid; /* the parent PID */
+ pid_t cpid; /* the child PID */
+};
+
+/* Code above is only inside the kernel */
+#ifdef __KERNEL__
+
+extern int cn_already_initialized;
+
+#ifdef CONFIG_FORK_CONNECTOR
+
+#define CN_FORK_INFO_SIZE sizeof(struct cn_fork_msg)
+#define CN_FORK_MSG_SIZE (sizeof(struct cn_msg) + CN_FORK_INFO_SIZE)
+
+extern int cn_fork_enable;
+extern struct cb_id cb_fork_id;
+
+DECLARE_PER_CPU(unsigned long, fork_counts);
+
+static inline void fork_connector(pid_t parent, pid_t child)
+{
+ if (cn_fork_enable) {
+ struct cn_msg *msg;
+ struct cn_fork_msg *forkmsg;
+ __u8 buffer[CN_FORK_MSG_SIZE];
+
+ msg = (struct cn_msg *)buffer;
+
+ memcpy(&msg->id, &cb_fork_id, sizeof(msg->id));
+
+ msg->ack = 0; /* not used */
+ msg->seq = get_cpu_var(fork_counts)++;
+
+ msg->len = CN_FORK_INFO_SIZE;
+ forkmsg = (struct cn_fork_msg *)msg->data;
+ forkmsg->cpu = smp_processor_id();
+ forkmsg->ppid = parent;
+ forkmsg->cpid = child;
+
+ put_cpu_var(fork_counts);
+
+ cn_netlink_send(msg, CN_IDX_FORK, GFP_ATOMIC);
+ }
+}
+#else
+static inline void fork_connector(pid_t parent, pid_t child)
+{
+ return;
+}
+#endif /* CONFIG_FORK_CONNECTOR */
+#endif /* __KERNEL__ */
+
+#endif /* CN_FORK_H */
Index: linux-2.6.12-rc3-mm3/include/linux/connector.h
===================================================================
--- linux-2.6.12-rc3-mm3.orig/include/linux/connector.h 2005-05-09 07:45:56.000000000 +0200
+++ linux-2.6.12-rc3-mm3/include/linux/connector.h 2005-05-09 09:50:01.000000000 +0200
@@ -26,6 +26,8 @@

#define CN_IDX_CONNECTOR 0xffffffff
#define CN_VAL_CONNECTOR 0xffffffff
+#define CN_IDX_FORK 0xfeed /* fork events */
+#define CN_VAL_FORK 0xbeef

/*
* Maximum connector's message size.
Index: linux-2.6.12-rc3-mm3/kernel/fork.c
===================================================================
--- linux-2.6.12-rc3-mm3.orig/kernel/fork.c 2005-05-09 07:45:56.000000000 +0200
+++ linux-2.6.12-rc3-mm3/kernel/fork.c 2005-05-09 08:03:15.000000000 +0200
@@ -41,6 +41,7 @@
#include <linux/profile.h>
#include <linux/rmap.h>
#include <linux/acct.h>
+#include <linux/cn_fork.h>

#include <asm/pgtable.h>
#include <asm/pgalloc.h>
@@ -63,6 +64,14 @@ DEFINE_PER_CPU(unsigned long, process_co

EXPORT_SYMBOL(tasklist_lock);

+#ifdef CONFIG_FORK_CONNECTOR
+/*
+ * fork_counts is used by the fork_connector() inline routine as
+ * the sequence number of the netlink message.
+ */
+static DEFINE_PER_CPU(unsigned long, fork_counts);
+#endif /* CONFIG_FORK_CONNECTOR */
+
int nr_processes(void)
{
int cpu;
@@ -1252,6 +1261,8 @@ long do_fork(unsigned long clone_flags,
if (unlikely (current->ptrace & PT_TRACE_VFORK_DONE))
ptrace_notify ((PTRACE_EVENT_VFORK_DONE << 8) | SIGTRAP);
}
+
+ fork_connector(current->pid, p->pid);
} else {
free_pidmap(pid);
pid = PTR_ERR(p);



2005-05-09 09:32:01

by Alexander Nyberg

[permalink] [raw]
Subject: Re: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector


> Index: linux-2.6.12-rc3-mm3/include/linux/cn_fork.h
> ===================================================================
> --- linux-2.6.12-rc3-mm3.orig/include/linux/cn_fork.h 2003-01-30 11:24:37.000000000 +0100
> +++ linux-2.6.12-rc3-mm3/include/linux/cn_fork.h 2005-05-09 09:50:28.000000000 +0200
> @@ -0,0 +1,86 @@
> +/*
> + * cn_fork.h - Fork connector
> + *
> + * Copyright (C) 2005 Nguyen Anh Quynh <[email protected]>
> + * Copyright (C) 2005 Guillaume Thouvenin <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#ifndef CN_FORK_H
> +#define CN_FORK_H
> +
> +#include <linux/connector.h>
> +
> +#define FORK_CN_STOP 0
> +#define FORK_CN_START 1
> +#define FORK_CN_STATUS 2
> +
> +struct cn_fork_msg
> +{
> + int cpu; /* ID of the cpu where the fork occured */
> + pid_t ppid; /* the parent PID */
> + pid_t cpid; /* the child PID */
> +};
> +
> +/* Code above is only inside the kernel */
> +#ifdef __KERNEL__
> +
> +extern int cn_already_initialized;
> +
> +#ifdef CONFIG_FORK_CONNECTOR
> +
> +#define CN_FORK_INFO_SIZE sizeof(struct cn_fork_msg)
> +#define CN_FORK_MSG_SIZE (sizeof(struct cn_msg) + CN_FORK_INFO_SIZE)
> +
> +extern int cn_fork_enable;
> +extern struct cb_id cb_fork_id;
> +
> +DECLARE_PER_CPU(unsigned long, fork_counts);
> +
> +static inline void fork_connector(pid_t parent, pid_t child)
> +{
> + if (cn_fork_enable) {
> + struct cn_msg *msg;
> + struct cn_fork_msg *forkmsg;
> + __u8 buffer[CN_FORK_MSG_SIZE];
> +
> + msg = (struct cn_msg *)buffer;
> +
> + memcpy(&msg->id, &cb_fork_id, sizeof(msg->id));
> +
> + msg->ack = 0; /* not used */
> + msg->seq = get_cpu_var(fork_counts)++;
> +
> + msg->len = CN_FORK_INFO_SIZE;
> + forkmsg = (struct cn_fork_msg *)msg->data;
> + forkmsg->cpu = smp_processor_id();
> + forkmsg->ppid = parent;
> + forkmsg->cpid = child;
> +
> + put_cpu_var(fork_counts);
> +
> + cn_netlink_send(msg, CN_IDX_FORK, GFP_ATOMIC);

Why is this GFP_ATOMIC?

> + }
> +}
> +#else
> +static inline void fork_connector(pid_t parent, pid_t child)
> +{
> + return;
> +}
> +#endif /* CONFIG_FORK_CONNECTOR */
> +#endif /* __KERNEL__ */
> +
> +#endif /* CN_FORK_H */
> Index: linux-2.6.12-rc3-mm3/include/linux/connector.h
> ===================================================================
> --- linux-2.6.12-rc3-mm3.orig/include/linux/connector.h 2005-05-09 07:45:56.000000000 +0200
> +++ linux-2.6.12-rc3-mm3/include/linux/connector.h 2005-05-09 09:50:01.000000000 +0200
> @@ -26,6 +26,8 @@
>
> #define CN_IDX_CONNECTOR 0xffffffff
> #define CN_VAL_CONNECTOR 0xffffffff
> +#define CN_IDX_FORK 0xfeed /* fork events */
> +#define CN_VAL_FORK 0xbeef
>
> /*
> * Maximum connector's message size.
> Index: linux-2.6.12-rc3-mm3/kernel/fork.c
> ===================================================================
> --- linux-2.6.12-rc3-mm3.orig/kernel/fork.c 2005-05-09 07:45:56.000000000 +0200
> +++ linux-2.6.12-rc3-mm3/kernel/fork.c 2005-05-09 08:03:15.000000000 +0200
> @@ -41,6 +41,7 @@
> #include <linux/profile.h>
> #include <linux/rmap.h>
> #include <linux/acct.h>
> +#include <linux/cn_fork.h>
>
> #include <asm/pgtable.h>
> #include <asm/pgalloc.h>
> @@ -63,6 +64,14 @@ DEFINE_PER_CPU(unsigned long, process_co
>
> EXPORT_SYMBOL(tasklist_lock);
>
> +#ifdef CONFIG_FORK_CONNECTOR
> +/*
> + * fork_counts is used by the fork_connector() inline routine as
> + * the sequence number of the netlink message.
> + */
> +static DEFINE_PER_CPU(unsigned long, fork_counts);
> +#endif /* CONFIG_FORK_CONNECTOR */
> +

The above should go into cn_fork.c

> int nr_processes(void)
> {
> int cpu;
> @@ -1252,6 +1261,8 @@ long do_fork(unsigned long clone_flags,
> if (unlikely (current->ptrace & PT_TRACE_VFORK_DONE))
> ptrace_notify ((PTRACE_EVENT_VFORK_DONE << 8) | SIGTRAP);
> }
> +
> + fork_connector(current->pid, p->pid);

Are you sure this is what you want? ->pid has a special meaning to the
kernel and doesn't necessarily mean the same to user-space, so I think
you want ->tgid here. If you look at sys_getpid() and sys_gettid()
you'll see what I mean.

> } else {
> free_pidmap(pid);
> pid = PTR_ERR(p);
>


2005-05-09 11:39:19

by Guillaume Thouvenin

[permalink] [raw]
Subject: Re: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector

On Mon, 2005-05-09 at 11:31 +0200, Alexander Nyberg wrote:
> > +static inline void fork_connector(pid_t parent, pid_t child)
> > +{
> > + if (cn_fork_enable) {
> > + struct cn_msg *msg;
> > + struct cn_fork_msg *forkmsg;
> > + __u8 buffer[CN_FORK_MSG_SIZE];
> > +
> > + msg = (struct cn_msg *)buffer;
> > +
> > + memcpy(&msg->id, &cb_fork_id, sizeof(msg->id));
> > +
> > + msg->ack = 0; /* not used */
> > + msg->seq = get_cpu_var(fork_counts)++;
> > +
> > + msg->len = CN_FORK_INFO_SIZE;
> > + forkmsg = (struct cn_fork_msg *)msg->data;
> > + forkmsg->cpu = smp_processor_id();
> > + forkmsg->ppid = parent;
> > + forkmsg->cpid = child;
> > +
> > + put_cpu_var(fork_counts);
> > +
> > + cn_netlink_send(msg, CN_IDX_FORK, GFP_ATOMIC);
>
> Why is this GFP_ATOMIC?

In the previous connector, cn_netlink_send(struct cn_msg *msg, u32
__groups) called alloc_skb(size, GFP_ATOMIC). Now a third parameter is
used with cn_netlink_send() in order to call alloc_skb(size, gfp_mask)
with a specific gfp_mask. So, I'm using GFP_ATOMIC as the third argument
to keep the same behavior.

> > + }
> > +}
> > +#else
> > +static inline void fork_connector(pid_t parent, pid_t child)
> > +{
> > + return;
> > +}
> > +#endif /* CONFIG_FORK_CONNECTOR */
> > +#endif /* __KERNEL__ */
> > +
> > +#endif /* CN_FORK_H */
> > Index: linux-2.6.12-rc3-mm3/include/linux/connector.h
> > ===================================================================
> > --- linux-2.6.12-rc3-mm3.orig/include/linux/connector.h 2005-05-09 07:45:56.000000000 +0200
> > +++ linux-2.6.12-rc3-mm3/include/linux/connector.h 2005-05-09 09:50:01.000000000 +0200
> > @@ -26,6 +26,8 @@
> >
> > #define CN_IDX_CONNECTOR 0xffffffff
> > #define CN_VAL_CONNECTOR 0xffffffff
> > +#define CN_IDX_FORK 0xfeed /* fork events */
> > +#define CN_VAL_FORK 0xbeef
> >
> > /*
> > * Maximum connector's message size.
> > Index: linux-2.6.12-rc3-mm3/kernel/fork.c
> > ===================================================================
> > --- linux-2.6.12-rc3-mm3.orig/kernel/fork.c 2005-05-09 07:45:56.000000000 +0200
> > +++ linux-2.6.12-rc3-mm3/kernel/fork.c 2005-05-09 08:03:15.000000000 +0200
> > @@ -41,6 +41,7 @@
> > #include <linux/profile.h>
> > #include <linux/rmap.h>
> > #include <linux/acct.h>
> > +#include <linux/cn_fork.h>
> >
> > #include <asm/pgtable.h>
> > #include <asm/pgalloc.h>
> > @@ -63,6 +64,14 @@ DEFINE_PER_CPU(unsigned long, process_co
> >
> > EXPORT_SYMBOL(tasklist_lock);
> >
> > +#ifdef CONFIG_FORK_CONNECTOR
> > +/*
> > + * fork_counts is used by the fork_connector() inline routine as
> > + * the sequence number of the netlink message.
> > + */
> > +static DEFINE_PER_CPU(unsigned long, fork_counts);
> > +#endif /* CONFIG_FORK_CONNECTOR */
> > +
>
> The above should go into cn_fork.c

I don't see why. It's used by fork_connector which is an inline routine
so, IMHO, 'fork_counts' must be defined here and declared in
include/linux/cn_fork.h

> > int nr_processes(void)
> > {
> > int cpu;
> > @@ -1252,6 +1261,8 @@ long do_fork(unsigned long clone_flags,
> > if (unlikely (current->ptrace & PT_TRACE_VFORK_DONE))
> > ptrace_notify ((PTRACE_EVENT_VFORK_DONE << 8) | SIGTRAP);
> > }
> > +
> > + fork_connector(current->pid, p->pid);
>
> Are you sure this is what you want? ->pid has a special meaning to the
> kernel and doesn't necessarily mean the same to user-space, so I think
> you want ->tgid here. If you look at sys_getpid() and sys_gettid()
> you'll see what I mean.

Yes, I think you're right. If I look the code of the BSD process
accounting they're using the field ->tgid to get the process ID. I fix
that.

Thank you very much for your comments,
Best regards,

Guillaume

2005-05-09 11:50:07

by Evgeniy Polyakov

[permalink] [raw]
Subject: Re: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector

On Mon, 09 May 2005 13:38:44 +0200
Guillaume Thouvenin <[email protected]> wrote:

> On Mon, 2005-05-09 at 11:31 +0200, Alexander Nyberg wrote:
> > > +static inline void fork_connector(pid_t parent, pid_t child)
> > > +{
> > > + if (cn_fork_enable) {
> > > + struct cn_msg *msg;
> > > + struct cn_fork_msg *forkmsg;
> > > + __u8 buffer[CN_FORK_MSG_SIZE];
> > > +
> > > + msg = (struct cn_msg *)buffer;
> > > +
> > > + memcpy(&msg->id, &cb_fork_id, sizeof(msg->id));
> > > +
> > > + msg->ack = 0; /* not used */
> > > + msg->seq = get_cpu_var(fork_counts)++;
> > > +
> > > + msg->len = CN_FORK_INFO_SIZE;
> > > + forkmsg = (struct cn_fork_msg *)msg->data;
> > > + forkmsg->cpu = smp_processor_id();
> > > + forkmsg->ppid = parent;
> > > + forkmsg->cpid = child;
> > > +
> > > + put_cpu_var(fork_counts);
> > > +
> > > + cn_netlink_send(msg, CN_IDX_FORK, GFP_ATOMIC);
> >
> > Why is this GFP_ATOMIC?
>
> In the previous connector, cn_netlink_send(struct cn_msg *msg, u32
> __groups) called alloc_skb(size, GFP_ATOMIC). Now a third parameter is
> used with cn_netlink_send() in order to call alloc_skb(size, gfp_mask)
> with a specific gfp_mask. So, I'm using GFP_ATOMIC as the third argument
> to keep the same behavior.

It was atomic there to allow non process context usage.
Since do_fork() is executed in process context you can use GFP_KERNEL with
__GFP_NOFAIL - it will guarantee memory allocation.

> > > + }
> > > +}
> > > +#else
> > > +static inline void fork_connector(pid_t parent, pid_t child)
> > > +{
> > > + return;
> > > +}
> > > +#endif /* CONFIG_FORK_CONNECTOR */
> > > +#endif /* __KERNEL__ */
> > > +
> > > +#endif /* CN_FORK_H */
> > > Index: linux-2.6.12-rc3-mm3/include/linux/connector.h
> > > ===================================================================
> > > --- linux-2.6.12-rc3-mm3.orig/include/linux/connector.h 2005-05-09 07:45:56.000000000 +0200
> > > +++ linux-2.6.12-rc3-mm3/include/linux/connector.h 2005-05-09 09:50:01.000000000 +0200
> > > @@ -26,6 +26,8 @@
> > >
> > > #define CN_IDX_CONNECTOR 0xffffffff
> > > #define CN_VAL_CONNECTOR 0xffffffff
> > > +#define CN_IDX_FORK 0xfeed /* fork events */
> > > +#define CN_VAL_FORK 0xbeef
> > >
> > > /*
> > > * Maximum connector's message size.
> > > Index: linux-2.6.12-rc3-mm3/kernel/fork.c
> > > ===================================================================
> > > --- linux-2.6.12-rc3-mm3.orig/kernel/fork.c 2005-05-09 07:45:56.000000000 +0200
> > > +++ linux-2.6.12-rc3-mm3/kernel/fork.c 2005-05-09 08:03:15.000000000 +0200
> > > @@ -41,6 +41,7 @@
> > > #include <linux/profile.h>
> > > #include <linux/rmap.h>
> > > #include <linux/acct.h>
> > > +#include <linux/cn_fork.h>
> > >
> > > #include <asm/pgtable.h>
> > > #include <asm/pgalloc.h>
> > > @@ -63,6 +64,14 @@ DEFINE_PER_CPU(unsigned long, process_co
> > >
> > > EXPORT_SYMBOL(tasklist_lock);
> > >
> > > +#ifdef CONFIG_FORK_CONNECTOR
> > > +/*
> > > + * fork_counts is used by the fork_connector() inline routine as
> > > + * the sequence number of the netlink message.
> > > + */
> > > +static DEFINE_PER_CPU(unsigned long, fork_counts);
> > > +#endif /* CONFIG_FORK_CONNECTOR */
> > > +
> >
> > The above should go into cn_fork.c
>
> I don't see why. It's used by fork_connector which is an inline routine
> so, IMHO, 'fork_counts' must be defined here and declared in
> include/linux/cn_fork.h
>
> > > int nr_processes(void)
> > > {
> > > int cpu;
> > > @@ -1252,6 +1261,8 @@ long do_fork(unsigned long clone_flags,
> > > if (unlikely (current->ptrace & PT_TRACE_VFORK_DONE))
> > > ptrace_notify ((PTRACE_EVENT_VFORK_DONE << 8) | SIGTRAP);
> > > }
> > > +
> > > + fork_connector(current->pid, p->pid);
> >
> > Are you sure this is what you want? ->pid has a special meaning to the
> > kernel and doesn't necessarily mean the same to user-space, so I think
> > you want ->tgid here. If you look at sys_getpid() and sys_gettid()
> > you'll see what I mean.
>
> Yes, I think you're right. If I look the code of the BSD process
> accounting they're using the field ->tgid to get the process ID. I fix
> that.
>
> Thank you very much for your comments,
> Best regards,
>
> Guillaume


Evgeniy Polyakov

Only failure makes us experts. -- Theo de Raadt

2005-05-09 12:00:56

by Nick Piggin

[permalink] [raw]
Subject: Re: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector

Guillaume Thouvenin wrote:
> On Mon, 2005-05-09 at 11:31 +0200, Alexander Nyberg wrote:

>>>Index: linux-2.6.12-rc3-mm3/kernel/fork.c
>>>===================================================================
>>>--- linux-2.6.12-rc3-mm3.orig/kernel/fork.c 2005-05-09 07:45:56.000000000 +0200
>>>+++ linux-2.6.12-rc3-mm3/kernel/fork.c 2005-05-09 08:03:15.000000000 +0200
>>>@@ -41,6 +41,7 @@
>>> #include <linux/profile.h>
>>> #include <linux/rmap.h>
>>> #include <linux/acct.h>
>>>+#include <linux/cn_fork.h>
>>>
>>> #include <asm/pgtable.h>
>>> #include <asm/pgalloc.h>
>>>@@ -63,6 +64,14 @@ DEFINE_PER_CPU(unsigned long, process_co
>>>
>>> EXPORT_SYMBOL(tasklist_lock);
>>>
>>>+#ifdef CONFIG_FORK_CONNECTOR
>>>+/*
>>>+ * fork_counts is used by the fork_connector() inline routine as
>>>+ * the sequence number of the netlink message.
>>>+ */
>>>+static DEFINE_PER_CPU(unsigned long, fork_counts);
>>>+#endif /* CONFIG_FORK_CONNECTOR */
>>>+
>>
>>The above should go into cn_fork.c
>
>
> I don't see why.
>

Because you get these ugly ifdefs and things just spilling into
conceptually the wrong place. Why should anyone apart from the
fork connecter care how `fork_counts` is stored? And why would
any generic code care that it is defined when CONFIG_FORK_CONNECTOR
is set? Alexander is right, unless I missed something that requires
reading the code :)

--
SUSE Labs, Novell Inc.

2005-05-09 12:22:46

by Alexander Nyberg

[permalink] [raw]
Subject: Re: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector

> > > + }
> > > +}
> > > +#else
> > > +static inline void fork_connector(pid_t parent, pid_t child)
> > > +{
> > > + return;
> > > +}
> > > +#endif /* CONFIG_FORK_CONNECTOR */
> > > +#endif /* __KERNEL__ */
> > > +
> > > +#endif /* CN_FORK_H */
> > > Index: linux-2.6.12-rc3-mm3/include/linux/connector.h
> > > ===================================================================
> > > --- linux-2.6.12-rc3-mm3.orig/include/linux/connector.h 2005-05-09 07:45:56.000000000 +0200
> > > +++ linux-2.6.12-rc3-mm3/include/linux/connector.h 2005-05-09 09:50:01.000000000 +0200
> > > @@ -26,6 +26,8 @@
> > >
> > > #define CN_IDX_CONNECTOR 0xffffffff
> > > #define CN_VAL_CONNECTOR 0xffffffff
> > > +#define CN_IDX_FORK 0xfeed /* fork events */
> > > +#define CN_VAL_FORK 0xbeef
> > >
> > > /*
> > > * Maximum connector's message size.
> > > Index: linux-2.6.12-rc3-mm3/kernel/fork.c
> > > ===================================================================
> > > --- linux-2.6.12-rc3-mm3.orig/kernel/fork.c 2005-05-09 07:45:56.000000000 +0200
> > > +++ linux-2.6.12-rc3-mm3/kernel/fork.c 2005-05-09 08:03:15.000000000 +0200
> > > @@ -41,6 +41,7 @@
> > > #include <linux/profile.h>
> > > #include <linux/rmap.h>
> > > #include <linux/acct.h>
> > > +#include <linux/cn_fork.h>
> > >
> > > #include <asm/pgtable.h>
> > > #include <asm/pgalloc.h>
> > > @@ -63,6 +64,14 @@ DEFINE_PER_CPU(unsigned long, process_co
> > >
> > > EXPORT_SYMBOL(tasklist_lock);
> > >
> > > +#ifdef CONFIG_FORK_CONNECTOR
> > > +/*
> > > + * fork_counts is used by the fork_connector() inline routine as
> > > + * the sequence number of the netlink message.
> > > + */
> > > +static DEFINE_PER_CPU(unsigned long, fork_counts);
> > > +#endif /* CONFIG_FORK_CONNECTOR */
> > > +
> >
> > The above should go into cn_fork.c
>
> I don't see why. It's used by fork_connector which is an inline routine
> so, IMHO, 'fork_counts' must be defined here and declared in
> include/linux/cn_fork.h
>

You said it yourself, it's used by the fork connector not fork.c. You've
already have DECLARE_PER_CPU(unsigned long, fork_counts); in cn_fork.h
so just put

/*
* fork_counts is used by the fork_connector() inline routine as
* the sequence number of the netlink message.
*/
DEFINE_PER_CPU(unsigned long, fork_counts);

in cn_fork.c and remove that chunk from kernel/fork.c

And I don't like putting code like fork_connector() in a header file
even if it is used once only, it's just a simple call/ret...

And before any abi sets itself I think you might want to consider
including both thread & process id of parent & child. This way the
user-space client can distinguish what is a thread and a group leader
although I admittedly don't know all your goals with this, just a
thought.

2005-05-09 12:43:56

by Guillaume Thouvenin

[permalink] [raw]
Subject: Re: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector

On Mon, 2005-05-09 at 14:22 +0200, Alexander Nyberg wrote:
>
> And before any abi sets itself I think you might want to consider
> including both thread & process id of parent & child. This way the
> user-space client can distinguish what is a thread and a group leader
> although I admittedly don't know all your goals with this, just a
> thought.

Yes I completely agree :)

I will include both thread & process id of parent and child in the
next release. I also removed the DEFINE_PER_CPU(...) from kernel/fork.c
and I use a call instead of putting the code in the header file.

Thank you for your comments,
Best regards,

Guillaume

2005-05-09 13:14:51

by Guillaume Thouvenin

[permalink] [raw]
Subject: Re: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector

ChangeLog:

- Replace the GFP_ATOMIC flag in cn_netlink_send() by
GFP_KERNEL|__GFP_NOFAIL
- Move the following code
#ifdef CONFIG_FORK_CONNECTOR
static DEFINE_PER_CPU(unsigned long, fork_counts);
#endif /* CONFIG_FORK_CONNECTOR */
in the driver/connector/cn_fork.c file.
- Remove the code of fork_connector() from the header file
and put it in cn_fork.c
- As fork_connector is not a module, we remove the module
part and replace it by __initcall()
- Include thread ID, process id of parent and process id
of child in the message. This way the user-space client can
distinguish what is a thread and a group leader.

Many thanks to Alexander for its great help. Also, thank you to Evgeniy
and Nick for their great comments.

Best regards,
Guillaume

Signed-off-by: Guillaume Thouvenin <[email protected]>

---

drivers/connector/Kconfig | 11 +++
drivers/connector/Makefile | 1
drivers/connector/cn_fork.c | 132 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/cn_fork.h | 54 ++++++++++++++++++
include/linux/connector.h | 2
kernel/fork.c | 3 +
6 files changed, 203 insertions(+)


Index: linux-2.6.12-rc3-mm3/drivers/connector/Kconfig
===================================================================
--- linux-2.6.12-rc3-mm3.orig/drivers/connector/Kconfig 2005-05-09 07:45:52.000000000 +0200
+++ linux-2.6.12-rc3-mm3/drivers/connector/Kconfig 2005-05-09 08:03:15.000000000 +0200
@@ -10,4 +10,15 @@ config CONNECTOR
Connector support can also be built as a module. If so, the module
will be called cn.ko.

+config FORK_CONNECTOR
+ bool "Enable fork connector"
+ select CONNECTOR
+ default y
+ ---help---
+ It adds a connector in kernel/fork.c:do_fork() function. When a fork
+ occurs, netlink is used to transfer information about the parent and
+ its child. This information can be used by a user space application.
+ The fork connector can be enable/disable by sending a message to the
+ connector with the corresponding group id.
+
endmenu
Index: linux-2.6.12-rc3-mm3/drivers/connector/Makefile
===================================================================
--- linux-2.6.12-rc3-mm3.orig/drivers/connector/Makefile 2005-05-09 07:45:52.000000000 +0200
+++ linux-2.6.12-rc3-mm3/drivers/connector/Makefile 2005-05-09 08:03:15.000000000 +0200
@@ -1,2 +1,3 @@
obj-$(CONFIG_CONNECTOR) += cn.o
+obj-$(CONFIG_FORK_CONNECTOR) += cn_fork.o
cn-objs := cn_queue.o connector.o
Index: linux-2.6.12-rc3-mm3/drivers/connector/cn_fork.c
===================================================================
--- linux-2.6.12-rc3-mm3.orig/drivers/connector/cn_fork.c 2003-01-30 11:24:37.000000000 +0100
+++ linux-2.6.12-rc3-mm3/drivers/connector/cn_fork.c 2005-05-09 14:36:22.000000000 +0200
@@ -0,0 +1,132 @@
+/*
+ * cn_fork.c - Fork connector
+ *
+ * Copyright (C) 2005 BULL SA.
+ * Written by Guillaume Thouvenin <[email protected]>
+ *
+ * This module implements the fork connector. It allows to send a
+ * netlink datagram, when enabled, from the do_fork() routine. The
+ * message can be read by a user space application. By this way,
+ * the user space application is alerted when a fork occurs.
+ *
+ * It uses the userspace <-> kernelspace connector that works on top of
+ * the netlink protocol. The fork connector is enabled or disabled by
+ * sending a message to the connector. The unique sequence number of
+ * messages can be used to check if a message is lost.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+
+#include <linux/cn_fork.h>
+
+#define CN_FORK_INFO_SIZE sizeof(struct cn_fork_msg)
+#define CN_FORK_MSG_SIZE (sizeof(struct cn_msg) + CN_FORK_INFO_SIZE)
+
+static int cn_fork_enable = 0;
+struct cb_id cb_fork_id = { CN_IDX_FORK, CN_VAL_FORK };
+
+/* fork_counts is used as the sequence number of the netlink message */
+static DEFINE_PER_CPU(unsigned long, fork_counts);
+
+void fork_connector(pid_t thread, pid_t parent, pid_t child)
+{
+ if (cn_fork_enable) {
+ struct cn_msg *msg;
+ struct cn_fork_msg *forkmsg;
+ __u8 buffer[CN_FORK_MSG_SIZE];
+
+ msg = (struct cn_msg *)buffer;
+
+ memcpy(&msg->id, &cb_fork_id, sizeof(msg->id));
+
+ msg->ack = 0; /* not used */
+ msg->seq = get_cpu_var(fork_counts)++;
+
+ msg->len = CN_FORK_INFO_SIZE;
+ forkmsg = (struct cn_fork_msg *)msg->data;
+ forkmsg->cpu = smp_processor_id();
+ forkmsg->tgid = thread;
+ forkmsg->ppid = parent;
+ forkmsg->cpid = child;
+
+ put_cpu_var(fork_counts);
+
+ cn_netlink_send(msg, CN_IDX_FORK, GFP_KERNEL|__GFP_NOFAIL);
+ }
+}
+EXPORT_SYMBOL_GPL(fork_connector);
+
+static inline void cn_fork_send_status(void)
+{
+ /* TODO: An informational line in log is maybe not enough... */
+ printk(KERN_INFO "cn_fork_enable == %d\n", cn_fork_enable);
+}
+
+/**
+ * cn_fork_callback - enable or disable the fork connector
+ * @data: message send by the connector
+ *
+ * The callback allows to enable or disable the sending of information
+ * about fork in the do_fork() routine. To enable the fork, the user
+ * space application must send the integer 1 in the data part of the
+ * message. To disable the fork connector, it must send the integer 0.
+ */
+static void cn_fork_callback(void *data)
+{
+ struct cn_msg *msg = data;
+ int action;
+
+ if (cn_already_initialized && (msg->len == sizeof(cn_fork_enable))) {
+ memcpy(&action, msg->data, sizeof(cn_fork_enable));
+ switch(action) {
+ case FORK_CN_START:
+ cn_fork_enable = 1;
+ break;
+ case FORK_CN_STOP:
+ cn_fork_enable = 0;
+ break;
+ case FORK_CN_STATUS:
+ cn_fork_send_status();
+ break;
+ }
+ }
+}
+
+/**
+ * cn_fork_init - initialization entry point
+ *
+ * This routine will be run at kernel boot time because this driver is
+ * built in the kernel. It adds the connector callback to the connector
+ * driver.
+ */
+int __init cn_fork_init(void)
+{
+ int err;
+
+ err = cn_add_callback(&cb_fork_id, "cn_fork", &cn_fork_callback);
+ if (err) {
+ printk(KERN_WARNING "Failed to register cn_fork\n");
+ return -EINVAL;
+ }
+
+ printk(KERN_NOTICE "cn_fork is registered\n");
+ return 0;
+}
+
+__initcall(cn_fork_init);
Index: linux-2.6.12-rc3-mm3/include/linux/cn_fork.h
===================================================================
--- linux-2.6.12-rc3-mm3.orig/include/linux/cn_fork.h 2003-01-30 11:24:37.000000000 +0100
+++ linux-2.6.12-rc3-mm3/include/linux/cn_fork.h 2005-05-09 14:34:36.000000000 +0200
@@ -0,0 +1,54 @@
+/*
+ * cn_fork.h - Fork connector
+ *
+ * Copyright (C) 2005 Nguyen Anh Quynh <[email protected]>
+ * Copyright (C) 2005 Guillaume Thouvenin <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef CN_FORK_H
+#define CN_FORK_H
+
+#include <linux/connector.h>
+
+#define FORK_CN_STOP 0
+#define FORK_CN_START 1
+#define FORK_CN_STATUS 2
+
+struct cn_fork_msg
+{
+ int cpu; /* ID of the cpu where the fork occured */
+ pid_t tgid; /* thread leader PID */
+ pid_t ppid; /* the parent PID */
+ pid_t cpid; /* the child PID */
+};
+
+/* Code above is only inside the kernel */
+#ifdef __KERNEL__
+
+extern int cn_already_initialized;
+
+#ifdef CONFIG_FORK_CONNECTOR
+extern void fork_connector(pid_t, pid_t, pid_t);
+#else
+static inline void fork_connector(pid_t thread, pid_t parent, pid_t child)
+{
+ return;
+}
+#endif /* CONFIG_FORK_CONNECTOR */
+
+#endif /* __KERNEL__ */
+#endif /* CN_FORK_H */
Index: linux-2.6.12-rc3-mm3/include/linux/connector.h
===================================================================
--- linux-2.6.12-rc3-mm3.orig/include/linux/connector.h 2005-05-09 07:45:56.000000000 +0200
+++ linux-2.6.12-rc3-mm3/include/linux/connector.h 2005-05-09 09:50:01.000000000 +0200
@@ -26,6 +26,8 @@

#define CN_IDX_CONNECTOR 0xffffffff
#define CN_VAL_CONNECTOR 0xffffffff
+#define CN_IDX_FORK 0xfeed /* fork events */
+#define CN_VAL_FORK 0xbeef

/*
* Maximum connector's message size.
Index: linux-2.6.12-rc3-mm3/kernel/fork.c
===================================================================
--- linux-2.6.12-rc3-mm3.orig/kernel/fork.c 2005-05-09 07:45:56.000000000 +0200
+++ linux-2.6.12-rc3-mm3/kernel/fork.c 2005-05-09 14:23:04.000000000 +0200
@@ -41,6 +41,7 @@
#include <linux/profile.h>
#include <linux/rmap.h>
#include <linux/acct.h>
+#include <linux/cn_fork.h>

#include <asm/pgtable.h>
#include <asm/pgalloc.h>
@@ -1252,6 +1253,8 @@ long do_fork(unsigned long clone_flags,
if (unlikely (current->ptrace & PT_TRACE_VFORK_DONE))
ptrace_notify ((PTRACE_EVENT_VFORK_DONE << 8) | SIGTRAP);
}
+
+ fork_connector(current->tgid, current->pid, p->pid);
} else {
free_pidmap(pid);
pid = PTR_ERR(p);



2005-05-09 13:32:42

by Nguyen Anh Quynh

[permalink] [raw]
Subject: Re: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector

On 5/9/05, Guillaume Thouvenin <[email protected]> wrote:
> ChangeLog:
>
> - Replace the GFP_ATOMIC flag in cn_netlink_send() by
> GFP_KERNEL|__GFP_NOFAIL
> - Move the following code
> #ifdef CONFIG_FORK_CONNECTOR
> static DEFINE_PER_CPU(unsigned long, fork_counts);
> #endif /* CONFIG_FORK_CONNECTOR */
> in the driver/connector/cn_fork.c file.
> - Remove the code of fork_connector() from the header file
> and put it in cn_fork.c

this is a good idea, because it will shrink your modification to
mainline kernel by few more lines ;-)

regards,
aq

2005-05-09 13:58:13

by Alexander Nyberg

[permalink] [raw]
Subject: Re: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector

> Index: linux-2.6.12-rc3-mm3/drivers/connector/cn_fork.c
> ===================================================================
> --- linux-2.6.12-rc3-mm3.orig/drivers/connector/cn_fork.c 2003-01-30 11:24:37.000000000 +0100
> +++ linux-2.6.12-rc3-mm3/drivers/connector/cn_fork.c 2005-05-09 14:36:22.000000000 +0200
> @@ -0,0 +1,132 @@
> +/*
> + * cn_fork.c - Fork connector
> + *
> + * Copyright (C) 2005 BULL SA.
> + * Written by Guillaume Thouvenin <[email protected]>
> + *
> + * This module implements the fork connector. It allows to send a
> + * netlink datagram, when enabled, from the do_fork() routine. The
> + * message can be read by a user space application. By this way,
> + * the user space application is alerted when a fork occurs.
> + *
> + * It uses the userspace <-> kernelspace connector that works on top of
> + * the netlink protocol. The fork connector is enabled or disabled by
> + * sending a message to the connector. The unique sequence number of
> + * messages can be used to check if a message is lost.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +
> +#include <linux/cn_fork.h>
> +
> +#define CN_FORK_INFO_SIZE sizeof(struct cn_fork_msg)
> +#define CN_FORK_MSG_SIZE (sizeof(struct cn_msg) + CN_FORK_INFO_SIZE)
> +
> +static int cn_fork_enable = 0;
> +struct cb_id cb_fork_id = { CN_IDX_FORK, CN_VAL_FORK };
> +
> +/* fork_counts is used as the sequence number of the netlink message */
> +static DEFINE_PER_CPU(unsigned long, fork_counts);
> +
> +void fork_connector(pid_t thread, pid_t parent, pid_t child)
> +{
> + if (cn_fork_enable) {
> + struct cn_msg *msg;
> + struct cn_fork_msg *forkmsg;
> + __u8 buffer[CN_FORK_MSG_SIZE];
> +
> + msg = (struct cn_msg *)buffer;
> +
> + memcpy(&msg->id, &cb_fork_id, sizeof(msg->id));
> +
> + msg->ack = 0; /* not used */
> + msg->seq = get_cpu_var(fork_counts)++;
> +
> + msg->len = CN_FORK_INFO_SIZE;
> + forkmsg = (struct cn_fork_msg *)msg->data;
> + forkmsg->cpu = smp_processor_id();
> + forkmsg->tgid = thread;
> + forkmsg->ppid = parent;
> + forkmsg->cpid = child;
> +
> + put_cpu_var(fork_counts);
> +
> + cn_netlink_send(msg, CN_IDX_FORK, GFP_KERNEL|__GFP_NOFAIL);
> + }
> +}
> +EXPORT_SYMBOL_GPL(fork_connector);

the export symbol police will get you for this, any intended users?

> +
> +static inline void cn_fork_send_status(void)
> +{
> + /* TODO: An informational line in log is maybe not enough... */
> + printk(KERN_INFO "cn_fork_enable == %d\n", cn_fork_enable);
> +}
> +
> +/**
> + * cn_fork_callback - enable or disable the fork connector
> + * @data: message send by the connector
> + *
> + * The callback allows to enable or disable the sending of information
> + * about fork in the do_fork() routine. To enable the fork, the user
> + * space application must send the integer 1 in the data part of the
> + * message. To disable the fork connector, it must send the integer 0.
> + */
> +static void cn_fork_callback(void *data)
> +{
> + struct cn_msg *msg = data;
> + int action;
> +
> + if (cn_already_initialized && (msg->len == sizeof(cn_fork_enable))) {
> + memcpy(&action, msg->data, sizeof(cn_fork_enable));
> + switch(action) {
> + case FORK_CN_START:
> + cn_fork_enable = 1;
> + break;
> + case FORK_CN_STOP:
> + cn_fork_enable = 0;
> + break;
> + case FORK_CN_STATUS:
> + cn_fork_send_status();

Why does this not pass down the status to the app asking about it
instead?

> + break;
> + }
> + }
> +}
> +
> +/**
> + * cn_fork_init - initialization entry point
> + *
> + * This routine will be run at kernel boot time because this driver is
> + * built in the kernel. It adds the connector callback to the connector
> + * driver.
> + */
> +int __init cn_fork_init(void)
> +{
> + int err;
> +
> + err = cn_add_callback(&cb_fork_id, "cn_fork", &cn_fork_callback);
> + if (err) {
> + printk(KERN_WARNING "Failed to register cn_fork\n");
> + return -EINVAL;
> + }
> +
> + printk(KERN_NOTICE "cn_fork is registered\n");
> + return 0;
> +}
> +
> +__initcall(cn_fork_init);
> Index: linux-2.6.12-rc3-mm3/include/linux/cn_fork.h
> ===================================================================
> --- linux-2.6.12-rc3-mm3.orig/include/linux/cn_fork.h 2003-01-30 11:24:37.000000000 +0100
> +++ linux-2.6.12-rc3-mm3/include/linux/cn_fork.h 2005-05-09 14:34:36.000000000 +0200
> @@ -0,0 +1,54 @@
> +/*
> + * cn_fork.h - Fork connector
> + *
> + * Copyright (C) 2005 Nguyen Anh Quynh <[email protected]>
> + * Copyright (C) 2005 Guillaume Thouvenin <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#ifndef CN_FORK_H
> +#define CN_FORK_H
> +
> +#include <linux/connector.h>
> +
> +#define FORK_CN_STOP 0
> +#define FORK_CN_START 1
> +#define FORK_CN_STATUS 2
> +
> +struct cn_fork_msg
> +{
> + int cpu; /* ID of the cpu where the fork occured */
> + pid_t tgid; /* thread leader PID */
> + pid_t ppid; /* the parent PID */
> + pid_t cpid; /* the child PID */
> +};
> +

What I meant here was something like:
struct cn_fork_msg {
int cpu;
pid_t ppid; /* parent process id */
pid_t ptid; /* parent thread id */
pid_t cpid; /* child process id */
pid_t ctid; /* child thread id */

};

Now if cpid == ctid it is a group leader but you cannot know this
otherwise. I don't know if you specifically need this but there may be
other users, and this is more complete. If not you may receive multiple
messages with the same child pid. Again I don't know how much you care.

As this is shared with user-space the fields need to switched like this:
ppid = current->tgid,
ptid = current->pid,
cpid = p->tgid,
ctid = current->pid

and perhaps a comment to explain why we assign the fields like this.
Please think through what you need to pass down to user-space, it's
important.

> +/* Code above is only inside the kernel */
> +#ifdef __KERNEL__
> +
> +extern int cn_already_initialized;

this should go into linux/connector.h

> +#ifdef CONFIG_FORK_CONNECTOR
> +extern void fork_connector(pid_t, pid_t, pid_t);
> +#else
> +static inline void fork_connector(pid_t thread, pid_t parent, pid_t child)
> +{
> + return;
> +}
> +#endif /* CONFIG_FORK_CONNECTOR */
> +
> +#endif /* __KERNEL__ */
> +#endif /* CN_FORK_H */
> Index: linux-2.6.12-rc3-mm3/include/linux/connector.h
> ===================================================================
> --- linux-2.6.12-rc3-mm3.orig/include/linux/connector.h 2005-05-09 07:45:56.000000000 +0200
> +++ linux-2.6.12-rc3-mm3/include/linux/connector.h 2005-05-09 09:50:01.000000000 +0200
> @@ -26,6 +26,8 @@
>
> #define CN_IDX_CONNECTOR 0xffffffff
> #define CN_VAL_CONNECTOR 0xffffffff
> +#define CN_IDX_FORK 0xfeed /* fork events */
> +#define CN_VAL_FORK 0xbeef
>
> /*
> * Maximum connector's message size.
> Index: linux-2.6.12-rc3-mm3/kernel/fork.c
> ===================================================================
> --- linux-2.6.12-rc3-mm3.orig/kernel/fork.c 2005-05-09 07:45:56.000000000 +0200
> +++ linux-2.6.12-rc3-mm3/kernel/fork.c 2005-05-09 14:23:04.000000000 +0200
> @@ -41,6 +41,7 @@
> #include <linux/profile.h>
> #include <linux/rmap.h>
> #include <linux/acct.h>
> +#include <linux/cn_fork.h>
>
> #include <asm/pgtable.h>
> #include <asm/pgalloc.h>
> @@ -1252,6 +1253,8 @@ long do_fork(unsigned long clone_flags,
> if (unlikely (current->ptrace & PT_TRACE_VFORK_DONE))
> ptrace_notify ((PTRACE_EVENT_VFORK_DONE << 8) | SIGTRAP);
> }
> +
> + fork_connector(current->tgid, current->pid, p->pid);

With your current implementation this is wrong, first two args should
switched, third arg should be p->tgid. The differences between kernel
and user pid must be hidden as you share struct cn_fork_msg with
user-space.

> } else {
> free_pidmap(pid);
> pid = PTR_ERR(p);
>
>


2005-05-09 21:10:35

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector

Evgeniy Polyakov <[email protected]> wrote:
>
> > In the previous connector, cn_netlink_send(struct cn_msg *msg, u32
> > __groups) called alloc_skb(size, GFP_ATOMIC). Now a third parameter is
> > used with cn_netlink_send() in order to call alloc_skb(size, gfp_mask)
> > with a specific gfp_mask. So, I'm using GFP_ATOMIC as the third argument
> > to keep the same behavior.
>
> It was atomic there to allow non process context usage.
> Since do_fork() is executed in process context you can use GFP_KERNEL with
> __GFP_NOFAIL - it will guarantee memory allocation.

Please avoid using __GFP_NOFAIL.

__GFP_NOFAIL was introduced because we had lots of places in the kernel
which were doing:

try_again:
p = allocate_something();
if (!p) {
/* A am too lame to handle this */
delay_in_some_manner();
goto try_again;
}

__GFP_NOFAIL simply takes the above bad code and centralises it so it's
always done in the same way and so that it's easily greppable for. But
it's still bad (ie: deadlocky) code.

Well-behaved code should notice the allocation failure and should back out
gracefully.

2005-05-09 22:17:46

by Evgeniy Polyakov

[permalink] [raw]
Subject: Re: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector

On Mon, 9 May 2005 14:08:36 -0700
Andrew Morton <[email protected]> wrote:

> Evgeniy Polyakov <[email protected]> wrote:
> >
> > > In the previous connector, cn_netlink_send(struct cn_msg *msg, u32
> > > __groups) called alloc_skb(size, GFP_ATOMIC). Now a third parameter is
> > > used with cn_netlink_send() in order to call alloc_skb(size, gfp_mask)
> > > with a specific gfp_mask. So, I'm using GFP_ATOMIC as the third argument
> > > to keep the same behavior.
> >
> > It was atomic there to allow non process context usage.
> > Since do_fork() is executed in process context you can use GFP_KERNEL with
> > __GFP_NOFAIL - it will guarantee memory allocation.
>
> Please avoid using __GFP_NOFAIL.
>
> __GFP_NOFAIL was introduced because we had lots of places in the kernel
> which were doing:
>
> try_again:
> p = allocate_something();
> if (!p) {
> /* A am too lame to handle this */
> delay_in_some_manner();
> goto try_again;
> }
>
> __GFP_NOFAIL simply takes the above bad code and centralises it so it's
> always done in the same way and so that it's easily greppable for. But
> it's still bad (ie: deadlocky) code.
>
> Well-behaved code should notice the allocation failure and should back out
> gracefully.

If system under so bad conditions that even process context allocation
fails, then connector may drop message and will return error,
probably in such conditions one may not even trust accounting data,
so, Guillaume, feel free to drop it and just do not send data -
it's only a matter of information importance.

Evgeniy Polyakov

Only failure makes us experts. -- Theo de Raadt

2005-05-10 11:13:55

by Nguyen Anh Quynh

[permalink] [raw]
Subject: Re: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector

On 5/10/05, Evgeniy Polyakov <[email protected]> wrote:
> On Mon, 9 May 2005 14:08:36 -0700
> Andrew Morton <[email protected]> wrote:
>
> > Evgeniy Polyakov <[email protected]> wrote:
> > >
> > > > In the previous connector, cn_netlink_send(struct cn_msg *msg, u32
> > > > __groups) called alloc_skb(size, GFP_ATOMIC). Now a third parameter is
> > > > used with cn_netlink_send() in order to call alloc_skb(size, gfp_mask)
> > > > with a specific gfp_mask. So, I'm using GFP_ATOMIC as the third argument
> > > > to keep the same behavior.
> > >
> > > It was atomic there to allow non process context usage.
> > > Since do_fork() is executed in process context you can use GFP_KERNEL with
> > > __GFP_NOFAIL - it will guarantee memory allocation.
> >
> > Please avoid using __GFP_NOFAIL.
> >
> > __GFP_NOFAIL was introduced because we had lots of places in the kernel
> > which were doing:
> >
> > try_again:
> > p = allocate_something();
> > if (!p) {
> > /* A am too lame to handle this */
> > delay_in_some_manner();
> > goto try_again;
> > }
> >
> > __GFP_NOFAIL simply takes the above bad code and centralises it so it's
> > always done in the same way and so that it's easily greppable for. But
> > it's still bad (ie: deadlocky) code.
> >
> > Well-behaved code should notice the allocation failure and should back out
> > gracefully.
>
> If system under so bad conditions that even process context allocation
> fails, then connector may drop message and will return error,
> probably in such conditions one may not even trust accounting data,
> so, Guillaume, feel free to drop it and just do not send data -
> it's only a matter of information importance.
>

what i find most troublesome of this solution is that information
collected by ELSA is not reliable. The reason is that it uses netlink,
which is connectionless. So under heavy pressure, we can lose some
critical accounting data.

At least it would be nice if we can know when and why there are
problems of losing data with netlink. Any solution?

thank you,
aq

2005-05-10 11:18:53

by Guillaume Thouvenin

[permalink] [raw]
Subject: Re: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector

ChangeLog:

- Remove __GFP_NOFAIL flag. If an error occurred, the message is
just dropped.
- Modify struct cn_fork_msg to hold the following values:
ID of the cpu where the fork occurred,
Parent process id,
Parent thread id,
Child process id
Child thread id
All values are from a user's point of view. An explanation
has been added in the cn_fork.h header.
- Remove "EXPORT_SYMBOL_GPL(fork_connector);"
- Move "extern int cn_already_initialized" in file
include/linux/connector.h. This declaration should be
include in the next release of the connector.

TODO:

- Change the way the status is displayed. I don't know yet how to
do it cleanly.


Signed-off-by: Guillaume Thouvenin <[email protected]>

---

drivers/connector/Kconfig | 11 +++
drivers/connector/Makefile | 1
drivers/connector/cn_fork.c | 133 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/cn_fork.h | 64 +++++++++++++++++++++
include/linux/connector.h | 4 +
kernel/fork.c | 4 +
6 files changed, 217 insertions(+)

Index: linux-2.6.12-rc3-mm3/drivers/connector/Kconfig
===================================================================
--- linux-2.6.12-rc3-mm3.orig/drivers/connector/Kconfig 2005-05-09 07:45:52.000000000 +0200
+++ linux-2.6.12-rc3-mm3/drivers/connector/Kconfig 2005-05-09 08:03:15.000000000 +0200
@@ -10,4 +10,15 @@ config CONNECTOR
Connector support can also be built as a module. If so, the module
will be called cn.ko.

+config FORK_CONNECTOR
+ bool "Enable fork connector"
+ select CONNECTOR
+ default y
+ ---help---
+ It adds a connector in kernel/fork.c:do_fork() function. When a fork
+ occurs, netlink is used to transfer information about the parent and
+ its child. This information can be used by a user space application.
+ The fork connector can be enable/disable by sending a message to the
+ connector with the corresponding group id.
+
endmenu
Index: linux-2.6.12-rc3-mm3/drivers/connector/Makefile
===================================================================
--- linux-2.6.12-rc3-mm3.orig/drivers/connector/Makefile 2005-05-09 07:45:52.000000000 +0200
+++ linux-2.6.12-rc3-mm3/drivers/connector/Makefile 2005-05-09 08:03:15.000000000 +0200
@@ -1,2 +1,3 @@
obj-$(CONFIG_CONNECTOR) += cn.o
+obj-$(CONFIG_FORK_CONNECTOR) += cn_fork.o
cn-objs := cn_queue.o connector.o
Index: linux-2.6.12-rc3-mm3/drivers/connector/cn_fork.c
===================================================================
--- linux-2.6.12-rc3-mm3.orig/drivers/connector/cn_fork.c 2003-01-30 11:24:37.000000000 +0100
+++ linux-2.6.12-rc3-mm3/drivers/connector/cn_fork.c 2005-05-10 09:40:08.000000000 +0200
@@ -0,0 +1,133 @@
+/*
+ * cn_fork.c - Fork connector
+ *
+ * Copyright (C) 2005 BULL SA.
+ * Written by Guillaume Thouvenin <[email protected]>
+ *
+ * This module implements the fork connector. It allows to send a
+ * netlink datagram, when enabled, from the do_fork() routine. The
+ * message can be read by a user space application. By this way,
+ * the user space application is alerted when a fork occurs.
+ *
+ * It uses the userspace <-> kernelspace connector that works on top of
+ * the netlink protocol. The fork connector is enabled or disabled by
+ * sending a message to the connector. The unique sequence number of
+ * messages can be used to check if a message is lost.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+
+#include <linux/cn_fork.h>
+
+#define CN_FORK_INFO_SIZE sizeof(struct cn_fork_msg)
+#define CN_FORK_MSG_SIZE (sizeof(struct cn_msg) + CN_FORK_INFO_SIZE)
+
+static int cn_fork_enable = 0;
+struct cb_id cb_fork_id = { CN_IDX_FORK, CN_VAL_FORK };
+
+/* fork_counts is used as the sequence number of the netlink message */
+static DEFINE_PER_CPU(unsigned long, fork_counts);
+
+void fork_connector(pid_t ppid, pid_t ptid, pid_t cpid, pid_t ctid)
+{
+ if (cn_fork_enable) {
+ struct cn_msg *msg;
+ struct cn_fork_msg *forkmsg;
+ __u8 buffer[CN_FORK_MSG_SIZE];
+
+ msg = (struct cn_msg *)buffer;
+
+ memcpy(&msg->id, &cb_fork_id, sizeof(msg->id));
+
+ msg->ack = 0; /* not used */
+ msg->seq = get_cpu_var(fork_counts)++;
+
+ msg->len = CN_FORK_INFO_SIZE;
+ forkmsg = (struct cn_fork_msg *)msg->data;
+ forkmsg->cpu = smp_processor_id();
+ forkmsg->ppid = ppid;
+ forkmsg->ptid = ptid;
+ forkmsg->cpid = cpid;
+ forkmsg->ctid = ctid;
+
+ put_cpu_var(fork_counts);
+
+ /* If cn_netlink_send() failed, the data is not send */
+ cn_netlink_send(msg, CN_IDX_FORK, GFP_KERNEL);
+ }
+}
+
+static inline void cn_fork_send_status(void)
+{
+ /* TODO: An informational line in log is maybe not enough... */
+ printk(KERN_INFO "cn_fork_enable == %d\n", cn_fork_enable);
+}
+
+/**
+ * cn_fork_callback - enable or disable the fork connector
+ * @data: message send by the connector
+ *
+ * The callback allows to enable or disable the sending of information
+ * about fork in the do_fork() routine. To enable the fork, the user
+ * space application must send the integer 1 in the data part of the
+ * message. To disable the fork connector, it must send the integer 0.
+ */
+static void cn_fork_callback(void *data)
+{
+ struct cn_msg *msg = data;
+ int action;
+
+ if (cn_already_initialized && (msg->len == sizeof(cn_fork_enable))) {
+ memcpy(&action, msg->data, sizeof(cn_fork_enable));
+ switch (action) {
+ case FORK_CN_START:
+ cn_fork_enable = 1;
+ break;
+ case FORK_CN_STOP:
+ cn_fork_enable = 0;
+ break;
+ case FORK_CN_STATUS:
+ cn_fork_send_status();
+ break;
+ }
+ }
+}
+
+/**
+ * cn_fork_init - initialization entry point
+ *
+ * This routine will be run at kernel boot time because this driver is
+ * built in the kernel. It adds the connector callback to the connector
+ * driver.
+ */
+int __init cn_fork_init(void)
+{
+ int err;
+
+ err = cn_add_callback(&cb_fork_id, "cn_fork", &cn_fork_callback);
+ if (err) {
+ printk(KERN_WARNING "Failed to register cn_fork\n");
+ return -EINVAL;
+ }
+
+ printk(KERN_NOTICE "cn_fork is registered\n");
+ return 0;
+}
+
+__initcall(cn_fork_init);
Index: linux-2.6.12-rc3-mm3/include/linux/cn_fork.h
===================================================================
--- linux-2.6.12-rc3-mm3.orig/include/linux/cn_fork.h 2003-01-30 11:24:37.000000000 +0100
+++ linux-2.6.12-rc3-mm3/include/linux/cn_fork.h 2005-05-10 11:27:49.000000000 +0200
@@ -0,0 +1,64 @@
+/*
+ * cn_fork.h - Fork connector
+ *
+ * Copyright (C) 2005 Nguyen Anh Quynh <[email protected]>
+ * Copyright (C) 2005 Guillaume Thouvenin <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef CN_FORK_H
+#define CN_FORK_H
+
+#include <linux/connector.h>
+
+#define FORK_CN_STOP 0
+#define FORK_CN_START 1
+#define FORK_CN_STATUS 2
+
+/*
+ * The fork connector sends information to a user-space
+ * application. From the user's point of view, the process
+ * ID is the thread group ID and thread ID is the internal
+ * kernel "pid". So, fields are assigned as follow:
+ *
+ * In user space - In kernel space
+ *
+ * parent process ID = parent->tgid
+ * parent thread ID = parent->pid
+ * child process ID = child->tgid
+ * child thread ID = child->pid
+ */
+struct cn_fork_msg {
+ int cpu; /* ID of the cpu where the fork occured */
+ pid_t ppid; /* parent process ID */
+ pid_t ptid; /* parent thread ID */
+ pid_t cpid; /* child process ID */
+ pid_t ctid; /* child thread ID */
+};
+
+/* Code above is only inside the kernel */
+#ifdef __KERNEL__
+#ifdef CONFIG_FORK_CONNECTOR
+extern void fork_connector(pid_t ppid, pid_t ptid, pid_t cpid, pid_t ctid);
+#else
+static inline void fork_connector(pid_t ppid, pid_t ptid, pid_t cpid,
+ pid_t ctid);
+{
+ return;
+}
+#endif /* CONFIG_FORK_CONNECTOR */
+#endif /* __KERNEL__ */
+#endif /* CN_FORK_H */
Index: linux-2.6.12-rc3-mm3/include/linux/connector.h
===================================================================
--- linux-2.6.12-rc3-mm3.orig/include/linux/connector.h 2005-05-09 07:45:56.000000000 +0200
+++ linux-2.6.12-rc3-mm3/include/linux/connector.h 2005-05-10 10:27:43.000000000 +0200
@@ -26,6 +26,8 @@

#define CN_IDX_CONNECTOR 0xffffffff
#define CN_VAL_CONNECTOR 0xffffffff
+#define CN_IDX_FORK 0xfeed /* fork events */
+#define CN_VAL_FORK 0xbeef

/*
* Maximum connector's message size.
@@ -137,6 +139,8 @@ struct cn_dev {
struct cn_queue_dev *cbdev;
};

+extern int cn_already_initialized;
+
int cn_add_callback(struct cb_id *, char *, void (*callback) (void *));
void cn_del_callback(struct cb_id *);
int cn_netlink_send(struct cn_msg *, u32, int);
Index: linux-2.6.12-rc3-mm3/kernel/fork.c
===================================================================
--- linux-2.6.12-rc3-mm3.orig/kernel/fork.c 2005-05-09 07:45:56.000000000 +0200
+++ linux-2.6.12-rc3-mm3/kernel/fork.c 2005-05-10 11:26:08.000000000 +0200
@@ -41,6 +41,7 @@
#include <linux/profile.h>
#include <linux/rmap.h>
#include <linux/acct.h>
+#include <linux/cn_fork.h>

#include <asm/pgtable.h>
#include <asm/pgalloc.h>
@@ -1252,6 +1253,9 @@ long do_fork(unsigned long clone_flags,
if (unlikely (current->ptrace & PT_TRACE_VFORK_DONE))
ptrace_notify ((PTRACE_EVENT_VFORK_DONE << 8) | SIGTRAP);
}
+
+ fork_connector(current->tgid, current->pid,
+ p->tgid, p->pid);
} else {
free_pidmap(pid);
pid = PTR_ERR(p);


2005-05-10 11:25:10

by Evgeniy Polyakov

[permalink] [raw]
Subject: Re: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector

On Tue, 10 May 2005 20:13:43 +0900
aq <[email protected]> wrote:

> > If system under so bad conditions that even process context allocation
> > fails, then connector may drop message and will return error,
> > probably in such conditions one may not even trust accounting data,
> > so, Guillaume, feel free to drop it and just do not send data -
> > it's only a matter of information importance.
> >
>
> what i find most troublesome of this solution is that information
> collected by ELSA is not reliable. The reason is that it uses netlink,
> which is connectionless. So under heavy pressure, we can lose some
> critical accounting data.
>
> At least it would be nice if we can know when and why there are
> problems of losing data with netlink. Any solution?

1. Netlink message may be lost only under strong memory pressure, when
allocation (in this case it is process allocation) fails.
cn_netlink_send() returns error code for that.
2. Userspace application do not read it's queue for a long time,
so it was overflowed. Do not blame netlink here.

All netlink messages being sent using connector have
two special fields in it's header - seq and ack,
which should be used by caller to provide information about
number of messages and their's order.
By properly using such fields [fork connector uses per-cpu
variables with processor id in each message] userspace can
detect that messages were lost.

Other info can be found in connector's documentation
and all previous discussions.

> thank you,
> aq

Evgeniy Polyakov

Only failure makes us experts. -- Theo de Raadt

2005-05-11 11:45:14

by Guillaume Thouvenin

[permalink] [raw]
Subject: Re: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector

On Mon, 2005-05-09 at 15:57 +0200, Alexander Nyberg wrote:
> > +static inline void cn_fork_send_status(void)
> > +{
> > + /* TODO: An informational line in log is maybe not enough... */
> > + printk(KERN_INFO "cn_fork_enable == %d\n", cn_fork_enable);
> > +}
> > +
> > +/**
> > + * cn_fork_callback - enable or disable the fork connector
> > + * @data: message send by the connector
> > + *
> > + * The callback allows to enable or disable the sending of information
> > + * about fork in the do_fork() routine. To enable the fork, the user
> > + * space application must send the integer 1 in the data part of the
> > + * message. To disable the fork connector, it must send the integer 0.
> > + */
> > +static void cn_fork_callback(void *data)
> > +{
> > + struct cn_msg *msg = data;
> > + int action;
> > +
> > + if (cn_already_initialized && (msg->len == sizeof(cn_fork_enable))) {
> > + memcpy(&action, msg->data, sizeof(cn_fork_enable));
> > + switch(action) {
> > + case FORK_CN_START:
> > + cn_fork_enable = 1;
> > + break;
> > + case FORK_CN_STOP:
> > + cn_fork_enable = 0;
> > + break;
> > + case FORK_CN_STATUS:
> > + cn_fork_send_status();
>
> Why does this not pass down the status to the app asking about it
> instead?

I don't know exactly how to do that. A solution could be to send a
message through the connector. I think about using the following
structure:

#define FORK_CN_MSG_P 0 /* Information is about processes */
#define FORK_CN_MSG_S 1 /* Information is about status */

/*
* The fork connector sends information to a user-space
* application. From the user's point of view, the process
* ID is the thread group ID and thread ID is the internal
* kernel "pid". So, fields are assigned as follow:
*
* In user space - In kernel space
*
* parent process ID = parent->tgid
* parent thread ID = parent->pid
* child process ID = child->tgid
* child thread ID = child->pid
*/
struct cn_fork_msg {
int type; /* 0: information about fork
1: information about the status */
int cpu; /* ID of the cpu where the fork occured */
union {
struct {
pid_t ppid; /* parent process ID */
pid_t ptid; /* parent thread ID */
pid_t cpid; /* child process ID */
pid_t ctid; /* child thread ID */
};
int status;
};
};

And then, the cn_fork_send_status() could be coded as follow:

/**
* cn_fork_send_status - send a message with the status
*/
static inline void cn_fork_send_status(void)
{
struct cn_msg *msg;
struct cn_fork_msg *forkmsg;
__u8 buffer[CN_FORK_MSG_SIZE];

msg = (struct cn_msg *)buffer;

memcpy(&msg->id, &cb_fork_id, sizeof(msg->id));

msg->ack = 0; /* not used */
msg->seq = 0; /* not used */

msg->len = CN_FORK_INFO_SIZE;
forkmsg = (struct cn_fork_msg *)msg->data;
forkmsg->type = FORK_CN_MSG_S;
forkmsg->status = cn_fork_enable;

cn_netlink_send(msg, CN_IDX_FORK, GFP_KERNEL);
}

I think this solution is good. Agree?

Best,
Guillaume.