Re: [PATCH v6 1/6] perf/cgroup: Reorder perf_cgroup_connect()

[Shuah Khan]

On 2/14/20 12:51 AM, Ian Rogers wrote:
> From: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>
> Move perf_cgroup_connect() after perf_event_alloc(), such that we can
> find/use the PMU's cpu context.

Can you elaborate on this usage? It will helpful to know how
this is used and what do we get from it. What were we missing
with the way it was done before?


> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> ---
>   kernel/events/core.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 3f1f77de7247..9bd2af954c54 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -10804,12 +10804,6 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
>   	if (!has_branch_stack(event))
>   		event->attr.branch_sample_type = 0;
>   
> -	if (cgroup_fd != -1) {
> -		err = perf_cgroup_connect(cgroup_fd, event, attr, group_leader);
> -		if (err)
> -			goto err_ns;
> -	}
> -
>   	pmu = perf_init_event(event);
>   	if (IS_ERR(pmu)) {
>   		err = PTR_ERR(pmu);
> @@ -10831,6 +10825,12 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
>   		goto err_pmu;

Is this patch based on linux-next or linux 5.6-rc1. I am finding code
path to be different in those. Also in 
https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git

goto err_ns makes more sense since if perf_init_event() doesn't return
valid pmu, especially since err_pmu tries to do a put pmu->module.

Something doesn't look right.


>   	}


>   
> +	if (cgroup_fd != -1) {
> +		err = perf_cgroup_connect(cgroup_fd, event, attr, group_leader);
> +		if (err)
> +			goto err_pmu;
> +	}
> +
>   	err = exclusive_event_init(event);
>   	if (err)
>   		goto err_pmu;
> @@ -10891,12 +10891,12 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
>   	exclusive_event_destroy(event);
>   
>   err_pmu:
> +	if (is_cgroup_event(event))
> +		perf_detach_cgroup(event);
>   	if (event->destroy)
>   		event->destroy(event);
>   	module_put(pmu->module);
>   err_ns:
> -	if (is_cgroup_event(event))
> -		perf_detach_cgroup(event);
>   	if (event->ns)
>   		put_pid_ns(event->ns);
>   	if (event->hw.target)

thanks,
-- Shuah