linux-kernel - Re: [PATCH v3 1/2] perf util: Check for fused instruction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20170706162402.GL27350@kernel.org>
Date:   Thu, 6 Jul 2017 13:24:02 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Jin Yao <yao.jin@...ux.intel.com>
Cc:     jolsa@...nel.org, peterz@...radead.org, mingo@...hat.com,
        alexander.shishkin@...ux.intel.com, Linux-kernel@...r.kernel.org,
        ak@...ux.intel.com, kan.liang@...el.com, yao.jin@...el.com
Subject: Re: [PATCH v3 1/2] perf util: Check for fused instruction

Em Tue, Jun 20, 2017 at 08:22:09PM +0800, Jin Yao escreveu:
> Macro fusion merges two instructions to a single micro-op. Intel
> core platform performs this hardware optimization under limited
> circumstances.
> 
> For example, CMP + JCC can be "fused" and executed /retired
> together. While with sampling this can result in the sample
> sometimes being on the JCC and sometimes on the CMP.
> So for the fused instruction pair, they could be considered
> together.
> 
> On Nehalem, fused instruction pairs:
> cmp/test + jcc.
> 
> On other new CPU:
> cmp/test/add/sub/and/inc/dec + jcc.
> 
> This patch adds an x86-specific function which checks if 2
> instructions are in a "fused" pair. For non-x86 arch, the
> function is just NULL.
> 
> Change-log:
> -----------
> v3: Add checking for Nehalem (CMP, TEST). For other newer
>     Intel CPUs just check it by default (CMP, TEST, ADD,
>     SUB, AND, INC, DEC).
> 
> v2: Remove the origial weak function. Arnaldo points out
>     that doing it as a weak function that will be overridden
>     by the host arch doesn't work. So now it's implemented
>     as an arch-specific function.
> 
> v1: Initial post
> 
> Signed-off-by: Jin Yao <yao.jin@...ux.intel.com>
> ---
>  tools/perf/arch/x86/annotate/instructions.c | 37 +++++++++++++++++++++++++++++
>  tools/perf/util/annotate.c                  | 12 ++++++++++
>  tools/perf/util/annotate.h                  |  2 ++
>  3 files changed, 51 insertions(+)
> 
> diff --git a/tools/perf/arch/x86/annotate/instructions.c b/tools/perf/arch/x86/annotate/instructions.c
> index c1625f2..8d06dd4 100644
> --- a/tools/perf/arch/x86/annotate/instructions.c
> +++ b/tools/perf/arch/x86/annotate/instructions.c
> @@ -76,3 +76,40 @@ static struct ins x86__instructions[] = {
>  	{ .name = "xbeginq",	.ops = &jump_ops, },
>  	{ .name = "retq",	.ops = &ret_ops,  },
>  };
> +
> +static bool x86__ins_is_fused(char *cpuid, const char *ins1, const char *ins2)
> +{
> +	unsigned int family, model, stepping;
> +	int ret;
> +
> +	/*
> +	 * cpuid = "GenuineIntel,family,model,stepping"
> +	 */
> +	ret = sscanf(cpuid, "%*[^,],%u,%u,%u", &family, &model, &stepping);

So, looking at the next patch, that uses this, I see that you'll call
this everytime that jump arrow will be printed, why not do this when
doing the initial disassembly, and having this info cached in the struct
ins or disasm_line (haven't looked closely to provide exact instructions
on how to do it)?

Even more, you could do this model checking just once per disassembly,
then use it as you go reading the disassembly lines, marking them as
fused/not fused and then at jump arror printing just look at a flag, no?

- Arnaldo

> +	if ((ret != 3) || (family != 6) || (model < 0x1e) ||
> +	     strstr(ins2, "jmp")) {
> +		return false;
> +	}
> +
> +	if (model == 0x1e) {
> +		/* Nehalem */
> +		if ((strstr(ins1, "cmp") && !strstr(ins1, "xchg")) ||
> +		     strstr(ins1, "test")) {
> +			return true;
> +		}
> +	} else {
> +		/* Newer platform */
> +		if ((strstr(ins1, "cmp") && !strstr(ins1, "xchg")) ||
> +		     strstr(ins1, "test") ||
> +		     strstr(ins1, "add") ||
> +		     strstr(ins1, "sub") ||
> +		     strstr(ins1, "and") ||
> +		     strstr(ins1, "inc") ||
> +		     strstr(ins1, "dec")) {
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index be1caab..8cf6025 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -48,6 +48,8 @@ struct arch {
>  	bool		initialized;
>  	void		*priv;
>  	int		(*init)(struct arch *arch);
> +	bool		(*ins_is_fused)(char *cpuid, const char *ins1,
> +					const char *ins2);
>  	struct		{
>  		char comment_char;
>  		char skip_functions_char;
> @@ -129,6 +131,7 @@ static struct arch architectures[] = {
>  		.name = "x86",
>  		.instructions = x86__instructions,
>  		.nr_instructions = ARRAY_SIZE(x86__instructions),
> +		.ins_is_fused = x86__ins_is_fused,
>  		.objdump =  {
>  			.comment_char = '#',
>  		},
> @@ -171,6 +174,15 @@ int ins__scnprintf(struct ins *ins, char *bf, size_t size,
>  	return ins__raw_scnprintf(ins, bf, size, ops);
>  }
>  
> +bool ins__is_fused(struct arch *arch, char *cpuid, const char *ins1,
> +		   const char *ins2)
> +{
> +	if (!arch || !arch->ins_is_fused || !cpuid)
> +		return false;
> +
> +	return arch->ins_is_fused(cpuid, ins1, ins2);
> +}
> +
>  static int call__parse(struct arch *arch, struct ins_operands *ops, struct map *map)
>  {
>  	char *endptr, *tok, *name;
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 2105503..35cb7b5 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -53,6 +53,8 @@ bool ins__is_jump(const struct ins *ins);
>  bool ins__is_call(const struct ins *ins);
>  bool ins__is_ret(const struct ins *ins);
>  int ins__scnprintf(struct ins *ins, char *bf, size_t size, struct ins_operands *ops);
> +bool ins__is_fused(struct arch *arch, char *cpuid, const char *ins1,
> +		   const char *ins2);
>  
>  struct annotation;
>  
> -- 
> 2.7.4