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

Shailabh Nagar
2006-07-04T00:13:44Z

Andrew Morton wrote:

>On Mon, 03 Jul 2006 17:11:59 -0400
>Shailabh Nagar <nagar@watson.ibm.com> wrote:
>
>
>>
>> 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..
>
>
>
>> 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);
>
>
>
>
>> 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.
>
>
>
>>+ 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.

>
>
>>+ 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.

>
>
>>+ 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 ?

>
>
>
>>+ }
>>+ } 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 !

>
>
>
>>+ 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.

>
>
>>+ 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.
>
>