Re: [Patch][RFC] Disabling per-tgid stats on task exit in taskstats

Andrew Morton wrote:

On Mon, 03 Jul 2006 17:11:59 -0400
Shailabh Nagar [off-list ref] wrote:
 

quoted

static inline void taskstats_exit_alloc(struct taskstats **ptidstats)
{
	*ptidstats = NULL;
-	if (taskstats_has_listeners())
+	if (!list_empty(&get_cpu_var(listener_list)))
		*ptidstats = kmem_cache_zalloc(taskstats_cache, SLAB_KERNEL);
+	put_cpu_var(listener_list);
}
   

It's time to uninline this function..

 

quoted

static inline void taskstats_exit_free(struct taskstats *tidstats)
Index: linux-2.6.17-mm3equiv/kernel/taskstats.c
===================================================================

--- linux-2.6.17-mm3equiv.orig/kernel/taskstats.c	2006-06-30 23:38:39.000000000 -0400
+++ linux-2.6.17-mm3equiv/kernel/taskstats.c	2006-07-02 00:16:18.000000000 -0400

@@ -19,6 +19,8 @@

#include <linux/kernel.h>
#include <linux/taskstats_kern.h>
#include <linux/delayacct.h>
+#include <linux/cpumask.h>
+#include <linux/percpu.h>
#include <net/genetlink.h>
#include <asm/atomic.h>

@@ -26,6 +28,9 @@ static DEFINE_PER_CPU(__u32, taskstats_s

static int family_registered = 0;
kmem_cache_t *taskstats_cache;

+DEFINE_PER_CPU(struct list_head, listener_list);
+static DEFINE_PER_CPU(struct rw_semaphore, listener_list_sem);
   

Which will permit listener_list to become static - it wasn't a good name
for a global anyway.

I suggest you implement a new

struct whatever {
struct rw_semaphore sem;
struct list_head list;
};
 

Ok. The listener_list was a global to allow taskstats_exit_alloc to 
access but this is better.

static DEFINE_PER_CPU(struct whatever, listener_aray);


 

quoted

static int prepare_reply(struct genl_info *info, u8 cmd, struct sk_buff **skbp,
			void **replyp, size_t size)
{

@@ -77,6 +92,8 @@ static int prepare_reply(struct genl_inf

static int send_reply(struct sk_buff *skb, pid_t pid, int event)
{
	struct genlmsghdr *genlhdr = nlmsg_data((struct nlmsghdr *)skb->data);
+	struct rw_semaphore *sem;
+	struct list_head *p, *head;
	void *reply;
	int rc;

@@ -88,9 +105,30 @@ static int send_reply(struct sk_buff *sk

		return rc;
	}

-	if (event == TASKSTATS_MSG_MULTICAST)
-		return genlmsg_multicast(skb, pid, TASKSTATS_LISTEN_GROUP);
-	return genlmsg_unicast(skb, pid);
+	if (event == TASKSTATS_MSG_UNICAST)
+		return genlmsg_unicast(skb, pid);
+
+	/*
+	 * Taskstats multicast is unicasts to listeners who have registered
+	 * interest in this cpu
+	 */
+	sem = &get_cpu_var(listener_list_sem);
+	head = &get_cpu_var(listener_list);
   

This has a double preempt_disable(), but the above will fix that.

 

quoted

+	down_read(sem);
+	list_for_each(p, head) {
+		int ret;
+		struct listener *s = list_entry(p, struct listener, list);
+		ret = genlmsg_unicast(skb, s->pid);
+		if (ret)
+			rc = ret;
+	}
+	up_read(sem);
+
+	put_cpu_var(listener_list);
+	put_cpu_var(listener_list_sem);
+
+	return rc;
}

static int fill_pid(pid_t pid, struct task_struct *pidtsk,

@@ -201,8 +239,73 @@ ret:

	return;
}

+static int add_del_listener(pid_t pid, cpumask_t *maskp, int isadd)
+{
+	struct listener *s;
+	unsigned int cpu, mycpu;
+	cpumask_t mask;
+	struct rw_semaphore *sem;
+	struct list_head *head, *p;

-static int taskstats_send_stats(struct sk_buff *skb, struct genl_info *info)
+	memcpy(&mask, maskp, sizeof(cpumask_t));
+	if (cpus_empty(mask))
+		return -EINVAL;
+
+	mycpu = get_cpu();
+	put_cpu();
   

This is effectively raw_smp_processor_id().  And after the put_cpu(),
`mycpu' is meaningless.
 

Hmm.

quoted

+	if (isadd == REGISTER) {
+		for_each_cpu_mask(cpu, mask) {
+			if (!cpu_possible(cpu))
+				continue;
+			if (cpu == mycpu)
+				preempt_disable();
+
+			sem = &per_cpu(listener_list_sem, cpu);
+			head = &per_cpu(listener_list, cpu);
+
+			s = kmalloc(sizeof(struct listener), GFP_KERNEL);
   

Cannot do GFP_KERNEL inside preempt_disable().

There's no easy solution to this problem.  GFP_ATOMIC is not a good fix at
all.  One approach would be to run lock_cpu_hotplug(), then allocate (with
GFP_KERNEL) all the memory which will be needed within the locked region,
then take the lock, then use that preallocated memory.
 

You should use kmalloc_node() here, to ensure that the memory on each CPU's
list resides with that CPU's local memory (not _this_ CPU's local memory).
 

Ok.

quoted

+			if (!s)
+				return -ENOMEM;
+			s->pid = pid;
+			INIT_LIST_HEAD(&s->list);
+
+			down_write(sem);
+			list_add(&s->list, head);
+			up_write(sem);
+
+			if (cpu == mycpu)
+				preempt_enable();
   

Actually, I don't understand the tricks which are going on with the local CPU here. 
What's it all for?
 

I was wanting to do a  get_cpu_var  for listener_list & sem
for the current cpu and per_cpu otherwise (since thats what I thought 
was the recommendation
for accessing the local cpu's variable). Perhaps the preempt_disable is 
uncalled for ?

quoted

+		}
+	} else {
+		for_each_cpu_mask(cpu, mask) {
+			struct list_head *tmp;
+
+			if (!cpu_possible(cpu))
+				continue;
   

I guess you could just do cpus_and(mask, cpus_possible_map) on entry.
 

Yup !

quoted

+			if (cpu == mycpu)
+				preempt_disable();
+
+			sem = &per_cpu(listener_list_sem, cpu);
+			head = &per_cpu(listener_list, cpu);
+
+			down_write(sem);
+			list_for_each_safe(p, tmp, head) {
+				s = list_entry(p, struct listener, list);
+				if (s->pid == pid) {
+					list_del(&s->list);
   

kfree(s);
 

Oops.

quoted

+					break;
+				}
+			}
+			up_write(sem);
+
+			if (cpu == mycpu)
+				preempt_enable();
+		}
+	}
+	return 0;
+}
+
+static int taskstats_user_cmd(struct sk_buff *skb, struct genl_info *info)
{
	int rc = 0;
	struct sk_buff *rep_skb;

@@ -210,6 +313,21 @@ static int taskstats_send_stats(struct s

	void *reply;
	size_t size;
	struct nlattr *na;
+	cpumask_t mask;
+
+	if (info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK]) {
+		na = info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK];
+		cpulist_parse((char *)nla_data(na), mask);
   

OK, so we're passing in an ASCII string.  Fair enough, I think.  Paul would
know better.