Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
References: <20231122092703.3163191-1-tmricht@linux.ibm.com>
In-Reply-To: <20231122092703.3163191-1-tmricht@linux.ibm.com>
From:   Namhyung Kim <namhyung@kernel.org>
Date:   Wed, 22 Nov 2023 18:34:14 -0800
Message-ID: <CAM9d7cjoniVhpb=xK8WSN1C80y4D25Tjedax9AmwPf1w2FsKrA@mail.gmail.com>
Subject: Re: [PATCH linux-next] perf report: remove warning on missing raw
 data for s390
To:     Thomas Richter <tmricht@linux.ibm.com>
Cc:     linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
        acme@kernel.org, svens@linux.ibm.com, gor@linux.ibm.com,
        sumanthk@linux.ibm.com, hca@linux.ibm.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

Hello,

On Wed, Nov 22, 2023 at 1:27 AM Thomas Richter <tmricht@linux.ibm.com> wrote:
>
> Command
>  # ./perf report -i /tmp/111 -D > /dev/null
> emits an error message when a sample for event CRYPTO_ALL in the
> perf.data file does not contain any raw data. This is ok.
> Do not trigger this warning when the sample in the perf.data files
> does not contain any raw data at all.
> Check for availability of raw data for all events and return if
> none is available.
>
> Output before:
>  # ./perf report -i /tmp/111 -D > /dev/null
>  Invalid CRYPTO_ALL raw data encountered
>  Invalid CRYPTO_ALL raw data encountered
>  Invalid CRYPTO_ALL raw data encountered
>  #
>
> Output after:
>  # ./perf report -i /tmp/111 -D > /dev/null
>  #
>
> Fixes: b539deafbadb ("perf report: Add s390 raw data interpretation for PAI counters")
> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
> Acked-by: Sumanth Korikkar <sumanthk@linux.ibm.com>

Acked-by: Namhyung Kim <namhyung@kernel.org>

> ---
>  tools/perf/util/s390-sample-raw.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/s390-sample-raw.c b/tools/perf/util/s390-sample-raw.c
> index 29a744eeb71e..53383e97ec9d 100644
> --- a/tools/perf/util/s390-sample-raw.c
> +++ b/tools/perf/util/s390-sample-raw.c
> @@ -51,8 +51,6 @@ static bool s390_cpumcfdg_testctr(struct perf_sample *sample)
>         struct cf_trailer_entry *te;
>         struct cf_ctrset_entry *cep, ce;
>
> -       if (!len)
> -               return false;
>         while (offset < len) {
>                 cep = (struct cf_ctrset_entry *)(buf + offset);
>                 ce.def = be16_to_cpu(cep->def);
> @@ -234,10 +232,9 @@ struct pai_data {          /* Event number and value */
>   */
>  static bool s390_pai_all_test(struct perf_sample *sample)
>  {
> -       unsigned char *buf = sample->raw_data;
>         size_t len = sample->raw_size;
>
> -       if (len < 0xa || !buf)
> +       if (len < 0xa)
>                 return false;
>         return true;
>  }
> @@ -299,6 +296,10 @@ void evlist__s390_sample_raw(struct evlist *evlist, union perf_event *event,
>         if (!evsel)
>                 return;
>
> +       /* Check for raw data in sample */
> +       if (!sample->raw_size || !sample->raw_data)
> +               return;

I was thinking it should check if evsel->attr.sample_type has
PERF_SAMPLE_RAW.  But evsel__parse_sample() sets the
sample->raw_data and ->raw_size only if it has the flag.
So I think it's fine.

Thanks,
Namhyung


> +
>         /* Display raw data on screen */
>         if (evsel->core.attr.config == PERF_EVENT_CPUM_CF_DIAG) {
>                 if (!evsel->pmu)
> --
> 2.41.0
>
>