Messages in this thread | | | Date | Fri, 17 Jan 2020 10:13:41 +0100 | From | Peter Zijlstra <> | Subject | Re: [RESEND PATCH V3] perf/x86: Consider pinned events for group validation |
| |
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.
| |