Re: [PATCH 2.6.11-rc4-mm1] connector: Add a fork connector

  ChangeLog:

    - Add parenthesis around sizeof(struct cn_msg) + CN_FORK_INFO_SIZE
      in the CN_FORK_MSG_SIZE macro
    - fork_cn_lock is declareed with DEFINE_SPINLOCK()
    - fork_cn_lock is defined as static and local to fork_connector()
    - Create a specific module cn_fork.c in drivers/connector to
      register the callback.
    - Improve the callback that turns on/off the fork connector

  I also run the lmbench and results are send in response to another
thread "A common layer for Accounting packages". When fork connector is
turned off the overhead is negligible. This patch works with another
small patch that fix a problem in the connector. Without it, there is a
message that says "skb does not have enough length". It will be fix in
the next -mm tree I think.


Thanks everyone for the comments,
Guillaume

Signed-off-by: Guillaume Thouvenin <redacted>

---

 drivers/connector/Kconfig   |   11 +++++
 drivers/connector/Makefile  |    1
 drivers/connector/cn_fork.c |   85 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/connector.h   |    4 ++
 kernel/fork.c               |   44 ++++++++++++++++++++++
 5 files changed, 145 insertions(+)

diff -uprN -X dontdiff linux-2.6.11-rc4-mm1/drivers/connector/cn_fork.c linux-2.6.11-rc4-mm1-cnfork/drivers/connector/cn_fork.c

--- linux-2.6.11-rc4-mm1/drivers/connector/cn_fork.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.11-rc4-mm1-cnfork/drivers/connector/cn_fork.c	2005-03-01 13:13:05.000000000 +0100

@@ -0,0 +1,85 @@
+/*
+ * 	cn_fork.c
+ * 
+ * 2005 Copyright (c) Guillaume Thouvenin <guillaume.thouvenin@bull.net>
+ * All rights reserved.
+ * 
+ * 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/connector.h>
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Guillaume Thouvenin <guillaume.thouvenin@bull.net>");
+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 };
+
+/**
+ * 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 = (struct cn_msg *)data;
+
+	if (cn_already_initialized && (msg->len == sizeof(cn_fork_enable)))
+		memcpy(&cn_fork_enable, msg->data, sizeof(cn_fork_enable));
+}
+
+/**
+ * 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);

diff -uprN -X dontdiff linux-2.6.11-rc4-mm1/drivers/connector/Kconfig linux-2.6.11-rc4-mm1-cnfork/drivers/connector/Kconfig

--- linux-2.6.11-rc4-mm1/drivers/connector/Kconfig	2005-02-23 11:12:15.000000000 +0100
+++ linux-2.6.11-rc4-mm1-cnfork/drivers/connector/Kconfig	2005-02-24 10:29:11.000000000 +0100

@@ -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"
+	depends on CONNECTOR=y
+	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

diff -uprN -X dontdiff linux-2.6.11-rc4-mm1/drivers/connector/Makefile linux-2.6.11-rc4-mm1-cnfork/drivers/connector/Makefile

--- linux-2.6.11-rc4-mm1/drivers/connector/Makefile	2005-02-23 11:12:15.000000000 +0100
+++ linux-2.6.11-rc4-mm1-cnfork/drivers/connector/Makefile	2005-02-25 13:49:57.000000000 +0100

@@ -1,2 +1,3 @@
 obj-$(CONFIG_CONNECTOR)		+= cn.o
+obj-$(CONFIG_FORK_CONNECTOR)	+= cn_fork.o 
 cn-objs		:= cn_queue.o connector.o

diff -uprN -X dontdiff linux-2.6.11-rc4-mm1/include/linux/connector.h linux-2.6.11-rc4-mm1-cnfork/include/linux/connector.h

--- linux-2.6.11-rc4-mm1/include/linux/connector.h	2005-02-23 11:12:17.000000000 +0100
+++ linux-2.6.11-rc4-mm1-cnfork/include/linux/connector.h	2005-03-01 12:44:50.000000000 +0100

@@ -28,6 +28,8 @@
 #define CN_VAL_KOBJECT_UEVENT		0x0000
 #define CN_IDX_SUPERIO			0xaabb  /* SuperIO subsystem */
 #define CN_VAL_SUPERIO			0xccdd
