linux-kernel - Re: [PATCH] perf annotate: Fix sample events lost in stdio mode

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: <53ff575f-1fcf-6650-76ad-a0304f6bdf15@huawei.com>
Date:   Thu, 11 Mar 2021 16:48:18 +0800
From:   Yang Jihong <yangjihong1@...wei.com>
To:     <peterz@...radead.org>, <mingo@...hat.com>, <acme@...nel.org>,
        <mark.rutland@....com>, <alexander.shishkin@...ux.intel.com>,
        <jolsa@...hat.com>, <namhyung@...nel.org>,
        <yao.jin@...ux.intel.com>, <gustavoars@...nel.org>,
        <mliska@...e.cz>, <linux-kernel@...r.kernel.org>
CC:     <zhangjinhao2@...wei.com>
Subject: Re: [PATCH] perf annotate: Fix sample events lost in stdio mode

Hello,

On 2021/3/6 16:28, Yang Jihong wrote:
> In hist__find_annotations function, since have a hist_entry per IP for the same
> symbol, we free notes->src to signal already processed this symbol in stdio mode;
> when annotate, entry will skipped if notes->src is NULL to avoid repeated output.
> 
> However, there is a problem, for example, run the following command:
> 
>   # perf record -e branch-misses -e branch-instructions -a sleep 1
> 
> perf.data file contains different types of sample event.
> 
> If the same IP sample event exists in branch-misses and branch-instructions,
> this event uses the same symbol. When annotate branch-misses events, notes->src
> corresponding to this event is set to null, as a result, when annotate
> branch-instructions events, this event is skipped and no annotate is output.
> 
> Solution of this patch is to add a u8 member to struct sym_hist and use a bit to
> indicate whether the symbol has been processed.
> Because different types of event correspond to different sym_hist, no conflict
> occurs.
> ---
>   tools/perf/builtin-annotate.c | 22 ++++++++++++++--------
>   tools/perf/util/annotate.h    |  4 ++++
>   2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index a23ba6bb99b6..c8c67892ae82 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -372,15 +372,21 @@ static void hists__find_annotations(struct hists *hists,
>   			if (next != NULL)
>   				nd = next;
>   		} else {
> -			hist_entry__tty_annotate(he, evsel, ann);
> +			struct sym_hist *h = annotated_source__histogram(notes->src,
> +									 evsel->idx);
> +
> +			if (h->processed == 0) {
> +				hist_entry__tty_annotate(he, evsel, ann);
> +
> +				/*
> +				 * Since we have a hist_entry per IP for the same
> +				 * symbol, set processed flag of evsel in sym_hist
> +				 * to signal we already processed this symbol.
> +				 */
> +				h->processed = 1;
> +			}
> +
>   			nd = rb_next(nd);
> -			/*
> -			 * Since we have a hist_entry per IP for the same
> -			 * symbol, free he->ms.sym->src to signal we already
> -			 * processed this symbol.
> -			 */
> -			zfree(&notes->src->cycles_hist);
> -			zfree(&notes->src);
>   		}
>   	}
>   }
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 096cdaf21b01..89872bfdc958 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -228,6 +228,10 @@ void symbol__calc_percent(struct symbol *sym, struct evsel *evsel);
>   struct sym_hist {
>   	u64		      nr_samples;
>   	u64		      period;
> +
> +	u8		      processed  : 1, /* whether symbol has been processed, used for annotate */
> +			      __reserved : 7;
> +
>   	struct sym_hist_entry addr[];
>   };
>   
> 
Please check whether this solution is feasible, look forward to your review.

Thanks,
Yang