CoreSight

coresight@lists.linaro.org

6 participants
2351 discussions

[PATCH v2 0/4] coresight: etm: Correct PID tracing for non-root namespace

by Leo Yan

If a profiling program runs in a non-root PID namespace, if CoreSight driver enables PID tracing (with contextID), it can lead to mismatching issue between the context ID traced in hardware (from the root namespace) and the PIDs gathered by profiling tool (e.g. perf) in its non-root namespace. CoreSight driver has tried to address this issue for the contextID related interfaces under sysfs, but it misses to prevent user to set VMID (virtual contextID) for kernel runs in EL2 with VHE; furthermore, it misses to handle the case when the profiling tool runs in the non-root PID namespace. For this reason, this patch series is to correct contextID tracing for non-root namespace. After applied this patchset, patch 02 doesn't permit users to access virtual contextID via sysfs nodes in the non-root PID namespace, patch 03 and 04 stop to trace PID packet for non-root PID namespace. This patch is dependent on the patchset "pid: Introduce helper task_is_in_root_ns()" [1]. [1] https://lore.kernel.org/lkml/20211208083320.472503-1-leo.yan@linaro.org/ Leo Yan (4): coresight: etm4x: Add lock for reading virtual context ID comparator coresight: etm4x: Don't use virtual contextID for non-root PID namespace coresight: etm4x: Don't trace PID for non-root PID namespace coresight: etm3x: Don't trace PID for non-root PID namespace .../coresight/coresight-etm3x-core.c | 4 +++ .../coresight/coresight-etm4x-core.c | 10 +++++-- .../coresight/coresight-etm4x-sysfs.c | 30 +++++++++++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-) -- 2.25.1

3 years, 5 months

Re: [PATCH v2 4/7] connector/cn_proc: Use task_is_in_init_pid_ns()

by Leo Yan

On Tue, Dec 14, 2021 at 04:58:32PM +1100, Balbir Singh wrote: > On Wed, Dec 08, 2021 at 04:33:17PM +0800, Leo Yan wrote: > > This patch replaces open code with task_is_in_init_pid_ns() to check if > > a task is in root PID namespace. > > > > Signed-off-by: Leo Yan <leo.yan(a)linaro.org> > > --- > > drivers/connector/cn_proc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c > > index 646ad385e490..ccac1c453080 100644 > > --- a/drivers/connector/cn_proc.c > > +++ b/drivers/connector/cn_proc.c > > @@ -358,7 +358,7 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, > > * other namespaces. > > */ > > if ((current_user_ns() != &init_user_ns) || > > - (task_active_pid_ns(current) != &init_pid_ns)) > > + !task_is_in_init_pid_ns(current)) > > return; > > > > Sounds like there might scope for other wrappers - is_current_in_user_init_ns() Indeed, for new wrapper is_current_in_user_init_ns(), I searched kernel and located there have multiple places use open code. If no one works on this refactoring, I will send a new patchset for it separately. > Acked-by: Balbir Singh <bsingharora(a)gmail.com> Thank you for review, Balbir! Also thanks review from Leon and Suzuki. Leo

3 years, 5 months

Re: [PATCH v2 4/4] coresight: etm3x: Don't trace PID for non-root PID namespace

by Leo Yan

