[RFC][PATCH 1/3] Process events biarch bug: Name process event data union type and annotate for compatibility.

[RFC][PATCH 1/3] Process events biarch bug: Name process event data union type and annotate for compatibility.

[Matt Helsley]

This patch adds an explicit type to the process events union structure so the
type may be reused. 

Signed-off-by: Matt Helsley <matthltc@us.ibm.com>
---
 include/linux/cn_proc.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.17-mm3-biarch/include/linux/cn_proc.h
===================================================================
--- linux-2.6.17-mm3-biarch.orig/include/linux/cn_proc.h
+++ linux-2.6.17-mm3-biarch/include/linux/cn_proc.h
@@ -56,11 +56,11 @@ struct proc_event {
 		/* "last" is the last process event: exit */
 		PROC_EVENT_EXIT = 0x80000000
 	} what;
 	__u32 cpu;
 	struct timespec timestamp;
-	union { /* must be last field of proc_event struct */
+	union process_event_data { /* must be last field of proc_event struct */
 		struct {
 			__u32 err;
 		} ack;
 
 		struct fork_proc_event {

--

[RFC][PATCH 2/3] Process events biarch bug: Process events timestamp bug

[Matt Helsley]

The event sent by Process Events from a 64-bit kernel is not compatible with
what a 32-bit userspace program would expect to recieve because the timespec
struct (used to send a timestamp) is not the same. This means that fields stored
after the timestamp are offset and programs that don't take this into account
break under these circumstances.

This is a problem on ppc64, s390, x86-64.. any "biarch" system where it's
possible for a 64-bit kernel to run 32-bit userspace programs.

This patch adds a comment highlighting the problem and a Documentation file
showing a userspace workaround.

Signed-off-by: Matt Helsley <matthltc@us.ibm.com>
---
 Documentation/connector/process_events.txt |   75 +++++++++++++++++++++++++++++
 drivers/connector/Kconfig                  |    3 +
 include/linux/cn_proc.h                    |    7 ++
 3 files changed, 85 insertions(+)

Index: linux-2.6.17-mm3-biarch/include/linux/cn_proc.h
===================================================================
--- linux-2.6.17-mm3-biarch.orig/include/linux/cn_proc.h
+++ linux-2.6.17-mm3-biarch/include/linux/cn_proc.h
@@ -55,11 +55,18 @@ struct proc_event {
 		/* "next" should be 0x00000400 */
 		/* "last" is the last process event: exit */
 		PROC_EVENT_EXIT = 0x80000000
 	} what;
 	__u32 cpu;
+
+ 	/*
+ 	 * UGH! timespec is biarch incompatible. See
+ 	 * Documentation/connector/process_events.txt if you're going to listen
+ 	 * for process events in userspace.
+ 	 */
 	struct timespec timestamp;
+
 	union process_event_data { /* must be last field of proc_event struct */
 		struct {
 			__u32 err;
 		} ack;
 
Index: linux-2.6.17-mm3-biarch/Documentation/connector/process_events.txt
===================================================================
--- /dev/null
+++ linux-2.6.17-mm3-biarch/Documentation/connector/process_events.txt
@@ -0,0 +1,75 @@
+NOTES:
+
+To safely use process events on biarch systems it's necessary to account for
+the difference in size of the events returned by 64-bit kernels to 32-bit
+applications. This size difference is caused by use of a struct timespec for
+the timestamp field.
+
+There are multiple ways of doing this. However, few of them account for the
+fact that the size of the process event structure may change in the future.
+
+#include "linux/cn_proc.h"
+
+{
+	...
+	struct process_event *ev;
+	union process_event_data *evdata;
+	struct timespec ts;
+
+	ev = cn_msg->data;
+	if ((cn_msg->len - sizeof(struct process_event)) == sizeof(__u64)) {
+		__u64 *v;
+
+		/* kernel is sending 64-bit timestamps to 32-bit userspace */
+
+		v = (*__u64)&ev->timestamp;
+		ts.tv_sec = *v;
+		v++;
+		ts.tv_nsec = *v;
+		v++;
+		evdata = (union process_event_data*)v;
+	} else {
+		evdata = &ev->event_data;
+		ts = ev->timestamp;
+	}
+
+	switch(ev->what) {
+	case PROCESS_EVENT_EXIT:
+		... evdata->exit.exit_code ...
+		break;
+	}
+}
+
+Alternately, you can compare the size of the message length to the known
+size of the two structure variations and thus determine which structure to
+use to access the data:
+
+struct pev32 {
+	enum what what;
+	__u32 cpu;
+	__u32 timestamp[2];
+	union process_event_data event_data;
+};
+
+struct pev64 {
+	enum what what;
+	__u32 cpu;
+	__u64 timestamp[2];
+	union process_event_data event_data;
+};
+
+...
+if (cn_msg->len == sizeof(struct pev64)) {
+	...
+} else if (cn_msg->len == sizeof(struct pev32)) {
+	...
+}
+
+Both approaches assume that the process events structure will never change
+size.
+
+To limit the number of times you need to test which format is being sent use
+the acknowledgement message returned by the kernel after sending
+PROC_CN_MCAST_LISTEN. Use any of the techniques described above on the
+acknowledgement message and then set function pointers, flags, etc. to avoid
+the test in the future.
Index: linux-2.6.17-mm3-biarch/drivers/connector/Kconfig
===================================================================
--- linux-2.6.17-mm3-biarch.orig/drivers/connector/Kconfig
+++ linux-2.6.17-mm3-biarch/drivers/connector/Kconfig
@@ -16,6 +16,9 @@ config PROC_EVENTS
 	depends on CONNECTOR
 	help
 	  Provide a connector that reports process events to userspace. Send
 	  events such as fork, exec, id change (uid, gid, suid, etc), and exit.
 
+	  See Documentation/connector/process_events.txt if you're going to
+	  listen for process events in userspace.
+
 endmenu

--

[RFC][PATCH 3/3] Process events biarch bug: New process events connector value

[Matt Helsley]

"Deprecate" existing Process Events connector interface and add a new one
that works cleanly on biarch platforms.

Any expansion of the previous event structure would break userspace's ability
to workaround the biarch incompatibility problem. Hence this patch creates a
new interface and generates events (for both when necessary).

Signed-off-by: Matt Helsley <matthltc@us.ibm.com>
---
 drivers/connector/cn_proc.c |   65 +++++++++++++++++++++++++++++---------------
 include/linux/cn_proc.h     |   23 ++++++++++-----
 include/linux/connector.h   |    3 +-
 3 files changed, 61 insertions(+), 30 deletions(-)

Index: linux-2.6.17-mm3-biarch/include/linux/connector.h
===================================================================
--- linux-2.6.17-mm3-biarch.orig/include/linux/connector.h
+++ linux-2.6.17-mm3-biarch/include/linux/connector.h
@@ -29,11 +29,12 @@
 
 /*
  * Process Events connector unique ids -- used for message routing
  */
 #define CN_IDX_PROC			0x1
-#define CN_VAL_PROC			0x1
+#define CN_VAL_PROC_DEPRECATED		0x1 /* deprecated */
+#define CN_VAL_PROC			0x2
 #define CN_IDX_CIFS			0x2
 #define CN_VAL_CIFS                     0x1
 #define CN_W1_IDX			0x3	/* w1 communication */
 #define CN_W1_VAL			0x1
 
Index: linux-2.6.17-mm3-biarch/drivers/connector/cn_proc.c
===================================================================
--- linux-2.6.17-mm3-biarch.orig/drivers/connector/cn_proc.c
+++ linux-2.6.17-mm3-biarch/drivers/connector/cn_proc.c
@@ -30,13 +30,19 @@
 #include <linux/notifier.h>
 #include <asm/atomic.h>
 
 #include <linux/cn_proc.h>
 
-#define CN_PROC_MSG_SIZE (sizeof(struct cn_msg) + sizeof(struct proc_event))
-
-static atomic_t proc_event_num_listeners = ATOMIC_INIT(0);
+#define CN_PROC_MSG_SIZE (sizeof(struct cn_msg) + \
+			  max(sizeof(struct proc_event), \
+			      sizeof(struct proc_event_deprecated)))
+
+static atomic_t proc_event_num_old_listeners = ATOMIC_INIT(0);
+static struct cb_id cn_proc_event_deprecated_id = {
+	CN_IDX_PROC,
+	CN_VAL_PROC_DEPRECATED
+};
 static struct cb_id cn_proc_event_id = { CN_IDX_PROC, CN_VAL_PROC };
 
 /* proc_event_counts is used as the sequence number of the netlink message */
 static DEFINE_PER_CPU(__u32, proc_event_counts) = { 0 };
 
@@ -100,18 +106,18 @@ static inline void proc_exit_connector(s
  * mechanisms.
  */
 static void cn_proc_ack(int err, int rcvd_seq, int rcvd_ack)
 {
 	struct cn_msg *msg;
-	struct proc_event *ev;
+	struct proc_event_deprecated *ev;
 	__u8 buffer[CN_PROC_MSG_SIZE];
 
-	if (atomic_read(&proc_event_num_listeners) < 1)
+	if (atomic_read(&proc_event_num_old_listeners) < 1)
 		return;
 
 	msg = (struct cn_msg*)buffer;
-	ev = (struct proc_event*)msg->data;
+	ev = (struct proc_event_deprecated*)msg->data;
 	msg->seq = rcvd_seq;
 	ktime_get_ts(&ev->timestamp);
 	ev->cpu = -1;
 	ev->what = PROC_EVENT_NONE;
 	ev->event_data.ack.err = err;
@@ -123,11 +129,11 @@ static void cn_proc_ack(int err, int rcv
 
 /**
  * cn_proc_mcast_ctl
  * @data: message sent from userspace via the connector
  */
-static void cn_proc_mcast_ctl(void *data)
+static void cn_proc_mcast_old_ctl(void *data)
 {
 	struct cn_msg *msg = data;
 	enum proc_cn_mcast_op *mc_op = NULL;
 	int err = 0;
 
@@ -135,14 +141,14 @@ static void cn_proc_mcast_ctl(void *data
 		return;
 
 	mc_op = (enum proc_cn_mcast_op*)msg->data;
 	switch (*mc_op) {
 	case PROC_CN_MCAST_LISTEN:
-		atomic_inc(&proc_event_num_listeners);
+		atomic_inc(&proc_event_num_old_listeners);
 		break;
 	case PROC_CN_MCAST_IGNORE:
-		atomic_dec(&proc_event_num_listeners);
+		atomic_dec(&proc_event_num_old_listeners);
 		break;
 	default:
 		err = EINVAL;
 		break;
 	}
@@ -158,16 +164,15 @@ static int cn_proc_watch_task(struct not
 			      void *t)
 {
 	struct task_struct *task = t;
 	struct cn_msg *msg;
 	struct proc_event *ev;
+	struct proc_event_deprecated *ev_old;
+	struct timespec timestamp;
 	__u8 buffer[CN_PROC_MSG_SIZE];
 	int rc = NOTIFY_OK;
 
-	if (atomic_read(&proc_event_num_listeners) < 1)
-		return rc;
-
 	msg = (struct cn_msg*)buffer;
 	ev = (struct proc_event*)msg->data;
 	switch (get_watch_event(val)) {
 	case WATCH_TASK_CLONE:
 		proc_fork_connector(task, ev);
@@ -189,16 +194,26 @@ static int cn_proc_watch_task(struct not
 		break;
 	}
 	if (rc != NOTIFY_OK)
 		return rc;
 	get_seq(&msg->seq, &ev->cpu);
-	ktime_get_ts(&ev->timestamp); /* get high res monotonic timestamp */
+	ktime_get_ts(&timestamp); /* get high res monotonic timestamp */
+	ev->timestamp_ns = ((__u64)timestamp.tv_sec*(__u64)NSEC_PER_SEC) + (__u64)timestamp.tv_nsec;
 	memcpy(&msg->id, &cn_proc_event_id, sizeof(msg->id));
 	msg->ack = 0; /* not used */
 	msg->len = sizeof(*ev);
 	cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
 	/* If cn_netlink_send() fails, drop data */
+
+	if (atomic_read(&proc_event_num_old_listeners) < 1)
+		return rc;
+	ev_old = (struct proc_event_deprecated*)msg->data;
+	msg->len = sizeof(*ev_old);
+	memmove(&ev_old->event_data, &ev->event_data, sizeof(ev->event_data));
+	memcpy(&ev_old->timestamp, &timestamp, sizeof(timestamp));
+	cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
+	/* If cn_netlink_send() fails, drop data */
 	return rc;
 }
 
 static struct notifier_block __read_mostly cn_proc_nb = {
 	.notifier_call = cn_proc_watch_task,
@@ -211,20 +226,27 @@ static struct notifier_block __read_most
  */
 static int __init cn_proc_init(void)
 {
 	int err;
 
-	if ((err = cn_add_callback(&cn_proc_event_id, "cn_proc",
-	 			   &cn_proc_mcast_ctl))) {
-		printk(KERN_WARNING "cn_proc failed to register\n");
-		goto out;
-	}
+	err = cn_add_callback(&cn_proc_event_deprecated_id, "cn_proc_old",
+			      &cn_proc_mcast_old_ctl);
+	if (err)
+		goto error_old;
+	err = cn_add_callback(&cn_proc_event_id, "cn_proc", NULL);
+	if (err)
+		goto error;
 
 	err = register_task_watcher(&cn_proc_nb);
-	if (err != 0)
-		cn_del_callback(&cn_proc_event_id);
-out:
+	if (err)
+		goto error;
+	return err;
+error:
+	cn_del_callback(&cn_proc_event_id);
+error_old:
+	cn_del_callback(&cn_proc_event_deprecated_id);
+	printk(KERN_WARNING "cn_proc failed to register\n");
 	return err;
 }
 
 static void cn_proc_fini(void)
 {
@@ -234,10 +256,11 @@ static void cn_proc_fini(void)
 	if (err != 0)
 		printk(KERN_WARNING
 		       "cn_proc failed to unregister its task notify block\n");
 
 	cn_del_callback(&cn_proc_event_id);
+	cn_del_callback(&cn_proc_event_deprecated_id);
 }
 
 module_init(cn_proc_init);
 module_exit(cn_proc_fini);
 
Index: linux-2.6.17-mm3-biarch/include/linux/cn_proc.h
===================================================================
--- linux-2.6.17-mm3-biarch.orig/include/linux/cn_proc.h
+++ linux-2.6.17-mm3-biarch/include/linux/cn_proc.h
@@ -55,18 +55,11 @@ struct proc_event {
 		/* "next" should be 0x00000400 */
 		/* "last" is the last process event: exit */
 		PROC_EVENT_EXIT = 0x80000000
 	} what;
 	__u32 cpu;
-
- 	/*
- 	 * UGH! timespec is biarch incompatible. See
- 	 * Documentation/connector/process_events.txt if you're going to listen
- 	 * for process events in userspace.
- 	 */
-	struct timespec timestamp;
-
+	__u64 timestamp_ns;
 	union process_event_data { /* must be last field of proc_event struct */
 		struct {
 			__u32 err;
 		} ack;
 
@@ -100,6 +93,20 @@ struct proc_event {
 			pid_t process_tgid;
 			__u32 exit_code, exit_signal;
 		} exit;
 	} event_data;
 };
+
+struct proc_event_deprecated {
+	enum what what;
+	__u32 cpu;
+
+	/*
+	 * UGH! timespec is biarch incompatible. See
+	 * Documentation/connector/process_events.txt if you're going to listen
+	 * for process events in userspace.
+	 */
+	struct timespec timestamp;
+	union process_event_data event_data;
+};
+
 #endif	/* CN_PROC_H */

--

Re: [RFC][PATCH 3/3] Process events biarch bug: New process events connector value

[Chandra Seetharaman]

On Tue, 2006-06-27 at 04:49 -0700, Matt Helsley wrote:
> "Deprecate" existing Process Events connector interface and add a new one
> that works cleanly on biarch platforms.
> 
> Any expansion of the previous event structure would break userspace's ability
> to workaround the biarch incompatibility problem. Hence this patch creates a
> new interface and generates events (for both when necessary).

Is there a reason why the # of listeners part is removed (basically the
LISTEN/IGNORE) ? and why as part of this patch ?

<snip>

> @@ -158,16 +164,15 @@ static int cn_proc_watch_task(struct not
>  			      void *t)
>  {
>  	struct task_struct *task = t;
>  	struct cn_msg *msg;
>  	struct proc_event *ev;
> +	struct proc_event_deprecated *ev_old;
> +	struct timespec timestamp;
>  	__u8 buffer[CN_PROC_MSG_SIZE];
>  	int rc = NOTIFY_OK;
>  
> -	if (atomic_read(&proc_event_num_listeners) < 1)
> -		return rc;
> -
>  	msg = (struct cn_msg*)buffer;
>  	ev = (struct proc_event*)msg->data;
>  	switch (get_watch_event(val)) {
>  	case WATCH_TASK_CLONE:
>  		proc_fork_connector(task, ev);
> @@ -189,16 +194,26 @@ static int cn_proc_watch_task(struct not
>  		break;
>  	}
>  	if (rc != NOTIFY_OK)
>  		return rc;
>  	get_seq(&msg->seq, &ev->cpu);
> -	ktime_get_ts(&ev->timestamp); /* get high res monotonic timestamp */
> +	ktime_get_ts(&timestamp); /* get high res monotonic timestamp */
> +	ev->timestamp_ns = ((__u64)timestamp.tv_sec*(__u64)NSEC_PER_SEC) + (__u64)timestamp.tv_nsec;
>  	memcpy(&msg->id, &cn_proc_event_id, sizeof(msg->id));
>  	msg->ack = 0; /* not used */
>  	msg->len = sizeof(*ev);
>  	cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
>  	/* If cn_netlink_send() fails, drop data */
> +
> +	if (atomic_read(&proc_event_num_old_listeners) < 1)
> +		return rc;
> +	ev_old = (struct proc_event_deprecated*)msg->data;
> +	msg->len = sizeof(*ev_old);

A comment saying the fields cpu, what, and ack are filled above and is
valid as is would help.

> +	memmove(&ev_old->event_data, &ev->event_data, sizeof(ev->event_data));
> +	memcpy(&ev_old->timestamp, &timestamp, sizeof(timestamp));
> +	cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
> +	/* If cn_netlink_send() fails, drop data */
>  	return rc;
>  }
>  
>  static struct notifier_block __read_mostly cn_proc_nb = {
>  	.notifier_call = cn_proc_watch_task,
> @@ -211,20 +226,27 @@ static struct notifier_block __read_most
>   */
>  static int __init cn_proc_init(void)
>  {
>  	int err;
>  
> -	if ((err = cn_add_callback(&cn_proc_event_id, "cn_proc",
> -	 			   &cn_proc_mcast_ctl))) {
> -		printk(KERN_WARNING "cn_proc failed to register\n");
> -		goto out;
> -	}
> +	err = cn_add_callback(&cn_proc_event_deprecated_id, "cn_proc_old",
> +			      &cn_proc_mcast_old_ctl);
> +	if (err)
> +		goto error_old;


> +	err = cn_add_callback(&cn_proc_event_id, "cn_proc", NULL);

is this needed if you are not going to have the callback ?

> +	if (err)
> +		goto error;
should not try to cn_del_callback(&cn_proc_event_id) ?! (goto error_old;
perhaps)

>  
>  	err = register_task_watcher(&cn_proc_nb);
> -	if (err != 0)
> -		cn_del_callback(&cn_proc_event_id);
> -out:
> +	if (err)
> +		goto error;
> +	return err;
> +error:
> +	cn_del_callback(&cn_proc_event_id);
> +error_old:
> +	cn_del_callback(&cn_proc_event_deprecated_id);
> +	printk(KERN_WARNING "cn_proc failed to register\n");
>  	return err;
>  }
>  
<snip>
-- 

----------------------------------------------------------------------
    Chandra Seetharaman               | Be careful what you choose....
              - sekharan@us.ibm.com   |      .......you may get it.
----------------------------------------------------------------------

Re: [RFC][PATCH 3/3] Process events biarch bug: New process events connector value

[Matt Helsley]

On Tue, 2006-06-27 at 12:14 -0700, Chandra Seetharaman wrote:
> On Tue, 2006-06-27 at 04:49 -0700, Matt Helsley wrote:
> > "Deprecate" existing Process Events connector interface and add a new one
> > that works cleanly on biarch platforms.
> > 
> > Any expansion of the previous event structure would break userspace's ability
> > to workaround the biarch incompatibility problem. Hence this patch creates a
> > new interface and generates events (for both when necessary).
> 
> Is there a reason why the # of listeners part is removed (basically the
> LISTEN/IGNORE) ? and why as part of this patch ?

	Michael Kerrisk had some objections to LISTEN/IGNORE and I've been
looking into making a connector function that would replace them. They
exist primarily to improve performance by avoiding the memory allocation
in cn_netlink_send() when there are no listeners.

> <snip>
> 
> > @@ -158,16 +164,15 @@ static int cn_proc_watch_task(struct not
> >  			      void *t)
> >  {
> >  	struct task_struct *task = t;
> >  	struct cn_msg *msg;
> >  	struct proc_event *ev;
> > +	struct proc_event_deprecated *ev_old;
> > +	struct timespec timestamp;
> >  	__u8 buffer[CN_PROC_MSG_SIZE];
> >  	int rc = NOTIFY_OK;
> >  
> > -	if (atomic_read(&proc_event_num_listeners) < 1)
> > -		return rc;
> > -
> >  	msg = (struct cn_msg*)buffer;
> >  	ev = (struct proc_event*)msg->data;
> >  	switch (get_watch_event(val)) {
> >  	case WATCH_TASK_CLONE:
> >  		proc_fork_connector(task, ev);
> > @@ -189,16 +194,26 @@ static int cn_proc_watch_task(struct not
> >  		break;
> >  	}
> >  	if (rc != NOTIFY_OK)
> >  		return rc;
> >  	get_seq(&msg->seq, &ev->cpu);
> > -	ktime_get_ts(&ev->timestamp); /* get high res monotonic timestamp */
> > +	ktime_get_ts(&timestamp); /* get high res monotonic timestamp */
> > +	ev->timestamp_ns = ((__u64)timestamp.tv_sec*(__u64)NSEC_PER_SEC) + (__u64)timestamp.tv_nsec;
> >  	memcpy(&msg->id, &cn_proc_event_id, sizeof(msg->id));
> >  	msg->ack = 0; /* not used */
> >  	msg->len = sizeof(*ev);
> >  	cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
> >  	/* If cn_netlink_send() fails, drop data */
> > +
> > +	if (atomic_read(&proc_event_num_old_listeners) < 1)
> > +		return rc;
> > +	ev_old = (struct proc_event_deprecated*)msg->data;
> > +	msg->len = sizeof(*ev_old);
> 
> A comment saying the fields cpu, what, and ack are filled above and is
> valid as is would help.

OK, good idea.

> > +	memmove(&ev_old->event_data, &ev->event_data, sizeof(ev->event_data));
> > +	memcpy(&ev_old->timestamp, &timestamp, sizeof(timestamp));
> > +	cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
> > +	/* If cn_netlink_send() fails, drop data */
> >  	return rc;
> >  }
> >  
> >  static struct notifier_block __read_mostly cn_proc_nb = {
> >  	.notifier_call = cn_proc_watch_task,
> > @@ -211,20 +226,27 @@ static struct notifier_block __read_most
> >   */
> >  static int __init cn_proc_init(void)
> >  {
> >  	int err;
> >  
> > -	if ((err = cn_add_callback(&cn_proc_event_id, "cn_proc",
> > -	 			   &cn_proc_mcast_ctl))) {
> > -		printk(KERN_WARNING "cn_proc failed to register\n");
> > -		goto out;
> > -	}
> > +	err = cn_add_callback(&cn_proc_event_deprecated_id, "cn_proc_old",
> > +			      &cn_proc_mcast_old_ctl);
> > +	if (err)
> > +		goto error_old;
> 
> 
> > +	err = cn_add_callback(&cn_proc_event_id, "cn_proc", NULL);
> 
> is this needed if you are not going to have the callback ?

I believe so. Evgeniy?

> > +	if (err)
> > +		goto error;
> should not try to cn_del_callback(&cn_proc_event_id) ?! (goto error_old;
> perhaps)

Ack! Good catch.

Thanks,
	-Matt Helsley

Re: [RFC][PATCH 3/3] Process events biarch bug: New process events connector value

[Chandra Seetharaman]

On Tue, 2006-06-27 at 14:39 -0700, Matt Helsley wrote:
> On Tue, 2006-06-27 at 12:14 -0700, Chandra Seetharaman wrote:
> > On Tue, 2006-06-27 at 04:49 -0700, Matt Helsley wrote:
> > > "Deprecate" existing Process Events connector interface and add a new one
> > > that works cleanly on biarch platforms.
> > > 
> > > Any expansion of the previous event structure would break userspace's ability
> > > to workaround the biarch incompatibility problem. Hence this patch creates a
> > > new interface and generates events (for both when necessary).
> > 
> > Is there a reason why the # of listeners part is removed (basically the
> > LISTEN/IGNORE) ? and why as part of this patch ?
> 
> 	Michael Kerrisk had some objections to LISTEN/IGNORE and I've been
> looking into making a connector function that would replace them. They
> exist primarily to improve performance by avoiding the memory allocation
> in cn_netlink_send() when there are no listeners.

If it not related this bug, can you please separate them.

<snip>
-- 

----------------------------------------------------------------------
    Chandra Seetharaman               | Be careful what you choose....
              - sekharan@us.ibm.com   |      .......you may get it.
----------------------------------------------------------------------

Re: [RFC][PATCH 3/3] Process events biarch bug: New process events connector value

[Matt Helsley]

On Tue, 2006-06-27 at 16:54 -0700, Chandra Seetharaman wrote:
> On Tue, 2006-06-27 at 14:39 -0700, Matt Helsley wrote:
> > On Tue, 2006-06-27 at 12:14 -0700, Chandra Seetharaman wrote:
<snip>

> > > Is there a reason why the # of listeners part is removed (basically the
> > > LISTEN/IGNORE) ? and why as part of this patch ?
> > 
> > 	Michael Kerrisk had some objections to LISTEN/IGNORE and I've been
> > looking into making a connector function that would replace them. They
> > exist primarily to improve performance by avoiding the memory allocation
> > in cn_netlink_send() when there are no listeners.
> 
> If it not related this bug, can you please separate them.
> 
> <snip>

OK, I'll separate it for the next submission.

Cheers,
	-Matt Helsley

Re: [RFC][PATCH 3/3] Process events biarch bug: New process events connector value

[Evgeniy Polyakov]

On Tue, Jun 27, 2006 at 02:39:42PM -0700, Matt Helsley (matthltc@us.ibm.com) wrote:
> > Is there a reason why the # of listeners part is removed (basically the
> > LISTEN/IGNORE) ? and why as part of this patch ?
> 
> 	Michael Kerrisk had some objections to LISTEN/IGNORE and I've been
> looking into making a connector function that would replace them. They
> exist primarily to improve performance by avoiding the memory allocation
> in cn_netlink_send() when there are no listeners.

Connector supports check for listeners before allocation quite long ago.

> > > +	err = cn_add_callback(&cn_proc_event_id, "cn_proc", NULL);
> > 
> > is this needed if you are not going to have the callback ?
> 
> I believe so. Evgeniy?

Depending on how are you going to use it.
It is obviously required for receiving data from userspace, but if you
only want to send data from kernelspace you can use cn_netlink_send()
without registering callback, but in that case you must provide group
number to cn_netlink_send().
 
> Thanks,
> 	-Matt Helsley

-- 
	Evgeniy Polyakov

Re: [RFC][PATCH 3/3] Process events biarch bug: New process events connector value

[Evgeniy Polyakov]

Albert Cahalan wrote:
>I haven't yet seen a good explanation of what this is or does,
>but I suspect it may be useful for the "top" program or for a
>debugger. In either case, I am a highly interested party.
>I maintain top as part of the procps package. People pay me to
>hack on debuggers.

>Mind pointing me to some documentation and an explanation of why
>the feature was added? Is there a man page? (there should be)

There are a lot of goodies for process accounting there.
One can find initial draft with detailed description of the project 
goals at http://lwn.net/Articles/153694/

-- 
	Evgeniy Polyakov

Re: [RFC][PATCH 3/3] Process events biarch bug: New process events connector value

[Albert Cahalan]

Matt Helsley writes:

> "Deprecate" existing Process Events connector interface
> and add a new one that works cleanly on biarch platforms.
>
> Any expansion of the previous event structure would break
> userspace's ability to workaround the biarch incompatibility
> problem. Hence this patch creates a new interface and generates
> events (for both when necessary).

I haven't yet seen a good explanation of what this is or does,
but I suspect it may be useful for the "top" program or for a
debugger. In either case, I am a highly interested party.
I maintain top as part of the procps package. People pay me to
hack on debuggers.

Mind pointing me to some documentation and an explanation of why
the feature was added? Is there a man page? (there should be)

Perhaps there is no need to support 32-bit apps using this on
64-bit systems. Obviously the 32-on-64 nonsense must include
full debugger support (else you can't work on it very well),
and I suppose there is a desire to support various proprietary
bits of junk like the Flash player, but... there is a place to
just draw the line.