lkml.org 
[lkml]   [2021]   [Dec]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 01/22] libperf: Add comments to perf_cpu_map.
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
> >

\
 
 \ /
  Last update: 2021-12-08 16:11    [W:0.058 / U:0.944 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site