linux-kernel - Re: [PATCH 1/3] perf: Optimize perf_install_in

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: <20191023134444.GV1817@hirez.programming.kicks-ass.net>
Date:   Wed, 23 Oct 2019 15:44:44 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Alexander Shishkin <alexander.shishkin@...ux.intel.com>
Cc:     mingo@...nel.org, linux-kernel@...r.kernel.org, acme@...nel.org,
        mark.rutland@....com, jolsa@...hat.com, namhyung@...nel.org,
        andi@...stfloor.org, kan.liang@...ux.intel.com
Subject: Re: [PATCH 1/3] perf: Optimize perf_install_in_event()

On Wed, Oct 23, 2019 at 03:30:27PM +0300, Alexander Shishkin wrote:
> Peter Zijlstra <peterz@...radead.org> writes:
> 
> > +	/*
> > +	 * perf_event_attr::disabled events will not run and can be initialized
> > +	 * without IPI. Except when this is the first event for the context, in
> > +	 * that case we need the magic of the IPI to set ctx->is_active.
> > +	 *
> > +	 * The IOC_ENABLE that is sure to follow the creation of a disabled
> > +	 * event will issue the IPI and reprogram the hardware.
> > +	 */
> > +	if (__perf_effective_state(event) == PERF_EVENT_STATE_OFF && ctx->nr_events) {
> > +		raw_spin_lock_irq(&ctx->lock);
> > +		if (task && ctx->task == TASK_TOMBSTONE) {
> 
> Confused: isn't that redundant? If ctx->task reads TASK_TOMBSTONE, task
> is always !NULL,

The test is only relevant for task contexts, that's what the first
'task' clause tests for, then we need to check the ctx isn't dying,
which is the second clause 'ctx->task == TASK_TOMBSTONE'.

> afaict. And in any case, if a task context is going
> away, we shouldn't probably be adding events there. Or am I missing
> something?

Right, so in that case we exit early.