+#define CN_IDX_FORK			0xfeed  /* fork events */
+#define CN_VAL_FORK			0xbeef
 
 
 #define CONNECTOR_MAX_MSG_SIZE 	1024

@@ -133,6 +135,8 @@ struct cn_dev
 };
 
 extern int cn_already_initialized;
+extern int cn_fork_enable;
+extern struct cb_id cb_fork_id;
 
 int cn_add_callback(struct cb_id *, char *, void (* callback)(void *));
 void cn_del_callback(struct cb_id *);

diff -uprN -X dontdiff linux-2.6.11-rc4-mm1/kernel/fork.c linux-2.6.11-rc4-mm1-cnfork/kernel/fork.c

--- linux-2.6.11-rc4-mm1/kernel/fork.c	2005-02-23 11:12:17.000000000 +0100
+++ linux-2.6.11-rc4-mm1-cnfork/kernel/fork.c	2005-03-01 08:39:13.000000000 +0100

@@ -41,6 +41,7 @@
 #include <linux/profile.h>
 #include <linux/rmap.h>
 #include <linux/acct.h>
+#include <linux/connector.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

@@ -63,6 +64,47 @@ DEFINE_PER_CPU(unsigned long, process_co
 
 EXPORT_SYMBOL(tasklist_lock);
 
+#ifdef CONFIG_FORK_CONNECTOR
+
+#define CN_FORK_INFO_SIZE	64
+#define CN_FORK_MSG_SIZE 	(sizeof(struct cn_msg) + CN_FORK_INFO_SIZE)
+
+static inline void fork_connector(pid_t parent, pid_t child)
+{
+	static DEFINE_SPINLOCK(cn_fork_lock);
+	static __u32 seq;   /* used to test if message is lost */
+
+	if (cn_fork_enable) {
+		struct cn_msg *msg;
+
+		__u8 buffer[CN_FORK_MSG_SIZE];	
+
+		msg = (struct cn_msg *)buffer;
+			
+		memcpy(&msg->id, &cb_fork_id, sizeof(msg->id));
+		spin_lock(&cn_fork_lock);
+		msg->seq = seq++;
+		spin_unlock(&cn_fork_lock);
+		msg->ack = 0; /* not used */
+		/* 
+		 * size of data is the number of characters 
+		 * printed plus one for the trailing '\0'
+		 */
+		/* just fill the data part with '\0' */
+		memset(msg->data, '\0', CN_FORK_INFO_SIZE);
+		msg->len = scnprintf(msg->data, CN_FORK_INFO_SIZE-1, 
+				    "%i %i", parent, child) + 1;
+
+		cn_netlink_send(msg, CN_IDX_FORK);
+	}
+}
+#else
+static inline void fork_connector(pid_t parent, pid_t child) 
+{
+	return; 
+}
+#endif
+
 int nr_processes(void)
 {
 	int cpu;

@@ -1238,6 +1280,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);

Guillaume wrote:

  I also run the lmbench and results are send in response to another
thread "A common layer for Accounting packages". When fork connector is
turned off the overhead is negligible. 

Good.

If I read this code right:

+static inline void fork_connector(pid_t parent, pid_t child)
+{
+	static DEFINE_SPINLOCK(cn_fork_lock);
+	static __u32 seq;   /* used to test if message is lost */
+
+	if (cn_fork_enable) {

then the code executed if the fork connector is off is a call to an
inline function that tests an integer, finds it zero, and returns.

This is sufficiently little code that I for one would hardly
even need lmbench to be comfortable that fork() wasn't impacted
seriously, in the case that the fork connector is disabled.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson [off-list ref] 1.650.933.1373, 1.925.600.0401

In addition to worrying about performance and scaling, with accounting
enabled or disabled, one should also try to minimize code clutter in key
kernel files, such as fork.c

For example, one might, instead of adding 40 lines os fork_connector()
code to kernel/fork.c, instead add something like just the #include
<linux/connector.h> and the "fork_connector(current->pid, p->pid)" call
to kernel/fork.c, where include/linux/connector.h had something like:

	#ifdef CONFIG_FORK_CONNECTOR
	static inline void fork_connector(pid_t parent, pid_t child)
	{
		if (cn_fork_enable)
			__fork_connector(parent, child);
	}
	#else
	static inline void fork_connector(pid_t parent, pid_t child) {}
	#endif

Then bury the interesting code in the implementation of __fork_connector(),
in drivers/connector/cn_fork.c or some such place.

This adds a real function call in the case that cn_fork_enable is set.
That code path requires more than that anyway (and it makes kernel stack
backtraces more transparent).

But it removes 40 lines of fork_connector detail from fork.c.  And it
avoids marking a 40 line routine as inline ...

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson [off-list ref] 1.650.933.1373, 1.925.600.0401

On Wednesday, March 2, 2005 6:51 am, Paul Jackson wrote:

Guillaume wrote:

quoted

  I also run the lmbench and results are send in response to another
thread "A common layer for Accounting packages". When fork connector is
turned off the overhead is negligible.

Good.

If I read this code right:

quoted

+static inline void fork_connector(pid_t parent, pid_t child)
+{
+ static DEFINE_SPINLOCK(cn_fork_lock);
+ static __u32 seq;   /* used to test if message is lost */
+
+ if (cn_fork_enable) {

then the code executed if the fork connector is off is a call to an
inline function that tests an integer, finds it zero, and returns.

This is sufficiently little code that I for one would hardly
even need lmbench to be comfortable that fork() wasn't impacted
seriously, in the case that the fork connector is disabled.

But if it *is* enabled, it takes a global lock on every fork.  That can't 
scale on a big multiprocessor if lots of CPUs are doing lots of forks...

Jesse

Hello, Guillaume

I tried to measure the process-creation/destruction performance on 2.6.11-rc4-mm1 plus
some extensiton(Normal/with PAGG/with Fork-Connector).
But I received a following messages endlessly on system console with Fork-Connector extensiton.

# on IA-64 environment / When an simple fork() iteration is run in parallel.
skb does not have enough length: requested msg->len=10[28], nlh->nlmsg_len=48[32], skb->len=48[must be 30].
skb does not have enough length: requested msg->len=10[28], nlh->nlmsg_len=48[32], skb->len=48[must be 30].
skb does not have enough length: requested msg->len=10[28], nlh->nlmsg_len=48[32], skb->len=48[must be 30].
  :

Is's generated at drivers/connector/connector.c:__cn_rx_skb(), and this warn the length of msg's payload
does not fit in nlmsghdr's length.
This message means netlink packet is not sent to user space.
I was notified occurence of fork() by printk(). :-(

The attached simple *.c file can enable/disable fork-connector and listen the fork-notification.
Because It's first experimence for me to write a code to use netlink, point out a right how-to-use
if there's some mistakes at user side apprication.

Thanks.

P.S. I can't reproduce lockup on 367th-fork() with your latest patch.

Guillaume Thouvenin wrote:

  ChangeLog:

    - Add parenthesis around sizeof(struct cn_msg) + CN_FORK_INFO_SIZE
      in the CN_FORK_MSG_SIZE macro
    - fork_cn_lock is declareed with DEFINE_SPINLOCK()
    - fork_cn_lock is defined as static and local to fork_connector()
    - Create a specific module cn_fork.c in drivers/connector to
      register the callback.
    - Improve the callback that turns on/off the fork connector

  I also run the lmbench and results are send in response to another
thread "A common layer for Accounting packages". When fork connector is
turned off the overhead is negligible. This patch works with another
small patch that fix a problem in the connector. Without it, there is a
message that says "skb does not have enough length". It will be fix in
the next -mm tree I think.


Thanks everyone for the comments,
Guillaume

-- 
Linux Promotion Center, NEC
KaiGai Kohei [off-list ref]