2014-04-26 17:18:16

by Richard Yao

Subject: [PATCH] perf machine: Search for modules in %s/lib/modules/%s

Modules installed outside of the kernel's build system should go into
"%s/lib/modules/%s/extra", but at present, perf will only look at them
when they are in "%s/lib/modules/%s/kernel". Lets encourage good
citizenship by relaxing this requirement to "%s/lib/modules/%s". This
way open source modules that are out-of-tree have no incentive to start
populating a directory reserved for in-kernel modules and I can stop
hex-editing my system's perf binary when profiling OSS out-of-tree
modules.

Feedback from Namhyung Kim correctly revealed that the hex-edits that I
had been doing meant that perf was also traversing the build and source
symlinks in %s/lib/modules/%s. That is undesireable, so we explicitly
exclude them from traversal with a minor tweak to the traversal routine.

Signed-off-by: Richard Yao <[email protected]>
---
tools/perf/util/machine.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index a53cd0b..27c2a5e 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -717,7 +717,7 @@ static char *get_kernel_version(const char *root_dir)
}

static int map_groups__set_modules_path_dir(struct map_groups *mg,
- const char *dir_name)
+ const char *dir_name, int depth)
{
struct dirent *dent;
DIR *dir = opendir(dir_name);
@@ -742,7 +742,15 @@ static int map_groups__set_modules_path_dir(struct map_groups *mg,
!strcmp(dent->d_name, ".."))
continue;

- ret = map_groups__set_modules_path_dir(mg, path);
+ /* Do not follow top-level source and build symlinks */
+ if (depth == 0) {
+ if (!strcmp(dent->d_name, "source") ||
+ !strcmp(dent->d_name, "build"))
+ continue;
+ }
+
+ ret = map_groups__set_modules_path_dir(mg, path,
+ depth + 1);
if (ret < 0)
goto out;
} else {
@@ -786,11 +794,11 @@ static int machine__set_modules_path(struct machine *machine)
if (!version)
return -1;

- snprintf(modules_path, sizeof(modules_path), "%s/lib/modules/%s/kernel",
+ snprintf(modules_path, sizeof(modules_path), "%s/lib/modules/%s",
machine->root_dir, version);
free(version);

- return map_groups__set_modules_path_dir(&machine->kmaps, modules_path);
+ return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0);
}

static int machine__create_module(void *arg, const char *name, u64 start)
--
1.8.3.2

2014-04-27 10:06:13

by Jiri Olsa

Subject: Re: [PATCH] perf machine: Search for modules in %s/lib/modules/%s

On Sat, Apr 26, 2014 at 01:17:55PM -0400, Richard Yao wrote:
> Modules installed outside of the kernel's build system should go into
> "%s/lib/modules/%s/extra", but at present, perf will only look at them
> when they are in "%s/lib/modules/%s/kernel". Lets encourage good
> citizenship by relaxing this requirement to "%s/lib/modules/%s". This
> way open source modules that are out-of-tree have no incentive to start
> populating a directory reserved for in-kernel modules and I can stop
> hex-editing my system's perf binary when profiling OSS out-of-tree
> modules.
>
> Feedback from Namhyung Kim correctly revealed that the hex-edits that I
> had been doing meant that perf was also traversing the build and source
> symlinks in %s/lib/modules/%s. That is undesireable, so we explicitly
> exclude them from traversal with a minor tweak to the traversal routine.

looks good to me, Namhyung?

thanks,
jirka

