Re: [PATCH] perf/core: Replace 1 with true

Re: [PATCH] perf/core: Replace 1 with true

[Yejune Deng]

Can my submission be accepted ? Do I need to do something, or wait for news?

On Wed, Dec 9, 2020 at 7:11 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Wed, Dec 09, 2020 at 11:37:19AM +0100, Peter Zijlstra wrote:
> > On Wed, Dec 09, 2020 at 06:09:01PM +0800, Yejune Deng wrote:
> > > In task_css_check(), the third parameter 'true' looks more readable.
> > >
> > > Signed-off-by: Yejune Deng <yejune.deng@gmail.com>
> > > ---
> > >  kernel/events/core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > > index d2f3ca7..0700022 100644
> > > --- a/kernel/events/core.c
> > > +++ b/kernel/events/core.c
> > > @@ -7265,7 +7265,7 @@ void perf_prepare_sample(struct perf_event_header *header,
> > >             struct cgroup *cgrp;
> > >
> > >             /* protected by RCU */
> > > -           cgrp = task_css_check(current, perf_event_cgrp_id, 1)->cgroup;
> > > +           cgrp = task_css_check(current, perf_event_cgrp_id, true)->cgroup;
> >
> > it looks like 3 wasted bytes to me
>
> I disagree; I think it'd make sense to do this for consistency, given all the
> other callers pass true/false (including in the header where this is defined):
>
> | [mark@lakrids:~/src/linux]% git grep task_css_check
> | include/linux/cgroup.h: * task_css_check - obtain css for (task, subsys) w/ extra access conds
> | include/linux/cgroup.h:#define task_css_check(task, subsys_id, __c)                             \
> | include/linux/cgroup.h: * See task_css_check().
> | include/linux/cgroup.h: return task_css_check(task, subsys_id, false);
> | include/linux/cgroup.h: return task_css_check(task, subsys_id, true) ==
> | include/linux/perf_event.h:     return container_of(task_css_check(task, perf_event_cgrp_id,
> | kernel/cgroup/pids.c: * task_css_check(true) in pids_can_fork() and pids_cancel_fork() relies
> | kernel/cgroup/pids.c:           css = task_css_check(current, pids_cgrp_id, true);
> | kernel/cgroup/pids.c:           css = task_css_check(current, pids_cgrp_id, true);
> | kernel/events/core.c:           cgrp = task_css_check(current, perf_event_cgrp_id, 1)->cgroup;
> | kernel/sched/core.c:     * which is pointless here. Thus, we pass "true" to task_css_check()
> | kernel/sched/core.c:    tg = container_of(task_css_check(tsk, cpu_cgrp_id, true),
> | net/core/netclassid_cgroup.c:   return css_cls_state(task_css_check(p, net_cls_cgrp_id,
>
> ... but it'd be helpful if this could be cc'd to LKML
> (linux-kernel@vger.kernel.org), rather than just the maintainers off-list.
>
> ... or we could waste more than 3 bytes arguing. ;)
>
> Thanks,
> Mark.