Messages in this thread | | | Date | Wed, 20 Jul 2011 23:50:48 +0400 | From | Vasiliy Kulikov <> | Subject | Re: [PATCH 1/2] taskstats: add_del_listener() shouldn't use the wrong node |
| |
On Wed, Jul 20, 2011 at 21:00 +0200, Oleg Nesterov wrote: > 1. 26c4caea "don't allow duplicate entries in listener mode" > changed add_del_listener(REGISTER) so that "next_cpu:" can > reuse the listener allocated for the previous cpu, this > doesn't look exactly right even if minor. > > Change the code to kfree() in the already-registered case, > this case is unlikely anyway so the extra kmalloc_node() > shouldn't hurt but looke more correct and clean. > > 2. use the plain list_for_each_entry() instead of _safe() to > scan listeners->list. > > 3. Remove the unneeded INIT_LIST_HEAD(&s->list), we are going > to list_add(&s->list).
I don't know whether (1) is needed, looks very minor to me. Other than that:
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Vasiliy Kulikov <segoon@openwall.com>
> --- > > kernel/taskstats.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > --- ts/kernel/taskstats.c~1_cleanup 2011-07-03 16:27:57.000000000 +0200 > +++ ts/kernel/taskstats.c 2011-07-20 19:35:38.000000000 +0200 > @@ -291,30 +291,28 @@ static int add_del_listener(pid_t pid, c > if (!cpumask_subset(mask, cpu_possible_mask)) > return -EINVAL; > > - s = NULL; > if (isadd == REGISTER) { > for_each_cpu(cpu, mask) { > - if (!s) > - s = kmalloc_node(sizeof(struct listener), > - GFP_KERNEL, cpu_to_node(cpu)); > + s = kmalloc_node(sizeof(struct listener), > + GFP_KERNEL, cpu_to_node(cpu)); > if (!s) > goto cleanup; > + > s->pid = pid; > - INIT_LIST_HEAD(&s->list); > s->valid = 1; > > listeners = &per_cpu(listener_array, cpu); > down_write(&listeners->sem); > - list_for_each_entry_safe(s2, tmp, &listeners->list, list) { > + list_for_each_entry(s2, &listeners->list, list) { > if (s2->pid == pid) > - goto next_cpu; > + goto exists; > } > list_add(&s->list, &listeners->list); > s = NULL; > -next_cpu: > +exists: > up_write(&listeners->sem); > + kfree(s); /* nop if NULL */ > } > - kfree(s); > return 0; > } > >
-- Vasiliy Kulikov http://www.openwall.com - bringing security into open computing environments
| |