| Message ID | 20211208024607.1784932-2-irogers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor perf cpumap | expand |
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? Generally developers don't always check comments where the struct is defined when the meaning could be judged intuitively Thanks, John
On Wed, Dec 8, 2021 at 4:06 AM John Garry <john.garry@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? 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 >
On Wed, Dec 8, 2021 at 6:34 AM Ian Rogers <irogers@google.com> wrote: > > On Wed, Dec 8, 2021 at 4:06 AM John Garry <john.garry@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? > > 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. A related follow up could be to switch perf_cpu_map to the more conventional cpu_set_t: https://man7.org/linux/man-pages/man3/CPU_SET.3.html However, that wouldn't allow the reference count to be alongside the contents. Thanks, Ian > Thanks, > Ian > > > Thanks, > > John > >
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@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 > >
On 10/12/2021 19:08, Arnaldo Carvalho de Melo wrote: >>>> +/** >>>> + * 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; I'd have /s/nr/len/, as it means the map length, as opposed to confusing nr meaning with number of cpus in the host or something else. And the new comment uses "Length" also. >>>> + /** 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" There are a lot of instances to change ... but I am all up for using consistent and well-meaning variable / argument names per type. > 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:
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[]; };
A particular observed problem is confusing the index with the CPU value, documentation should hopefully reduce this type of problem. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/lib/perf/include/internal/cpumap.h | 7 +++++++ 1 file changed, 7 insertions(+)