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
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
Hi Leon,
On Mon, Dec 06, 2021 at 08:49:01AM +0200, Leon Romanovsky wrote:
> On Sun, Dec 05, 2021 at 10:50:59PM +0800, Leo Yan wrote:
[...]
> > +static inline bool task_is_in_root_ns(struct task_struct *tsk)
>
> It is bad that this name doesn't reflect PID nature of this namespace.
> Won't it better to name it task_is_in_init_pid_ns()?
Yes, task_is_in_init_pid_ns() is more clear.
Will respin for this. Thank you for suggestion!
Leo
The kernel uses open code to check if a process is in root PID namespace
or not in several places.
Suggested by Suzuki, this patch set is to create a helper function
task_is_in_root_ns() so we can use it replace open code.
To test this patch set, I built Arm64 kernel with enabling all relevant
modules, and verified the kernel with CoreSight module on Arm64 Juno
board.
Leo Yan (7):
pid: Introduce helper task_is_in_root_ns()
coresight: etm3x: Use task_is_in_root_ns() to check PID namespace
coresight: etm4x: Use task_is_in_root_ns() to check PID namespace
connector/cn_proc: Use task_is_in_root_ns() to check PID namespace
coda: Use task_is_in_root_ns()
audit: Use task_is_in_root_ns()
taskstats: Use task_is_in_root_ns()
drivers/connector/cn_proc.c | 2 +-
drivers/hwtracing/coresight/coresight-etm3x-sysfs.c | 8 ++++----
drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 8 ++++----
fs/coda/inode.c | 2 +-
fs/coda/psdev.c | 2 +-
include/linux/pid_namespace.h | 5 +++++
kernel/audit.c | 2 +-
kernel/taskstats.c | 2 +-
8 files changed, 18 insertions(+), 13 deletions(-)
--
2.25.1