Re: [RESEND PATCH V3] perf/x86: Consider pinned events for group validation

[Peter Zijlstra <peterz@infradead.org>]

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.