Messages in this thread | | | Date | Thu, 6 Jul 2006 02:56:33 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH] per-task delay accounting taskstats interface: control exit data through cpumasks] |
| |
On Thu, 06 Jul 2006 05:28:35 -0400 Shailabh Nagar <nagar@watson.ibm.com> wrote:
> On systems with a large number of cpus, with even a modest rate of > tasks exiting per cpu, the volume of taskstats data sent on thread exit > can overflow a userspace listener's buffers. > > One approach to avoiding overflow is to allow listeners to get data for > a limited and specific set of cpus. By scaling the number of listeners > and/or the cpus they monitor, userspace can handle the statistical data > overload more gracefully. > > In this patch, each listener registers to listen to a specific set of > cpus by specifying a cpumask. The interest is recorded per-cpu. When > a task exits on a cpu, its taskstats data is unicast to each listener > interested in that cpu. > > Thanks to Andrew Morton for pointing out the various scalability and > general concerns of previous attempts and for suggesting this design. > > ... > > --- linux-2.6.17-mm3equiv.orig/include/linux/taskstats.h 2006-06-30 19:03:40.000000000 -0400 > +++ linux-2.6.17-mm3equiv/include/linux/taskstats.h 2006-07-06 02:38:28.000000000 -0400
Your email client performs space-stuffing. Fortunately "sed -e 's/^ / /'" is easy.
> #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>
Like that.
> > +static int add_del_listener(pid_t pid, cpumask_t *maskp, int isadd) > +{ > + struct listener *s; > + struct listener_list *listeners; > + unsigned int cpu; > + cpumask_t mask; > + struct list_head *p; > + > + memcpy(&mask, maskp, sizeof(cpumask_t));
mask = *maskp; ?
> + if (!cpus_subset(mask, cpu_possible_map)) > + return -EINVAL; > + > + if (isadd == REGISTER) { > + for_each_cpu_mask(cpu, mask) { > + s = kmalloc_node(sizeof(struct listener), GFP_KERNEL, > + cpu_to_node(cpu)); > + if (!s) > + return -ENOMEM; > + s->pid = pid; > + INIT_LIST_HEAD(&s->list); > + > + listeners = &per_cpu(listener_array, cpu); > + down_write(&listeners->sem); > + list_add(&s->list, &listeners->list); > + up_write(&listeners->sem); > + } > + } else { > + for_each_cpu_mask(cpu, mask) { > + struct list_head *tmp; > + > + listeners = &per_cpu(listener_array, cpu); > + down_write(&listeners->sem); > + list_for_each_safe(p, tmp, &listeners->list) { > + s = list_entry(p, struct listener, list); > + if (s->pid == pid) { > + list_del(&s->list); > + kfree(s); > + break; > + } > + } > + up_write(&listeners->sem); > + } > + } > + return 0; > +}
You might choose to handle the ENOMEM situation here by backing out and not leaving things half-installed. I suspect that's just a simple `goto'.
> -static int taskstats_send_stats(struct sk_buff *skb, struct genl_info *info) > +static int taskstats_user_cmd(struct sk_buff *skb, struct genl_info *info) > { > int rc = 0; > struct sk_buff *rep_skb; > @@ -210,6 +302,25 @@ static int taskstats_send_stats(struct s > void *reply; > size_t size; > struct nlattr *na; > + cpumask_t mask;
When counting add_del_listener(), that's two cpumasks on the stack. How big can these get? 256 bytes? Is it possible to get by with just the one?
> + > + if (info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK]) { > + na = info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK]; > + if (nla_len(na) > TASKSTATS_CPUMASK_MAXLEN) > + return -E2BIG; > + cpulist_parse((char *)nla_data(na), mask);
Best check the return value from this function.
> + rc = add_del_listener(info->snd_pid, &mask, REGISTER); > + return rc; > + } > + > + if (info->attrs[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK]) { > + na = info->attrs[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK]; > + if (nla_len(na) > TASKSTATS_CPUMASK_MAXLEN) > + return -E2BIG; > + cpulist_parse((char *)nla_data(na), mask); > + rc = add_del_listener(info->snd_pid, &mask, DEREGISTER); > + return rc; > + } > > ... > > +void taskstats_exit_alloc(struct taskstats **ptidstats, unsigned int *mycpu) > +{ > + struct listener_list *listeners; > + struct taskstats *tmp; > + /* > + * This is the cpu on which the task is exiting currently and will > + * be the one for which the exit event is sent, even if the cpu > + * on which this function is running changes later. > + */ > + *mycpu = raw_smp_processor_id(); > + > + *ptidstats = NULL; > + tmp = kmem_cache_zalloc(taskstats_cache, SLAB_KERNEL); > + if (!tmp) > + return; > + > + listeners = &per_cpu(listener_array, *mycpu); > + down_read(&listeners->sem); > + if (!list_empty(&listeners->list)) { > + *ptidstats = tmp; > + tmp = NULL; > + } > + up_read(&listeners->sem); > + if (tmp) > + kfree(tmp);
kfree(NULL) is legal.
Looks good to me. Does it work? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |