Re: [PATCH 3/4] perf tools: report: introduce --map-adjustment argument.

Jiri Olsa
2015-04-01T13:21:47Z

On Wed, Apr 01, 2015 at 10:33:14AM +0000, Wang Nan wrote:
> This patch introduces a --map-adjustment argument for perf report. The
> goal of this option is to deal with private dynamic loader used in some
> special program.
>

SNIP

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 051883a..dc9e91e 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1155,21 +1155,291 @@ out_problem:
> return -1;
> }
>
> +/*
> + * Users are allowed to provide map adjustment setting for the case
> + * that an address range is actually privatly mapped but known to be
> + * ELF object file backended. Like this:
> + *
> + * |<- copied from libx.so ->| |<- copied from liby.so ->|
> + * |<-------------------- MMAP area --------------------->|
> + *
> + * When dealing with such mmap events, try to obey user adjustment.
> + * Such adjustment settings are not allowed overlapping.
> + * Adjustments won't be considered as valid code until real MMAP events
> + * take place. Therefore, users are allowed to provide adjustments which
> + * cover never mapped areas, like:
> + *
> + * |<- libx.so ->| |<- liby.so ->|
> + * |<-- MMAP area -->|
> + *
> + * This feature is useful when dealing with private dynamic linkers,
> + * which assemble code piece from different ELF objects.
> + *
> + * map_adj_list is an ordered linked list. Order of two adjustments is
> + * first defined by their pid, and then by their start address.
> + * Therefore, adjustments for specific pids are groupped together
> + * naturally.
> + */
> +static LIST_HEAD(map_adj_list);

we dont like global stuff ;-)

I think this belongs to the machine object, which is created
within the perf_session__new, so after options parsing.. hum

perhaps you could stash stash 'struct map_adj' objects and
add some interface to init perf_session::machines::host
once it's created?

> +struct map_adj {

IMHO 'struct map_adjust' suits better.. using 'adjust' instead
of 'adj' is not such a waste of space and it's more readable
(for all 'adj' instances in the patch)

> + u32 pid;
> + u64 start;
> + u64 len;
> + u64 pgoff;
> + struct list_head list;
> + char filename[PATH_MAX];
> +};
> +
> +enum map_adj_cross {

'enum map_adjust' ?

> + MAP_ADJ_LEFT_PID,
> + MAP_ADJ_LEFT,
> + MAP_ADJ_CROSS,
> + MAP_ADJ_RIGHT,
> + MAP_ADJ_RIGHT_PID,
> +};
> +
> +/*
> + * Check whether two map_adj cross over each other. This function is
> + * used for comparing adjustments. For overlapping adjustments, it
> + * reports different between two start address and the length of
> + * overlapping area. Signess of pgoff_diff can be used to determine
> + * which one is the left one.
> + *
> + * If anyone in r and l has pid set as -1, don't consider pid.
> + */

SNIP

> static int machine_map_new(struct machine *machine, u64 start, u64 len,
> u64 pgoff, u32 pid, u32 d_maj, u32 d_min, u64 ino,
> u64 ino_gen, u32 prot, u32 flags, char *filename,
> enum map_type type, struct thread *thread)
> {
> + struct map_adj *pos;
> struct map *map;
>
> - map = map__new(machine, start, len, pgoff, pid, d_maj, d_min,
> - ino, ino_gen, prot, flags, filename, type, thread);

could you please loop below into separate function?

> + list_for_each_entry(pos, &map_adj_list, list) {
> + u64 adj_start, adj_len, adj_pgoff, cross_len;
> + enum map_adj_cross cross;
> + struct map_adj tmp;
> + int pgoff_diff;

just curious.. how many --map-adjust entries do you normaly use?
maybe if it's bigger number then a) using rb_tree might be faster
and b) using some sort of config file could be better way for
input might be easier

> +
> +again:
> + if (len == 0)
> + break;
> +
> + tmp.pid = pid;
> + tmp.start = start;
> + tmp.len = len;
> +
> + cross = check_map_adj_cross(&tmp,
> + pos, &pgoff_diff, &cross_len);
> +
> + if (cross < MAP_ADJ_CROSS)
> + break;
> + if (cross > MAP_ADJ_CROSS)
> + continue;
> +
> + if (pgoff_diff <= 0) {
> + /*
> + * |<----- tmp ----->|
> + * |<----- pos ----->|
> + */
> +
> + adj_start = tmp.start;

SNIP

> +int parse_map_adjustment(const struct option *opt, const char *arg, int unset);
> +
> #endif /* __PERF_MACHINE_H */
> --
> 1.8.3.4
>