In perf_cgroup_connect(), fput_light() is missing in a failure
path.
Signed-off-by: Li Zefan <[email protected]>
---
kernel/perf_event.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 64a018e..4a955fd 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -404,8 +404,10 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
return -EBADF;
css = cgroup_css_from_dir(file, perf_subsys_id);
- if (IS_ERR(css))
- return PTR_ERR(css);
+ if (IS_ERR(css)) {
+ ret = PTR_ERR(css);
+ goto out;
+ }
cgrp = container_of(css, struct perf_cgroup, css);
event->cgrp = cgrp;
@@ -422,6 +424,7 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
/* must be done before we fput() the file */
perf_get_cgroup(event);
}
+out:
fput_light(file, fput_needed);
return ret;
}
--
1.6.3
In the failure path, we call perf_detach_cgroup(), but we didn't
call perf_get_cgroup() prio to it.
Signed-off-by: Li Zefan <[email protected]>
---
kernel/perf_event.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 4a955fd..dca92b2 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -412,6 +412,9 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
cgrp = container_of(css, struct perf_cgroup, css);
event->cgrp = cgrp;
+ /* must be done before we fput() the file */
+ perf_get_cgroup(event);
+
/*
* all events in a group must monitor
* the same cgroup because a task belongs
@@ -420,9 +423,6 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
if (group_leader && group_leader->cgrp != cgrp) {
perf_detach_cgroup(event);
ret = -EINVAL;
- } else {
- /* must be done before we fput() the file */
- perf_get_cgroup(event);
}
out:
fput_light(file, fput_needed);
--
1.6.3
- Use kzalloc() to replace kmalloc() + memset().
- Remove redundant initialization, since alloc_percpu() returns
zero-filled percpu memory.
Signed-off-by: Li Zefan <[email protected]>
---
kernel/perf_event.c | 11 +----------
1 files changed, 1 insertions(+), 10 deletions(-)
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index dca92b2..d6b3d16 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -7341,26 +7341,17 @@ static struct cgroup_subsys_state *perf_cgroup_create(
struct cgroup_subsys *ss, struct cgroup *cont)
{
struct perf_cgroup *jc;
- struct perf_cgroup_info *t;
- int c;
- jc = kmalloc(sizeof(*jc), GFP_KERNEL);
+ jc = kzalloc(sizeof(*jc), GFP_KERNEL);
if (!jc)
return ERR_PTR(-ENOMEM);
- memset(jc, 0, sizeof(*jc));
-
jc->info = alloc_percpu(struct perf_cgroup_info);
if (!jc->info) {
kfree(jc);
return ERR_PTR(-ENOMEM);
}
- for_each_possible_cpu(c) {
- t = per_cpu_ptr(jc->info, c);
- t->time = 0;
- t->timestamp = 0;
- }
return &jc->css;
}
--
1.6.3
s/specificied/specified
Signed-off-by: Li Zefan <[email protected]>
---
init/Kconfig | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/init/Kconfig b/init/Kconfig
index 20d6bd9..4c4edf2 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -688,7 +688,7 @@ config CGROUP_PERF
depends on PERF_EVENTS && CGROUPS
help
This option extends the per-cpu mode to restrict monitoring to
- threads which belong to the cgroup specificied and run on the
+ threads which belong to the cgroup specified and run on the
designated cpu.
Say N if unsure.
--
1.6.3
On Thu, Mar 3, 2011 at 7:25 AM, Li Zefan <[email protected]> wrote:
> In perf_cgroup_connect(), fput_light() is missing in a failure
> path.
>
Yes, you're right.
> Signed-off-by: Li Zefan <[email protected]>
> ---
> kernel/perf_event.c | 7 +++++--
> 1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index 64a018e..4a955fd 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -404,8 +404,10 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
> return -EBADF;
>
> css = cgroup_css_from_dir(file, perf_subsys_id);
> - if (IS_ERR(css))
> - return PTR_ERR(css);
> + if (IS_ERR(css)) {
> + ret = PTR_ERR(css);
> + goto out;
> + }
>
> cgrp = container_of(css, struct perf_cgroup, css);
> event->cgrp = cgrp;
> @@ -422,6 +424,7 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
> /* must be done before we fput() the file */
> perf_get_cgroup(event);
> }
> +out:
> fput_light(file, fput_needed);
> return ret;
> }
> --
> 1.6.3
>
On Thu, Mar 3, 2011 at 7:25 AM, Li Zefan <[email protected]> wrote:
> In the failure path, we call perf_detach_cgroup(), but we didn't
> call perf_get_cgroup() prio to it.
>
the funny thing is that I had this change at some point. But apparently
it got lost in the many revisions of the patch. Thanks for fixing it.
Acked-by: Stephane Eranian <[email protected]>
> Signed-off-by: Li Zefan <[email protected]>
> ---
> kernel/perf_event.c | 6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index 4a955fd..dca92b2 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -412,6 +412,9 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
> cgrp = container_of(css, struct perf_cgroup, css);
> event->cgrp = cgrp;
>
> + /* must be done before we fput() the file */
> + perf_get_cgroup(event);
> +
> /*
> * all events in a group must monitor
> * the same cgroup because a task belongs
> @@ -420,9 +423,6 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
> if (group_leader && group_leader->cgrp != cgrp) {
> perf_detach_cgroup(event);
> ret = -EINVAL;
> - } else {
> - /* must be done before we fput() the file */
> - perf_get_cgroup(event);
> }
> out:
> fput_light(file, fput_needed);
> --
> 1.6.3
>
On Thu, Mar 3, 2011 at 7:26 AM, Li Zefan <[email protected]> wrote:
> - Use kzalloc() to replace kmalloc() + memset().
> - Remove redundant initialization, since alloc_percpu() returns
> zero-filled percpu memory.
>
Acked-by: Stephane Eranian <[email protected]>
> Signed-off-by: Li Zefan <[email protected]>
> ---
> kernel/perf_event.c | 11 +----------
> 1 files changed, 1 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index dca92b2..d6b3d16 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -7341,26 +7341,17 @@ static struct cgroup_subsys_state *perf_cgroup_create(
> struct cgroup_subsys *ss, struct cgroup *cont)
> {
> struct perf_cgroup *jc;
> - struct perf_cgroup_info *t;
> - int c;
>
> - jc = kmalloc(sizeof(*jc), GFP_KERNEL);
> + jc = kzalloc(sizeof(*jc), GFP_KERNEL);
> if (!jc)
> return ERR_PTR(-ENOMEM);
>
> - memset(jc, 0, sizeof(*jc));
> -
> jc->info = alloc_percpu(struct perf_cgroup_info);
> if (!jc->info) {
> kfree(jc);
> return ERR_PTR(-ENOMEM);
> }
>
> - for_each_possible_cpu(c) {
> - t = per_cpu_ptr(jc->info, c);
> - t->time = 0;
> - t->timestamp = 0;
> - }
> return &jc->css;
> }
>
> --
> 1.6.3
>
Li,
I have to NACK this patch or at least request more information.
The for_each_possible_cpu() is initializing
what's allocated via alloc_percpu(), jc->info and NOT jc.
I don't think this gets zeroed by this allocator. But I could
be wrong.
On Thu, Mar 3, 2011 at 7:26 AM, Li Zefan <[email protected]> wrote:
> - Use kzalloc() to replace kmalloc() + memset().
> - Remove redundant initialization, since alloc_percpu() returns
> zero-filled percpu memory.
>
> Signed-off-by: Li Zefan <[email protected]>
> ---
> kernel/perf_event.c | 11 +----------
> 1 files changed, 1 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index dca92b2..d6b3d16 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -7341,26 +7341,17 @@ static struct cgroup_subsys_state *perf_cgroup_create(
> struct cgroup_subsys *ss, struct cgroup *cont)
> {
> struct perf_cgroup *jc;
> - struct perf_cgroup_info *t;
> - int c;
>
> - jc = kmalloc(sizeof(*jc), GFP_KERNEL);
> + jc = kzalloc(sizeof(*jc), GFP_KERNEL);
> if (!jc)
> return ERR_PTR(-ENOMEM);
>
> - memset(jc, 0, sizeof(*jc));
> -
> jc->info = alloc_percpu(struct perf_cgroup_info);
> if (!jc->info) {
> kfree(jc);
> return ERR_PTR(-ENOMEM);
> }
>
> - for_each_possible_cpu(c) {
> - t = per_cpu_ptr(jc->info, c);
> - t->time = 0;
> - t->timestamp = 0;
> - }
> return &jc->css;
> }
>
> --
> 1.6.3
>
Ok,
I checked and yes, alloc_percpu() does return zeroed out memory.
So the patch looks good to me.
Thanks.
On Thu, Mar 3, 2011 at 9:37 AM, Stephane Eranian <[email protected]> wrote:
> Li,
>
> I have to NACK this patch or at least request more information.
>
> The for_each_possible_cpu() is initializing
> what's allocated via alloc_percpu(), jc->info and NOT jc.
>
> I don't think this gets zeroed by this allocator. But I could
> be wrong.
>
>
> On Thu, Mar 3, 2011 at 7:26 AM, Li Zefan <[email protected]> wrote:
>> - Use kzalloc() to replace kmalloc() + memset().
>> - Remove redundant initialization, since alloc_percpu() returns
>> zero-filled percpu memory.
>>
>> Signed-off-by: Li Zefan <[email protected]>
>> ---
>> kernel/perf_event.c | 11 +----------
>> 1 files changed, 1 insertions(+), 10 deletions(-)
>>
>> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
>> index dca92b2..d6b3d16 100644
>> --- a/kernel/perf_event.c
>> +++ b/kernel/perf_event.c
>> @@ -7341,26 +7341,17 @@ static struct cgroup_subsys_state *perf_cgroup_create(
>> struct cgroup_subsys *ss, struct cgroup *cont)
>> {
>> struct perf_cgroup *jc;
>> - struct perf_cgroup_info *t;
>> - int c;
>>
>> - jc = kmalloc(sizeof(*jc), GFP_KERNEL);
>> + jc = kzalloc(sizeof(*jc), GFP_KERNEL);
>> if (!jc)
>> return ERR_PTR(-ENOMEM);
>>
>> - memset(jc, 0, sizeof(*jc));
>> -
>> jc->info = alloc_percpu(struct perf_cgroup_info);
>> if (!jc->info) {
>> kfree(jc);
>> return ERR_PTR(-ENOMEM);
>> }
>>
>> - for_each_possible_cpu(c) {
>> - t = per_cpu_ptr(jc->info, c);
>> - t->time = 0;
>> - t->timestamp = 0;
>> - }
>> return &jc->css;
>> }
>>
>> --
>> 1.6.3
>>
>
Commit-ID: 3db272c0494900fcb905a201180a78cae3addd6e
Gitweb: http://git.kernel.org/tip/3db272c0494900fcb905a201180a78cae3addd6e
Author: Li Zefan <[email protected]>
AuthorDate: Thu, 3 Mar 2011 14:25:37 +0800
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 4 Mar 2011 11:32:50 +0100
perf cgroup: Fix leak of file reference count
In perf_cgroup_connect(), fput_light() is missing in a failure path.
Signed-off-by: Li Zefan <[email protected]>
Acked-by: Stephane Eranian <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
kernel/perf_event.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 821ce82..7c999e8 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -404,8 +404,10 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
return -EBADF;
css = cgroup_css_from_dir(file, perf_subsys_id);
- if (IS_ERR(css))
- return PTR_ERR(css);
+ if (IS_ERR(css)) {
+ ret = PTR_ERR(css);
+ goto out;
+ }
cgrp = container_of(css, struct perf_cgroup, css);
event->cgrp = cgrp;
@@ -422,6 +424,7 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
/* must be done before we fput() the file */
perf_get_cgroup(event);
}
+out:
fput_light(file, fput_needed);
return ret;
}
Commit-ID: f75e18cb9627b1d3d752b83a0b5563da0042c50a
Gitweb: http://git.kernel.org/tip/f75e18cb9627b1d3d752b83a0b5563da0042c50a
Author: Li Zefan <[email protected]>
AuthorDate: Thu, 3 Mar 2011 14:25:50 +0800
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 4 Mar 2011 11:32:51 +0100
perf cgroup: Fix unmatched call to perf_detach_cgroup()
In the failure path, we call perf_detach_cgroup(), but we didn't
call perf_get_cgroup() prio to it.
Signed-off-by: Li Zefan <[email protected]>
Acked-by: Stephane Eranian <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
kernel/perf_event.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 7c999e8..b002095 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -412,6 +412,9 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
cgrp = container_of(css, struct perf_cgroup, css);
event->cgrp = cgrp;
+ /* must be done before we fput() the file */
+ perf_get_cgroup(event);
+
/*
* all events in a group must monitor
* the same cgroup because a task belongs
@@ -420,9 +423,6 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
if (group_leader && group_leader->cgrp != cgrp) {
perf_detach_cgroup(event);
ret = -EINVAL;
- } else {
- /* must be done before we fput() the file */
- perf_get_cgroup(event);
}
out:
fput_light(file, fput_needed);
Commit-ID: 1b15d0558e82df9b3659804ceb44187b98eda354
Gitweb: http://git.kernel.org/tip/1b15d0558e82df9b3659804ceb44187b98eda354
Author: Li Zefan <[email protected]>
AuthorDate: Thu, 3 Mar 2011 14:26:06 +0800
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 4 Mar 2011 11:32:51 +0100
perf cgroup: Clean up perf_cgroup_create()
- Use kzalloc() to replace kmalloc() + memset().
- Remove redundant initialization, since alloc_percpu() returns
zero-filled percpu memory.
Signed-off-by: Li Zefan <[email protected]>
Acked-by: Stephane Eranian <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
kernel/perf_event.c | 11 +----------
1 files changed, 1 insertions(+), 10 deletions(-)
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index b002095..193b190 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -7346,26 +7346,17 @@ static struct cgroup_subsys_state *perf_cgroup_create(
struct cgroup_subsys *ss, struct cgroup *cont)
{
struct perf_cgroup *jc;
- struct perf_cgroup_info *t;
- int c;
- jc = kmalloc(sizeof(*jc), GFP_KERNEL);
+ jc = kzalloc(sizeof(*jc), GFP_KERNEL);
if (!jc)
return ERR_PTR(-ENOMEM);
- memset(jc, 0, sizeof(*jc));
-
jc->info = alloc_percpu(struct perf_cgroup_info);
if (!jc->info) {
kfree(jc);
return ERR_PTR(-ENOMEM);
}
- for_each_possible_cpu(c) {
- t = per_cpu_ptr(jc->info, c);
- t->time = 0;
- t->timestamp = 0;
- }
return &jc->css;
}
Commit-ID: 2d0f25201ee210a0666ec9c41538ba05a07f8bc6
Gitweb: http://git.kernel.org/tip/2d0f25201ee210a0666ec9c41538ba05a07f8bc6
Author: Li Zefan <[email protected]>
AuthorDate: Thu, 3 Mar 2011 14:26:20 +0800
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 4 Mar 2011 11:32:51 +0100
perf cgroup: Fix a typo in kernel config
s/specificied/specified
Signed-off-by: Li Zefan <[email protected]>
Acked-by: Stephane Eranian <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
init/Kconfig | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/init/Kconfig b/init/Kconfig
index 20d6bd9..4c4edf2 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -688,7 +688,7 @@ config CGROUP_PERF
depends on PERF_EVENTS && CGROUPS
help
This option extends the per-cpu mode to restrict monitoring to
- threads which belong to the cgroup specificied and run on the
+ threads which belong to the cgroup specified and run on the
designated cpu.
Say N if unsure.