>
> Signed-off-by: Richard Yao <[email protected]>
> ---
> tools/perf/util/machine.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index a53cd0b..27c2a5e 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -717,7 +717,7 @@ static char *get_kernel_version(const char *root_dir)
> }
>
> static int map_groups__set_modules_path_dir(struct map_groups *mg,
> - const char *dir_name)
> + const char *dir_name, int depth)
> {
> struct dirent *dent;
> DIR *dir = opendir(dir_name);
> @@ -742,7 +742,15 @@ static int map_groups__set_modules_path_dir(struct map_groups *mg,
> !strcmp(dent->d_name, ".."))
> continue;
>
> - ret = map_groups__set_modules_path_dir(mg, path);
> + /* Do not follow top-level source and build symlinks */
> + if (depth == 0) {
> + if (!strcmp(dent->d_name, "source") ||
> + !strcmp(dent->d_name, "build"))
> + continue;
> + }
> +
> + ret = map_groups__set_modules_path_dir(mg, path,
> + depth + 1);
> if (ret < 0)
> goto out;
> } else {
> @@ -786,11 +794,11 @@ static int machine__set_modules_path(struct machine *machine)
> if (!version)
> return -1;
>
> - snprintf(modules_path, sizeof(modules_path), "%s/lib/modules/%s/kernel",
> + snprintf(modules_path, sizeof(modules_path), "%s/lib/modules/%s",
> machine->root_dir, version);
> free(version);
>
> - return map_groups__set_modules_path_dir(&machine->kmaps, modules_path);
> + return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0);
> }
>
> static int machine__create_module(void *arg, const char *name, u64 start)
> --
> 1.8.3.2
>

2014-04-29 08:06:41

by Jiri Olsa

Subject: Re: [PATCH] perf machine: Search for modules in %s/lib/modules/%s

On Sun, Apr 27, 2014 at 12:05:56PM +0200, Jiri Olsa wrote:
> On Sat, Apr 26, 2014 at 01:17:55PM -0400, Richard Yao wrote:
> > Modules installed outside of the kernel's build system should go into
> > "%s/lib/modules/%s/extra", but at present, perf will only look at them
> > when they are in "%s/lib/modules/%s/kernel". Lets encourage good
> > citizenship by relaxing this requirement to "%s/lib/modules/%s". This
> > way open source modules that are out-of-tree have no incentive to start
> > populating a directory reserved for in-kernel modules and I can stop
> > hex-editing my system's perf binary when profiling OSS out-of-tree
> > modules.
> >
> > Feedback from Namhyung Kim correctly revealed that the hex-edits that I
> > had been doing meant that perf was also traversing the build and source
> > symlinks in %s/lib/modules/%s. That is undesireable, so we explicitly
> > exclude them from traversal with a minor tweak to the traversal routine.
>
> looks good to me, Namhyung?

just realized Namhyung wasn't CC-ed on this one ;-)

jirka

>
> thanks,
> jirka
>
> >
> > Signed-off-by: Richard Yao <[email protected]>
> > ---
> > tools/perf/util/machine.c | 16 ++++++++++++----
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index a53cd0b..27c2a5e 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -717,7 +717,7 @@ static char *get_kernel_version(const char *root_dir)
> > }
> >
> > static int map_groups__set_modules_path_dir(struct map_groups *mg,
> > - const char *dir_name)
> > + const char *dir_name, int depth)
> > {
> > struct dirent *dent;
> > DIR *dir = opendir(dir_name);
> > @@ -742,7 +742,15 @@ static int map_groups__set_modules_path_dir(struct map_groups *mg,
> > !strcmp(dent->d_name, ".."))
> > continue;
> >
> > - ret = map_groups__set_modules_path_dir(mg, path);
> > + /* Do not follow top-level source and build symlinks */
> > + if (depth == 0) {
> > + if (!strcmp(dent->d_name, "source") ||
> > + !strcmp(dent->d_name, "build"))
> > + continue;
> > + }
> > +
> > + ret = map_groups__set_modules_path_dir(mg, path,
> > + depth + 1);
> > if (ret < 0)
> > goto out;
> > } else {
> > @@ -786,11 +794,11 @@ static int machine__set_modules_path(struct machine *machine)
> > if (!version)
> > return -1;
> >
> > - snprintf(modules_path, sizeof(modules_path), "%s/lib/modules/%s/kernel",
> > + snprintf(modules_path, sizeof(modules_path), "%s/lib/modules/%s",
> > machine->root_dir, version);
> > free(version);
> >
> > - return map_groups__set_modules_path_dir(&machine->kmaps, modules_path);
> > + return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0);
> > }
> >
> > static int machine__create_module(void *arg, const char *name, u64 start)
> > --
> > 1.8.3.2
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2014-04-29 23:46:43

