Re: [PATCH v2 01/17] perf tools: Configure contextID tracing in CPU-wide mode

[Mathieu Poirier]

On Fri, 7 Jun 2019 at 12:20, Arnaldo Carvalho de Melo
<arnaldo.melo@xxxxxxxxx> wrote:
>
> Em Fri, Jun 07, 2019 at 10:21:36AM +0100, Suzuki K Poulose escreveu:
> > Hi Mathieu,
> >
> > On 24/05/2019 18:34, Mathieu Poirier wrote:
> > > When operating in CPU-wide mode being notified of contextID changes is
> > > required so that the decoding mechanic is aware of the process context
> > > switch.
> > >
> > > Signed-off-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> >
> >
> > > Reviewed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> >
> > I am sorry but, I don't remember reviewing this patch in the previous
> > postings. But here we go.
>
> Can I keep it as is? I addressed one of your concerns below, please
> check.
>
> - Arnaldo
>
> > > +++ b/tools/perf/util/cs-etm.h
> > > @@ -103,6 +103,18 @@ struct intlist *traceid_list;
> > >   #define KiB(x) ((x) * 1024)
> > >   #define MiB(x) ((x) * 1024 * 1024)
> > > +/*
> > > + * Create a contiguous bitmask starting at bit position @l and ending at
> > > + * position @h. For example
> > > + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> > > + *
> > > + * Carbon copy of implementation found in $KERNEL/include/linux/bitops.h
> > > + */
> > > +#define GENMASK(h, l) \
> > > +   (((~0UL) - (1UL << (l)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
> > > +
> >
> > minor nit: Could this be placed in a more generic header file for the other
> > parts of the perf tool to consume ?
> >
>
> Yeah, since we have:
>
> Good catch, we have it already:
>
> [acme@quaco perf]$ tail tools/include/linux/bits.h
>  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
>  */
> #define GENMASK(h, l) \
>         (((~0UL) - (1UL << (l)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
>
> #define GENMASK_ULL(h, l) \
>         (((~0ULL) - (1ULL << (l)) + 1) & \
>          (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
>
> #endif  /* __LINUX_BITS_H */
> [acme@quaco perf]$
> [acme@quaco perf]$
>
> So I'm adding this to the pile with a Suggested-by: Suzuki, ok?
>
> commit 3217a621248824fbff8563d8447fdafe69c5316d
> Author: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Date:   Fri Jun 7 15:14:27 2019 -0300
>
>     perf cs-etm: Remove duplicate GENMASK() define, use linux/bits.h instead
>
>     Suzuki noticed that this should be more useful in a generic header, and
>     after looking I noticed we have it already in our copy of
>     include/linux/bits.h in tools/include, so just use it, test built on
>     x86-64 and ubuntu 19.04 with:
>
>       perfbuilder@46646c9e848e:/$ aarch64-linux-gnu-gcc --version |& head -1
>       aarch64-linux-gnu-gcc (Ubuntu/Linaro 8.3.0-6ubuntu1) 8.3.0
>       perfbuilder@46646c9e848e:/$
>
>     Suggested-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>     Link: https://lkml.kernel.org/r/68c1c548-33cd-31e8-100d-7ffad008c7b2@xxxxxxx
>     Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
>     Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
>     Cc: Leo Yan <leo.yan@xxxxxxxxxx>
>     Cc: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
>     Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
>     Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>     Cc: coresight@xxxxxxxxxxxxxxxx
>     Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx,
>     Link: https://lkml.kernel.org/n/tip-69pd3mqvxdlh2shddsc7yhyv@xxxxxxxxxxxxxx
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>
> diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h
> index 33b57e748c3d..bc848fd095f4 100644
> --- a/tools/perf/util/cs-etm.h
> +++ b/tools/perf/util/cs-etm.h
> @@ -9,6 +9,7 @@
>
>  #include "util/event.h"
>  #include "util/session.h"
> +#include <linux/bits.h>
>
>  /* Versionning header in case things need tro change in the future.  That way
>   * decoding of old snapshot is still possible.
> @@ -161,16 +162,6 @@ struct cs_etm_packet_queue {
>
>  #define CS_ETM_INVAL_ADDR 0xdeadbeefdeadbeefUL
>
> -/*
> - * Create a contiguous bitmask starting at bit position @l and ending at
> - * position @h. For example
> - * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> - *
> - * Carbon copy of implementation found in $KERNEL/include/linux/bitops.h
> - */
> -#define GENMASK(h, l) \
> -       (((~0UL) - (1UL << (l)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
> -

Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>

>  #define BMVAL(val, lsb, msb)   ((val & GENMASK(msb, lsb)) >> lsb)
>
>  #define CS_ETM_HEADER_SIZE (CS_HEADER_VERSION_0_MAX * sizeof(u64))