Re: [PATCH 2.6.11-rc3-mm2] connector: Add a fork connector

[Andrew Morton <akpm@osdl.org>]

Guillaume Thouvenin <guillaume.thouvenin@bull.net> wrote:
>
> Hello,
> 
>   This patch replaces the relay_fork module and it implements a fork
> connector in the kernel/fork.c:do_fork() routine. The connector sends
> information about parent PID and child PID over a netlink interface. It
> allows to several user space applications to be informed when a fork
> occurs in the kernel. The main drawback is that even if nobody listens,
> message is send. I don't know how to avoid that.

We really should find a way to fix that.  Especially if we want all the
distributors to enable the connector in their builds (we do).

What happened to the idea of sending an on/off message down the netlink
socket?

> +#ifdef CONFIG_FORK_CONNECTOR
> +#define FORK_CN_INFO_SIZE	64 
> +static inline void fork_connector(pid_t parent, pid_t child)
> +{
> +	struct cb_id fork_id = {CN_IDX_FORK, CN_VAL_FORK};

This can be static const, which will save some stack and cycles.

> +	static __u32 seq; /* used to test if we lost message */
> +	
> +	if (cn_already_initialized) {
> +		struct cn_msg *msg;
> +		size_t size;
> +
> +		size = sizeof(*msg) + FORK_CN_INFO_SIZE;
> +		msg = kmalloc(size, GFP_KERNEL);
> +		if (msg) {
> +			memset(msg, '\0', size);

Do we really need to memset the whole thing?

> +			memcpy(&msg->id, &fork_id, sizeof(msg->id));
> +			msg->seq = seq++;

`seq' needs a lock to protect it.  Or use atomic_add_return(), maybe.

> +			msg->ack = 0; /* not used */
> +			/* 
> +			 * size of data is the number of characters 
> +			 * printed plus one for the trailing '\0'
> +			 */
> +			msg->len = snprintf(msg->data, FORK_CN_INFO_SIZE-1, 
> +					    "%i %i", parent, child) + 1;

scnprintf() would be more appropriate here, even though it should be a "can't
happen".

> +			cn_netlink_send(msg, CN_IDX_FORK);
> +
> +			kfree(msg);

`msg' is only 84 bytes and do_fork() has a shallow call graph.  Make `msg'
a local?