linux-kernel - Re: [PATCH RESEND v4] perf/core: Fix installing cgroup event into cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180208153648.GC12817@krava>
Date:   Thu, 8 Feb 2018 16:36:48 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     linxiulei@...il.com
Cc:     peterz@...radead.org, mingo@...hat.com, acme@...nel.org,
        alexander.shishkin@...ux.intel.com, tglx@...utronix.de,
        eranian@...il.com, torvalds@...ux-foundation.org,
        brendan.d.gregg@...il.com, linux-kernel@...r.kernel.org,
        linux-perf-users@...r.kernel.org, yang_oliver@...mail.com,
        jinli.zjl@...baba-inc.com,
        "leilei.lin" <leilei.lin@...baba-inc.com>
Subject: Re: [PATCH RESEND v4] perf/core: Fix installing cgroup event into cpu

On Thu, Feb 08, 2018 at 11:33:44AM +0800, linxiulei@...il.com wrote:
> From: "leilei.lin" <leilei.lin@...baba-inc.com>
> 
> Do not install cgroup event into the CPU context and schedule it
> if the cgroup is not running on this CPU
> 
> While there is no task of cgroup running specified CPU, current
> kernel still install cgroup event into CPU context that causes
> another cgroup event can't be installed into this CPU.
> 
> This patch prevent scheduling events at __perf_install_in_context()
> and installing events at list_update_cgroup_event() if cgroup isn't
> running on specified CPU.
> 
> Signed-off-by: leilei.lin <leilei.lin@...baba-inc.com>
> ---
>  v2: Set cpuctx->cgrp only if the same cgroup is running on this
>    CPU otherwise following events couldn't be activated immediately
>  v3: Enhance the comments and commit message
>  v4: Adjust to config
> 
>  kernel/events/core.c | 50 +++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 4df5b69..fd28d61 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -933,31 +933,41 @@ list_update_cgroup_event(struct perf_event *event,
>  {
>  	struct perf_cpu_context *cpuctx;
>  	struct list_head *cpuctx_entry;
> +	struct perf_cgroup *cgrp;
>  
>  	if (!is_cgroup_event(event))
>  		return;
>  
> -	if (add && ctx->nr_cgroups++)
> -		return;
> -	else if (!add && --ctx->nr_cgroups)
> -		return;

I might be missing something, but should this check stay on
the top regardles of the cgroup_is_descendant check below?

you could put NULL into cpuctx->cgrp on context with cgroup
event in the list

thanks,
jirka

>  	/*
>  	 * Because cgroup events are always per-cpu events,
>  	 * this will always be called from the right CPU.
>  	 */
>  	cpuctx = __get_cpu_context(ctx);
> -	cpuctx_entry = &cpuctx->cgrp_cpuctx_entry;
> -	/* cpuctx->cgrp is NULL unless a cgroup event is active in this CPU .*/
> -	if (add) {
> -		struct perf_cgroup *cgrp = perf_cgroup_from_task(current, ctx);
> +	cgrp = perf_cgroup_from_task(current, ctx);
>  
> -		list_add(cpuctx_entry, this_cpu_ptr(&cgrp_cpuctx_list));
> -		if (cgroup_is_descendant(cgrp->css.cgroup, event->cgrp->css.cgroup))
> +	/*
> +	 * if only the cgroup is running on this cpu,
> +	 * we put/remove this cgroup into cpu context.
> +	 * Or it would case mismatch in following cgroup
> +	 * events at event_filter_match()
> +	 */
> +	if (cgroup_is_descendant(cgrp->css.cgroup, event->cgrp->css.cgroup)) {
> +		if (add)
>  			cpuctx->cgrp = cgrp;
> -	} else {
> -		list_del(cpuctx_entry);
> -		cpuctx->cgrp = NULL;
> +		else
> +			cpuctx->cgrp = NULL;

SNIP