Date: Wed, 1 Apr 2015 15:21:40 +0200
From: Jiri Olsa <jolsa@redhat.com>
To: Wang Nan <wangnan0@huawei.com>
Cc: acme@kernel.org, jolsa@kernel.org, namhyung@kernel.org, mingo@redhat.com,
        lizefan@huawei.com, pi3orama@163.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/4] perf tools: report: introduce --map-adjustment
 argument.
Message-ID: <20150401132140.GC10820@krava.brq.redhat.com>
References: <1427884395-241111-1-git-send-email-wangnan0@huawei.com>
 <1427884395-241111-4-git-send-email-wangnan0@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1427884395-241111-4-git-send-email-wangnan0@huawei.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4477
Lines: 151

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
> 
--
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/