Hi, On Tue, Dec 14, 2021 at 03:16:42AM +0800, kernel test robot wrote: > Hi Leo, > > I love your patch! Yet something to improve: > > [auto build test ERROR on linus/master] > [also build test ERROR on v5.16-rc5] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Leo-Yan/coresight-etm-Correct-PID-… > base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2585cf9dfaaddf00b069673f27bb3f8530e2039c > config: arm-buildonly-randconfig-r003-20211213 (https://download.01.org/0day-ci/archive/20211214/202112140344.viPmOWp6-lkp@…) > compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project b6a2ddb6c8ac29412b1361810972e15221fa021c) > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # install arm cross compiling tool for clang build > # apt-get install binutils-arm-linux-gnueabi > # https://github.com/0day-ci/linux/commit/81d5f47788c40d34c8159d64d4505eb4852… > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Leo-Yan/coresight-etm-Correct-PID-tracing-for-non-root-namespace/20211213-201632 > git checkout 81d5f47788c40d34c8159d64d4505eb485254e8f > # save the config file to linux build tree > mkdir build_dir > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/hwtracing/coresight/ > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp(a)intel.com> > > All errors (new ones prefixed by >>): > > >> drivers/hwtracing/coresight/coresight-etm3x-core.c:344:7: error: implicit declaration of function 'task_is_in_init_pid_ns' [-Werror,-Wimplicit-function-declaration] > if (!task_is_in_init_pid_ns(current)) > ^ > 1 error generated. > > > vim +/task_is_in_init_pid_ns +344 drivers/hwtracing/coresight/coresight-etm3x-core.c > > 301 > 302 #define ETM3X_SUPPORTED_OPTIONS (ETMCR_CYC_ACC | \ > 303 ETMCR_TIMESTAMP_EN | \ > 304 ETMCR_RETURN_STACK) > 305 > 306 static int etm_parse_event_config(struct etm_drvdata *drvdata, > 307 struct perf_event *event) > 308 { > 309 struct etm_config *config = &drvdata->config; > 310 struct perf_event_attr *attr = &event->attr; > 311 > 312 if (!attr) > 313 return -EINVAL; > 314 > 315 /* Clear configuration from previous run */ > 316 memset(config, 0, sizeof(struct etm_config)); > 317 > 318 if (attr->exclude_kernel) > 319 config->mode = ETM_MODE_EXCL_KERN; > 320 > 321 if (attr->exclude_user) > 322 config->mode = ETM_MODE_EXCL_USER; > 323 > 324 /* Always start from the default config */ > 325 etm_set_default(config); > 326 > 327 /* > 328 * By default the tracers are configured to trace the whole address > 329 * range. Narrow the field only if requested by user space. > 330 */ > 331 if (config->mode) > 332 etm_config_trace_mode(config); > 333 > 334 /* > 335 * At this time only cycle accurate, return stack and timestamp > 336 * options are available. > 337 */ > 338 if (attr->config & ~ETM3X_SUPPORTED_OPTIONS) > 339 return -EINVAL; > 340 > 341 config->ctrl = attr->config; > 342 > 343 /* Don't trace contextID when runs in non-root PID namespace */ > > 344 if (!task_is_in_init_pid_ns(current)) This patchset is based on another patchset [1], as described on the cover letter patch. This is why here reports for building failure. To avoid the false positive reporting, if any better practice I can follow up to resolve the dependency between two patchsets, please let me know and I will do in next time. Thanks, Leo [1] https://lore.kernel.org/lkml/20211208083320.472503-1-leo.yan@linaro.org/ > 345 config->ctrl &= ~ETMCR_CTXID_SIZE; > 346 > 347 /* > 348 * Possible to have cores with PTM (supports ret stack) and ETM > 349 * (never has ret stack) on the same SoC. So if we have a request > 350 * for return stack that can't be honoured on this core then > 351 * clear the bit - trace will still continue normally > 352 */ > 353 if ((config->ctrl & ETMCR_RETURN_STACK) && > 354 !(drvdata->etmccer & ETMCCER_RETSTACK)) > 355 config->ctrl &= ~ETMCR_RETURN_STACK; > 356 > 357 return 0; > 358 } > 359 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

3 years, 5 months

Re: [PATCH] coresight: core: fix typo in a comment

by Mathieu Poirier

On Sat, Dec 11, 2021 at 05:02:21PM +0800, Jason Wang wrote: > The double `the' in the comment in line 732 is repeated. Remove one > of them from the comment. > > Signed-off-by: Jason Wang <wangborong(a)cdjrlc.com> > --- > drivers/hwtracing/coresight/coresight-core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index 8a18c71df37a..88653d1c06a4 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -729,7 +729,7 @@ static inline void coresight_put_ref(struct coresight_device *csdev) > * coresight_grab_device - Power up this device and any of the helper > * devices connected to it for trace operation. Since the helper devices > * don't appear on the trace path, they should be handled along with the > - * the master device. > + * master device. Applied. Thanks, Mathieu > */ > static int coresight_grab_device(struct coresight_device *csdev) > { > -- > 2.34.1 >

3 years, 5 months

Re: [PATCH 00/22] Refactor perf cpumap

by James Clark

On 08/12/2021 02:45, Ian Rogers wrote: > Perf cpu map has various functions where a cpumap and index are passed > in order to load the cpu. A problem with this is that the wrong index > may be passed for the cpumap, causing problems like aggregation on the > wrong CPU: > https://lore.kernel.org/lkml/20211204023409.969668-1-irogers@google.com/ > > This patch set refactors the cpu map API, greatly reducing it and > explicitly passing the cpu (rather than the pair) to functions that > need it. Comments are added at the same time. > > Ian Rogers (22): > libperf: Add comments to perf_cpu_map. > perf stat: Add aggr creators that are passed a cpu. > perf stat: Switch aggregation to use for_each loop > perf stat: Switch to cpu version of cpu_map__get > perf cpumap: Switch cpu_map__build_map to cpu function > perf cpumap: Remove map+index get_socket > perf cpumap: Remove map+index get_die > perf cpumap: Remove map+index get_core > perf cpumap: Remove map+index get_node > perf cpumap: Add comments to aggr_cpu_id > perf cpumap: Remove unused cpu_map__socket > perf cpumap: Simplify equal function name. > perf cpumap: Rename empty functions. > perf cpumap: Document cpu__get_node and remove redundant function > perf cpumap: Remove map from function names that don't use a map. > perf cpumap: Remove cpu_map__cpu, use libperf function. > perf cpumap: Refactor cpu_map__build_map > perf cpumap: Rename cpu_map__get_X_aggr_by_cpu functions > perf cpumap: Move 'has' function to libperf > perf cpumap: Add some comments to cpu_aggr_map > perf cpumap: Trim the cpu_aggr_map > perf stat: Fix memory leak in check_per_pkg > > tools/lib/perf/cpumap.c | 7 +- > tools/lib/perf/include/internal/cpumap.h | 9 +- > tools/lib/perf/include/perf/cpumap.h | 1 + > tools/perf/arch/arm/util/cs-etm.c | 16 +- > tools/perf/builtin-ftrace.c | 2 +- > tools/perf/builtin-sched.c | 6 +- > tools/perf/builtin-stat.c | 273 ++++++++++++----------- > tools/perf/tests/topology.c | 10 +- > tools/perf/util/cpumap.c | 182 ++++++--------- > tools/perf/util/cpumap.h | 102 ++++++--- > tools/perf/util/cputopo.c | 2 +- > tools/perf/util/env.c | 6 +- > tools/perf/util/stat-display.c | 69 +++--- > tools/perf/util/stat.c | 9 +- > tools/perf/util/stat.h | 3 +- > 15 files changed, 361 insertions(+), 336 deletions(-) > For the whole set: Reviewed-by: James Clark <james.clark(a)arm.com> I didn't see any obvious issues with mixing up aggregation modes or CPU/idx types. Also gave perf stat a test in the different modes and didn't see an issue. But I'm wondering if it's possible to go further and add a struct around the CPU int so that the compiler checks for correctness instead. It still seems quite easy to mix up index and CPU, for example these functions are subtly different, but both use int: LIBPERF_API int perf_cpu_map__cpu(const struct perf_cpu_map *cpus, int idx); LIBPERF_API bool perf_cpu_map__has(const struct perf_cpu_map *map, int cpu); Something like this would make it impossible to make a mistake: struct cpu { int cpu }; I mean it's more of a coincidence that CPUs can be identified by an integer, but they are more of an object than an integer, so it could make sense to wrap it. But maybe it could be quite cumbersome to use and be overkill.

3 years, 5 months

Re: [PATCH 02/22] perf stat: Add aggr creators that are passed a cpu.

by Arnaldo Carvalho de Melo

Em Tue, Dec 07, 2021 at 06:45:47PM -0800, Ian Rogers escreveu: > The cpu_map and index can get confused. Add variants of the cpu_map__get > routines that are passed a cpu. Make the existing cpu_map__get routines > use the new functions with a view to remove them when no longer used. Looks ok from a quick lock > Signed-off-by: Ian Rogers <irogers(a)google.com> > --- > tools/perf/util/cpumap.c | 79 +++++++++++++++++++++++----------------- > tools/perf/util/cpumap.h | 6 ++- > 2 files changed, 51 insertions(+), 34 deletions(-) > > diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c > index 87d3eca9b872..49fba2c53822 100644 > --- a/tools/perf/util/cpumap.c > +++ b/tools/perf/util/cpumap.c > @@ -128,21 +128,23 @@ int cpu_map__get_socket_id(int cpu) > return ret ?: value; > } > > -struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx, > - void *data __maybe_unused) > +struct aggr_cpu_id cpu_map__get_socket_aggr_by_cpu(int cpu, void *data __maybe_unused) > { > - int cpu; > struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id(); > > - if (idx > map->nr) > - return id; > - > - cpu = map->map[idx]; > - > id.socket = cpu_map__get_socket_id(cpu); > return id; > } > > +struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx, > + void *data) > +{ > + if (idx < 0 || idx > map->nr) > + return cpu_map__empty_aggr_cpu_id(); > + > + return cpu_map__get_socket_aggr_by_cpu(map->map[idx], data); > +} > + > static int cmp_aggr_cpu_id(const void *a_pointer, const void *b_pointer) > { > struct aggr_cpu_id *a = (struct aggr_cpu_id *)a_pointer; > @@ -200,15 +202,10 @@ int cpu_map__get_die_id(int cpu) > return ret ?: value; > } > > -struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data) > +struct aggr_cpu_id cpu_map__get_die_aggr_by_cpu(int cpu, void *data) > { > - int cpu, die; > - struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id(); > - > - if (idx > map->nr) > - return id; > - > - cpu = map->map[idx]; > + struct aggr_cpu_id id; > + int die; > > die = cpu_map__get_die_id(cpu); > /* There is no die_id on legacy system. */ > @@ -220,7 +217,7 @@ struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *dat > * with the socket ID and then add die to > * make a unique ID. > */ > - id = cpu_map__get_socket(map, idx, data); > + id = cpu_map__get_socket_aggr_by_cpu(cpu, data); > if (cpu_map__aggr_cpu_id_is_empty(id)) > return id; > > @@ -228,6 +225,15 @@ struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *dat > return id; > } > > +struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, > + void *data) > +{ > + if (idx < 0 || idx > map->nr) > + return cpu_map__empty_aggr_cpu_id(); > + > + return cpu_map__get_die_aggr_by_cpu(map->map[idx], data); > +} > + > int cpu_map__get_core_id(int cpu) > { > int value, ret = cpu__get_topology_int(cpu, "core_id", &value); > @@ -239,20 +245,13 @@ int cpu_map__get_node_id(int cpu) > return cpu__get_node(cpu); > } > > -struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data) > +struct aggr_cpu_id cpu_map__get_core_aggr_by_cpu(int cpu, void *data) > { > - int cpu; > - struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id(); > - > - if (idx > map->nr) > - return id; > - > - cpu = map->map[idx]; > - > - cpu = cpu_map__get_core_id(cpu); > + struct aggr_cpu_id id; > + int core = cpu_map__get_core_id(cpu); > > /* cpu_map__get_die returns a struct with socket and die set*/ > - id = cpu_map__get_die(map, idx, data); > + id = cpu_map__get_die_aggr_by_cpu(cpu, data); > if (cpu_map__aggr_cpu_id_is_empty(id)) > return id; > > @@ -260,19 +259,33 @@ struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *da > * core_id is relative to socket and die, we need a global id. > * So we combine the result from cpu_map__get_die with the core id > */ > - id.core = cpu; > + id.core = core; > return id; > + > } > > -struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data __maybe_unused) > +struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data) > +{ > + if (idx < 0 || idx > map->nr) > + return cpu_map__empty_aggr_cpu_id(); > + > + return cpu_map__get_core_aggr_by_cpu(map->map[idx], data); > +} > + > +struct aggr_cpu_id cpu_map__get_node_aggr_by_cpu(int cpu, void *data __maybe_unused) > { > struct aggr_cpu_id id = cpu_map__empty_aggr_cpu_id(); > > + id.node = cpu_map__get_node_id(cpu); > + return id; > +} > + > +struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data) > +{ > if (idx < 0 || idx >= map->nr) > - return id; > + return cpu_map__empty_aggr_cpu_id(); > > - id.node = cpu_map__get_node_id(map->map[idx]); > - return id; > + return cpu_map__get_node_aggr_by_cpu(map->map[idx], data); > } > > int cpu_map__build_socket_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **sockp) > diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h > index a27eeaf086e8..c62d67704425 100644 > --- a/tools/perf/util/cpumap.h > +++ b/tools/perf/util/cpumap.h > @@ -31,13 +31,17 @@ size_t cpu_map__snprint(struct perf_cpu_map *map, char *buf, size_t size); > size_t cpu_map__snprint_mask(struct perf_cpu_map *map, char *buf, size_t size); > size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp); > int cpu_map__get_socket_id(int cpu); > +struct aggr_cpu_id cpu_map__get_socket_aggr_by_cpu(int cpu, void *data); > struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx, void *data); > int cpu_map__get_die_id(int cpu); > +struct aggr_cpu_id cpu_map__get_die_aggr_by_cpu(int cpu, void *data); > struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data); > int cpu_map__get_core_id(int cpu); > +struct aggr_cpu_id cpu_map__get_core_aggr_by_cpu(int cpu, void *data); > struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data); > int cpu_map__get_node_id(int cpu); > -struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data); > +struct aggr_cpu_id cpu_map__get_node_aggr_by_cpu(int cpu, void *data); > +struct aggr_cpu_id cpu_map__get_node(struct perf_cpu_map *map, int idx, void *data); > int cpu_map__build_socket_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **sockp); > int cpu_map__build_die_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **diep); > int cpu_map__build_core_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **corep); > -- > 2.34.1.400.ga245620fadb-goog -- - Arnaldo

3 years, 5 months

Re: [PATCH 01/22] libperf: Add comments to perf_cpu_map.

by Arnaldo Carvalho de Melo

Em Wed, Dec 08, 2021 at 06:34:14AM -0800, Ian Rogers escreveu: > On Wed, Dec 8, 2021 at 4:06 AM John Garry <john.garry(a)huawei.com> wrote: > > > > On 08/12/2021 02:45, Ian Rogers wrote: > > > diff --git a/tools/lib/perf/include/internal/cpumap.h b/tools/lib/perf/include/internal/cpumap.h > > > index 840d4032587b..1c1726f4a04e 100644 > > > --- a/tools/lib/perf/include/internal/cpumap.h > > > +++ b/tools/lib/perf/include/internal/cpumap.h > > > @@ -4,9 +4,16 @@ > > > > > > #include <linux/refcount.h> > > > > > > +/** > > > + * A sized, reference counted, sorted array of integers representing CPU > > > + * numbers. This is commonly used to capture which CPUs a PMU is associated > > > + * with. > > > + */ > > > struct perf_cpu_map { > > > refcount_t refcnt; > > > + /** Length of the map array. */ > > > int nr; > > > + /** The CPU values. */ > > > int map[]; > > > > would simply more distinct names for the variables help instead of or in > > addition to comments? Well, in this case the typical usage doesn't help, as 'struct perf_cpu_map' are being used simply as "map" where it should be cpu_map, so we would have: cpu_map->nr And all should be obvious, no? Otherwise we would have redundant 'cpu', like: cpu_map->nr_cpus And 'map' should really be entries, so: cpu_map->entries[index]; Would be clear enough, o? > Thanks John! I agree. The phrase that is often used is intention > revealing names. The kernel style for naming is to be brief: > https://www.kernel.org/doc/html/v4.10/process/coding-style.html#naming > These names are both brief. nr is a little unusual, of course an > integer is a number - size and length are common names in situations > like these. In this case number makes sense as it is the number of > CPUs in the array, and there is a certain readability in saying number > of CPUs and not length or size of CPUs. The name map I have issue > with, it is always a smell if you are calling a variable a data type. > Given the convention in the context of this code I decided to leave > it. Something like array_of_cpu_values would be more intention > revealing but when run through the variable name shrinkifier could end > up as just being array, which would be little better than map. > > The guidance on comments is that they are good and to focus on the > what of what the code is doing: > https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting > refcnt was intention revealing enough and so I didn't add a comment to it. > > > Generally developers don't always check comments where the struct is > > defined when the meaning could be judged intuitively > > Agreed. I think there could be a follow up to change to better names. > As I was lacking a better suggestion I think for the time being, and > in this patch set, we can keep things as they are. > > Thanks, > Ian > > > Thanks, > > John > > -- - Arnaldo

3 years, 5 months

[PATCH] perf cs-etm: Remove duplicate and incorrect aux size checks

by James Clark

There are two checks, one is for size when running without admin, but this one is covered by the driver and reported on in more detail here (builtin-record.c): pr_err("Permission error mapping pages.\n" "Consider increasing " "/proc/sys/kernel/perf_event_mlock_kb,\n" "or try again with a smaller value of -m/--mmap_pages.\n" "(current value: %u,%u)\n", This had the effect of artificially limiting the aux buffer size to a value smaller than what was allowed because perf_event_mlock_kb wasn't taken into account. The second is to check for a power of two, but this is covered here (evlist.c): pr_info("rounding mmap pages size to %s (%lu pages)\n", buf, pages); Signed-off-by: James Clark <james.clark(a)arm.com> --- tools/perf/arch/arm/util/cs-etm.c | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c index 293a23bf8be3..8a3d54a86c9c 100644 --- a/tools/perf/arch/arm/util/cs-etm.c +++ b/tools/perf/arch/arm/util/cs-etm.c @@ -407,25 +407,6 @@ static int cs_etm_recording_options(struct auxtrace_record *itr, } - /* Validate auxtrace_mmap_pages provided by user */ - if (opts->auxtrace_mmap_pages) { - unsigned int max_page = (KiB(128) / page_size); - size_t sz = opts->auxtrace_mmap_pages * (size_t)page_size; - - if (!privileged && - opts->auxtrace_mmap_pages > max_page) { - opts->auxtrace_mmap_pages = max_page; - pr_err("auxtrace too big, truncating to %d\n", - max_page); - } - - if (!is_power_of_2(sz)) { - pr_err("Invalid mmap size for %s: must be a power of 2\n", - CORESIGHT_ETM_PMU_NAME); - return -EINVAL; - } - } - if (opts->auxtrace_snapshot_mode) pr_debug2("%s snapshot size: %zu\n", CORESIGHT_ETM_PMU_NAME, opts->auxtrace_snapshot_size); -- 2.28.0

3 years, 5 months

Re: [RFC PATCH] Change coresight tmc from misc device to cdev device

by Mathieu Poirier

Hi Jay, On Wed, Dec 01, 2021 at 03:09:42PM +0800, Jay Chen wrote: > For coresight in the server scenario, if there are too many > cpu cores and too many tmc peripherals, the error of insufficient > device numbers will occur by misc device > There really is more than 128 miscdevice allocated in your system? How may TMC do you have? > Signed-off-by: Jay Chen <jkchen(a)linux.alibaba.com> > --- > drivers/hwtracing/coresight/coresight-core.c | 53 +++++++++++++++++++ > .../hwtracing/coresight/coresight-tmc-core.c | 13 ++--- > drivers/hwtracing/coresight/coresight-tmc.h | 4 +- > include/linux/coresight.h | 23 ++++++++ > 4 files changed, 83 insertions(+), 10 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index 8a18c71df37a..b0cca3060f4e 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -17,6 +17,7 @@ > #include <linux/coresight.h> > #include <linux/of_platform.h> > #include <linux/delay.h> > +#include <linux/cdev.h> > #include <linux/pm_runtime.h> > > #include "coresight-etm-perf.h" > @@ -1567,6 +1568,58 @@ void coresight_release_platform_data(struct coresight_device *csdev, > coresight_remove_conns_sysfs_group(csdev); > } > > +int coresight_cdev_register(struct coresight_cdev *cdev, const struct file_operations *fops, > + const char *name) > +{ > + struct device *device; > + > + if (alloc_chrdev_region(&cdev->devno, 0, 1, name)) { > + pr_err("failed to create the coresight cdev region\n"); > + return -EFAULT; > + } > + > + cdev->class = class_create(THIS_MODULE, name); > + if (IS_ERR(cdev->class)) { > + pr_err("failed to create the coresight class\n"); > + goto coresight_unregister_region; > + } >From the above a new class is created for every TMC, even if they are all the same. A better way to do this would be to create a single TMC class with a new cdev for every new TMC. To do this alloc_chrdev_region() and class_create() have to be called when the module is added to the system. Have a look at funnel_init() on how to refactor the TMC core driver with an init function and rpmsg_chrdev_init() for the chrdev region allocation and the class creation. >From there every time a TMC is probe cdev_init() and cdev_add() can be called - look at rpmsg_chrdev_probe() for an example. Instead of using an IDA to pick out a minor number you should be able to use dev_list->nr_idx - 1. Thanks, Mathieu > + > + cdev_init(&cdev->cdev, fops); > + if (cdev_add(&cdev->cdev, cdev->devno, 1)) { > + pr_err("failed to add the coresight cdev\n"); > + goto coresight_free_class; > + } > + > + device = device_create(cdev->class, NULL, cdev->devno, > + cdev, name); > + if (IS_ERR(device)) { > + pr_err("failed to create device for coresight\n"); > + goto coresight_del_cdev; > + } > + > + return 0; > + > +coresight_del_cdev: > + cdev_del(&cdev->cdev); > +coresight_free_class: > + class_destroy(cdev->class); > +coresight_unregister_region: > + unregister_chrdev_region(cdev->devno, 1); > + > + return -EFAULT; > +} > +EXPORT_SYMBOL_GPL(coresight_cdev_register); > + > +int coresight_cdev_unregister(struct coresight_cdev *cdev) > +{ > + device_destroy(cdev->class, cdev->devno); > + cdev_del(&cdev->cdev); > + class_destroy(cdev->class); > + unregister_chrdev_region(cdev->devno, 1); > + return 0; > +} > +EXPORT_SYMBOL_GPL(coresight_cdev_unregister); > + > struct coresight_device *coresight_register(struct coresight_desc *desc) > { > int ret; > diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c > index 74c6323d4d6a..c4aae8ff65eb 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-core.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c > @@ -147,7 +147,7 @@ static int tmc_open(struct inode *inode, struct file *file) > { > int ret; > struct tmc_drvdata *drvdata = container_of(file->private_data, > - struct tmc_drvdata, miscdev); > + struct tmc_drvdata, cdev); > > ret = tmc_read_prepare(drvdata); > if (ret) > @@ -179,7 +179,7 @@ static ssize_t tmc_read(struct file *file, char __user *data, size_t len, > char *bufp; > ssize_t actual; > struct tmc_drvdata *drvdata = container_of(file->private_data, > - struct tmc_drvdata, miscdev); > + struct tmc_drvdata, cdev); > actual = tmc_get_sysfs_trace(drvdata, *ppos, len, &bufp); > if (actual <= 0) > return 0; > @@ -200,7 +200,7 @@ static int tmc_release(struct inode *inode, struct file *file) > { > int ret; > struct tmc_drvdata *drvdata = container_of(file->private_data, > - struct tmc_drvdata, miscdev); > + struct tmc_drvdata, cdev); > > ret = tmc_read_unprepare(drvdata); > if (ret) > @@ -529,10 +529,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) > goto out; > } > > - drvdata->miscdev.name = desc.name; > - drvdata->miscdev.minor = MISC_DYNAMIC_MINOR; > - drvdata->miscdev.fops = &tmc_fops; > - ret = misc_register(&drvdata->miscdev); > + ret = coresight_cdev_register(&drvdata->cdev, &tmc_fops, desc.name); > if (ret) > coresight_unregister(drvdata->csdev); > else > @@ -572,7 +569,7 @@ static void tmc_remove(struct amba_device *adev) > * etb fops in this case, device is there until last file > * handler to this device is closed. > */ > - misc_deregister(&drvdata->miscdev); > + coresight_cdev_unregister(&drvdata->cdev); > coresight_unregister(drvdata->csdev); > } > > diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h > index b91ec7dde7bc..c37e2ed1cb9d 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc.h > +++ b/drivers/hwtracing/coresight/coresight-tmc.h > @@ -166,7 +166,7 @@ struct etr_buf { > * struct tmc_drvdata - specifics associated to an TMC component > * @base: memory mapped base address for this component. > * @csdev: component vitals needed by the framework. > - * @miscdev: specifics to handle "/dev/xyz.tmc" entry. > + * @cdev: specifics to handle "/dev/xyz.tmc" entry. > * @spinlock: only one at a time pls. > * @pid: Process ID of the process being monitored by the session > * that is using this component. > @@ -188,7 +188,7 @@ struct etr_buf { > struct tmc_drvdata { > void __iomem *base; > struct coresight_device *csdev; > - struct miscdevice miscdev; > + struct coresight_cdev cdev; > spinlock_t spinlock; > pid_t pid; > bool reading; > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > index 93a2922b7653..aebe8b8327ad 100644 > --- a/include/linux/coresight.h > +++ b/include/linux/coresight.h > @@ -9,6 +9,7 @@ > #include <linux/device.h> > #include <linux/io.h> > #include <linux/perf_event.h> > +#include <linux/cdev.h> > #include <linux/sched.h> > > /* Peripheral id registers (0xFD0-0xFEC) */ > @@ -252,6 +253,12 @@ struct coresight_device { > void *active_cscfg_ctxt; > }; > > +struct coresight_cdev { > + dev_t devno; > + struct class *class; > + struct cdev cdev; > +}; > + > /* > * coresight_dev_list - Mapping for devices to "name" index for device > * names. > @@ -506,6 +513,10 @@ void coresight_relaxed_write64(struct coresight_device *csdev, > u64 val, u32 offset); > void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset); > > +int coresight_cdev_register(struct coresight_cdev *cdev, const struct file_operations *fops, > + const char *name); > + > +int coresight_cdev_unregister(struct coresight_cdev *cdev); > #else > static inline struct coresight_device * > coresight_register(struct coresight_desc *desc) { return NULL; } > @@ -581,6 +592,18 @@ static inline void coresight_write64(struct coresight_device *csdev, u64 val, u3 > { > } > > +static inline int coresight_cdev_register(struct coresight_cdev *cdev, > + const struct file_operations *fops, > + const char *name) > +{ > + return 0; > +} > + > +static inline int coresight_cdev_unregister(struct coresight_cdev *cdev) > +{ > + return 0; > +} > + > #endif /* IS_ENABLED(CONFIG_CORESIGHT) */ > > extern int coresight_get_cpu(struct device *dev); > -- > 2.27.0 >

3 years, 5 months

Re: [PATCH 19/22] perf cpumap: Move 'has' function to libperf

by Mathieu Poirier

On Tue, Dec 07, 2021 at 06:46:04PM -0800, Ian Rogers wrote: > Make the cpu map argument const for consistency with the rest of the > API. Modify cpu_map__idx accordingly. > > Signed-off-by: Ian Rogers <irogers(a)google.com> > --- > tools/lib/perf/cpumap.c | 7 ++++++- > tools/lib/perf/include/internal/cpumap.h | 2 +- > tools/lib/perf/include/perf/cpumap.h | 1 + > tools/perf/arch/arm/util/cs-etm.c | 16 ++++++++-------- For the coresight part: Reviewed-by: Mathieu Poirier <mathieu.poirier(a)linaro.org> > tools/perf/builtin-sched.c | 6 +++--- > tools/perf/tests/topology.c | 2 +- > tools/perf/util/cpumap.c | 5 ----- > tools/perf/util/cpumap.h | 2 -- > tools/perf/util/cputopo.c | 2 +- > 9 files changed, 21 insertions(+), 22 deletions(-) > > diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c > index adaad3dddf6e..3c36a06771af 100644 > --- a/tools/lib/perf/cpumap.c > +++ b/tools/lib/perf/cpumap.c > @@ -268,7 +268,7 @@ bool perf_cpu_map__empty(const struct perf_cpu_map *map) > return map ? map->map[0] == -1 : true; > } > > -int perf_cpu_map__idx(struct perf_cpu_map *cpus, int cpu) > +int perf_cpu_map__idx(const struct perf_cpu_map *cpus, int cpu) > { > int low = 0, high = cpus->nr; > > @@ -288,6 +288,11 @@ int perf_cpu_map__idx(struct perf_cpu_map *cpus, int cpu) > return -1; > } > > +bool perf_cpu_map__has(const struct perf_cpu_map *cpus, int cpu) > +{ > + return perf_cpu_map__idx(cpus, cpu) != -1; > +} > + > int perf_cpu_map__max(struct perf_cpu_map *map) > { > // cpu_map__trim_new() qsort()s it, cpu_map__default_new() sorts it as well. > diff --git a/tools/lib/perf/include/internal/cpumap.h b/tools/lib/perf/include/internal/cpumap.h > index 1c1726f4a04e..6a0ff0abcbc4 100644 > --- a/tools/lib/perf/include/internal/cpumap.h > +++ b/tools/lib/perf/include/internal/cpumap.h > @@ -21,6 +21,6 @@ struct perf_cpu_map { > #define MAX_NR_CPUS 2048 > #endif > > -int perf_cpu_map__idx(struct perf_cpu_map *cpus, int cpu); > +int perf_cpu_map__idx(const struct perf_cpu_map *cpus, int cpu); > > #endif /* __LIBPERF_INTERNAL_CPUMAP_H */ > diff --git a/tools/lib/perf/include/perf/cpumap.h b/tools/lib/perf/include/perf/cpumap.h > index 7c27766ea0bf..3f1c0afa3ccd 100644 > --- a/tools/lib/perf/include/perf/cpumap.h > +++ b/tools/lib/perf/include/perf/cpumap.h > @@ -20,6 +20,7 @@ LIBPERF_API int perf_cpu_map__cpu(const struct perf_cpu_map *cpus, int idx); > LIBPERF_API int perf_cpu_map__nr(const struct perf_cpu_map *cpus); > LIBPERF_API bool perf_cpu_map__empty(const struct perf_cpu_map *map); > LIBPERF_API int perf_cpu_map__max(struct perf_cpu_map *map); > +LIBPERF_API bool perf_cpu_map__has(const struct perf_cpu_map *map, int cpu); > > #define perf_cpu_map__for_each_cpu(cpu, idx, cpus) \ > for ((idx) = 0, (cpu) = perf_cpu_map__cpu(cpus, idx); \ > diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c > index 293a23bf8be3..76c66780617c 100644 > --- a/tools/perf/arch/arm/util/cs-etm.c > +++ b/tools/perf/arch/arm/util/cs-etm.c > @@ -204,8 +204,8 @@ static int cs_etm_set_option(struct auxtrace_record *itr, > > /* Set option of each CPU we have */ > for (i = 0; i < cpu__max_cpu(); i++) { > - if (!cpu_map__has(event_cpus, i) || > - !cpu_map__has(online_cpus, i)) > + if (!perf_cpu_map__has(event_cpus, i) || > + !perf_cpu_map__has(online_cpus, i)) > continue; > > if (option & BIT(ETM_OPT_CTXTID)) { > @@ -542,8 +542,8 @@ cs_etm_info_priv_size(struct auxtrace_record *itr __maybe_unused, > /* cpu map is not empty, we have specific CPUs to work with */ > if (!perf_cpu_map__empty(event_cpus)) { > for (i = 0; i < cpu__max_cpu(); i++) { > - if (!cpu_map__has(event_cpus, i) || > - !cpu_map__has(online_cpus, i)) > + if (!perf_cpu_map__has(event_cpus, i) || > + !perf_cpu_map__has(online_cpus, i)) > continue; > > if (cs_etm_is_ete(itr, i)) > @@ -556,7 +556,7 @@ cs_etm_info_priv_size(struct auxtrace_record *itr __maybe_unused, > } else { > /* get configuration for all CPUs in the system */ > for (i = 0; i < cpu__max_cpu(); i++) { > - if (!cpu_map__has(online_cpus, i)) > + if (!perf_cpu_map__has(online_cpus, i)) > continue; > > if (cs_etm_is_ete(itr, i)) > @@ -741,8 +741,8 @@ static int cs_etm_info_fill(struct auxtrace_record *itr, > } else { > /* Make sure all specified CPUs are online */ > for (i = 0; i < perf_cpu_map__nr(event_cpus); i++) { > - if (cpu_map__has(event_cpus, i) && > - !cpu_map__has(online_cpus, i)) > + if (perf_cpu_map__has(event_cpus, i) && > + !perf_cpu_map__has(online_cpus, i)) > return -EINVAL; > } > > @@ -763,7 +763,7 @@ static int cs_etm_info_fill(struct auxtrace_record *itr, > offset = CS_ETM_SNAPSHOT + 1; > > for (i = 0; i < cpu__max_cpu() && offset < priv_size; i++) > - if (cpu_map__has(cpu_map, i)) > + if (perf_cpu_map__has(cpu_map, i)) > cs_etm_get_metadata(i, &offset, itr, info); > > perf_cpu_map__put(online_cpus); > diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c > index 4527f632ebe4..9da1da4749c9 100644 > --- a/tools/perf/builtin-sched.c > +++ b/tools/perf/builtin-sched.c > @@ -1617,10 +1617,10 @@ static int map_switch_event(struct perf_sched *sched, struct evsel *evsel, > if (curr_thread && thread__has_color(curr_thread)) > pid_color = COLOR_PIDS; > > - if (sched->map.cpus && !cpu_map__has(sched->map.cpus, cpu)) > + if (sched->map.cpus && !perf_cpu_map__has(sched->map.cpus, cpu)) > continue; > > - if (sched->map.color_cpus && cpu_map__has(sched->map.color_cpus, cpu)) > + if (sched->map.color_cpus && perf_cpu_map__has(sched->map.color_cpus, cpu)) > cpu_color = COLOR_CPUS; > > if (cpu != this_cpu) > @@ -1639,7 +1639,7 @@ static int map_switch_event(struct perf_sched *sched, struct evsel *evsel, > color_fprintf(stdout, color, " "); > } > > - if (sched->map.cpus && !cpu_map__has(sched->map.cpus, this_cpu)) > + if (sched->map.cpus && !perf_cpu_map__has(sched->map.cpus, this_cpu)) > goto out; > > timestamp__scnprintf_usec(timestamp, stimestamp, sizeof(stimestamp)); > diff --git a/tools/perf/tests/topology.c b/tools/perf/tests/topology.c > index 0cb7b015b4b9..cb29ea7ec409 100644 > --- a/tools/perf/tests/topology.c > +++ b/tools/perf/tests/topology.c > @@ -112,7 +112,7 @@ static int check_cpu_topology(char *path, struct perf_cpu_map *map) > TEST_ASSERT_VAL("Session header CPU map not set", session->header.env.cpu); > > for (i = 0; i < session->header.env.nr_cpus_avail; i++) { > - if (!cpu_map__has(map, i)) > + if (!perf_cpu_map__has(map, i)) > continue; > pr_debug("CPU %d, core %d, socket %d\n", i, > session->header.env.cpu[i].core_id, > diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c > index 0e325559c33c..8a72ee996722 100644 > --- a/tools/perf/util/cpumap.c > +++ b/tools/perf/util/cpumap.c > @@ -463,11 +463,6 @@ int cpu__setup_cpunode_map(void) > return 0; > } > > -bool cpu_map__has(struct perf_cpu_map *cpus, int cpu) > -{ > - return perf_cpu_map__idx(cpus, cpu) != -1; > -} > - > size_t cpu_map__snprint(struct perf_cpu_map *map, char *buf, size_t size) > { > int i, cpu, start = -1; > diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h > index 15043e764fa6..832fc53f3c11 100644 > --- a/tools/perf/util/cpumap.h > +++ b/tools/perf/util/cpumap.h > @@ -78,8 +78,6 @@ struct cpu_aggr_map *cpu_aggr_map__new(const struct perf_cpu_map *cpus, > aggr_cpu_id_get_t f, > void *data); > > -bool cpu_map__has(struct perf_cpu_map *cpus, int cpu); > - > bool aggr_cpu_id__equal(const struct aggr_cpu_id *a, const struct aggr_cpu_id *b); > bool aggr_cpu_id__is_empty(const struct aggr_cpu_id *a); > struct aggr_cpu_id aggr_cpu_id__empty(void); > diff --git a/tools/perf/util/cputopo.c b/tools/perf/util/cputopo.c > index 51b429c86f98..8affb37d90e7 100644 > --- a/tools/perf/util/cputopo.c > +++ b/tools/perf/util/cputopo.c > @@ -218,7 +218,7 @@ struct cpu_topology *cpu_topology__new(void) > tp->core_cpus_list = addr; > > for (i = 0; i < nr; i++) { > - if (!cpu_map__has(map, i)) > + if (!perf_cpu_map__has(map, i)) > continue; > > ret = build_cpu_topology(tp, i); > -- > 2.34.1.400.ga245620fadb-goog >

3 years, 5 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight