Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 17 Jan 2020 10:13:41 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     kan.liang@linux.intel.com
Cc:     mingo@redhat.com, acme@kernel.org, linux-kernel@vger.kernel.org,
        eranian@google.com, ak@linux.intel.com
Subject: Re: [RESEND PATCH V3] perf/x86: Consider pinned events for group
 validation
Message-ID: <20200117091341.GX2827@hirez.programming.kicks-ass.net>
References: <1579201225-178031-1-git-send-email-kan.liang@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1579201225-178031-1-git-send-email-kan.liang@linux.intel.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jan 16, 2020 at 11:00:25AM -0800, kan.liang@linux.intel.com wrote:
> @@ -2054,9 +2057,38 @@ static int validate_group(struct perf_event *event)
>  	if (n < 0)
>  		goto out;
>  
> +	/*
> +	 * Disable interrupts and preemption to prevent the events in this
> +	 * CPU's cpuc going away and getting freed.
> +	 */
> +	local_irq_save(flags);
> +
> +	/*
> +	 * The new group must can be scheduled together with current pinned
> +	 * events. Otherwise, it will never get a chance to be scheduled later.
> +	 *
> +	 * It won't catch all possible cases that cannot schedule, such as
> +	 * events pinned on CPU1, but the validation for a new CPU1 event
> +	 * running on other CPU. However, it's good enough to handle common
> +	 * cases like the global NMI watchdog.
> +	 */
> +	for (i = 0; i < cpuc->n_events; i++) {
> +		pinned_event = cpuc->event_list[i];
> +		if (WARN_ON_ONCE(!pinned_event))
> +			continue;
> +		if (!pinned_event->attr.pinned)
> +			continue;
> +		fake_cpuc->n_events = n;
> +		n = collect_events(fake_cpuc, pinned_event, false);
> +		if (n < 0)
> +			goto irq;
> +	}
> +

So I still completely hate this, because it makes the counter scheduling
more eratic.

It changes a situation where we only have false-positives (we allow
scheduling a group that might not ever get to run) into a situation
where we can have both false-positives and false-negatives.

Imagine the pinned event is for a currently running task; and that task
only runs sporadically. Then you can sometimes not create the group, but
mostly it'll work.

Yes, this is all very annoying, but I really don't see how this makes
anything any better.