by Namhyung Kim

Subject: Re: [PATCH] perf machine: Search for modules in %s/lib/modules/%s

Hi Jiri,

On Tue, 29 Apr 2014 10:06:17 +0200, Jiri Olsa wrote:
> On Sun, Apr 27, 2014 at 12:05:56PM +0200, Jiri Olsa wrote:
>> On Sat, Apr 26, 2014 at 01:17:55PM -0400, Richard Yao wrote:
>> > Modules installed outside of the kernel's build system should go into
>> > "%s/lib/modules/%s/extra", but at present, perf will only look at them
>> > when they are in "%s/lib/modules/%s/kernel". Lets encourage good
>> > citizenship by relaxing this requirement to "%s/lib/modules/%s". This
>> > way open source modules that are out-of-tree have no incentive to start
>> > populating a directory reserved for in-kernel modules and I can stop
>> > hex-editing my system's perf binary when profiling OSS out-of-tree
>> > modules.
>> >
>> > Feedback from Namhyung Kim correctly revealed that the hex-edits that I
>> > had been doing meant that perf was also traversing the build and source
>> > symlinks in %s/lib/modules/%s. That is undesireable, so we explicitly
>> > exclude them from traversal with a minor tweak to the traversal routine.
>>
>> looks good to me, Namhyung?
>
> just realized Namhyung wasn't CC-ed on this one ;-)

:)

Well, I still prefer the symlink check with lstat(), I can live with
this, so:

Acked-by: Namhyung kim <[email protected]>

Thanks,
Namhyung

>
>>
>> thanks,
>> jirka
>>
>> >
>> > Signed-off-by: Richard Yao <[email protected]>
>> > ---
>> > tools/perf/util/machine.c | 16 ++++++++++++----
>> > 1 file changed, 12 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
>> > index a53cd0b..27c2a5e 100644
>> > --- a/tools/perf/util/machine.c
>> > +++ b/tools/perf/util/machine.c
>> > @@ -717,7 +717,7 @@ static char *get_kernel_version(const char *root_dir)
>> > }
>> >
>> > static int map_groups__set_modules_path_dir(struct map_groups *mg,
>> > - const char *dir_name)
>> > + const char *dir_name, int depth)
>> > {
>> > struct dirent *dent;
>> > DIR *dir = opendir(dir_name);
>> > @@ -742,7 +742,15 @@ static int map_groups__set_modules_path_dir(struct map_groups *mg,
>> > !strcmp(dent->d_name, ".."))
>> > continue;
>> >
>> > - ret = map_groups__set_modules_path_dir(mg, path);
>> > + /* Do not follow top-level source and build symlinks */
>> > + if (depth == 0) {
>> > + if (!strcmp(dent->d_name, "source") ||
>> > + !strcmp(dent->d_name, "build"))
>> > + continue;
>> > + }
>> > +
>> > + ret = map_groups__set_modules_path_dir(mg, path,
>> > + depth + 1);
>> > if (ret < 0)
>> > goto out;
>> > } else {
>> > @@ -786,11 +794,11 @@ static int machine__set_modules_path(struct machine *machine)
>> > if (!version)
>> > return -1;
>> >
>> > - snprintf(modules_path, sizeof(modules_path), "%s/lib/modules/%s/kernel",
>> > + snprintf(modules_path, sizeof(modules_path), "%s/lib/modules/%s",
>> > machine->root_dir, version);
>> > free(version);
>> >
>> > - return map_groups__set_modules_path_dir(&machine->kmaps, modules_path);
>> > + return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0);
>> > }
>> >
>> > static int machine__create_module(void *arg, const char *name, u64 start)
>> > --
>> > 1.8.3.2
>> >
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/