On 23/07/2024 16:30, Lucas De Marchi wrote:
On Tue, Jul 23, 2024 at 09:03:25AM GMT, Tvrtko Ursulin wrote:
On 22/07/2024 22:06, Lucas De Marchi wrote:
Instead of calling perf_pmu_unregister() when unbinding, defer that to
the destruction of i915 object. Since perf itself holds a reference in
the event, this only happens when all events are gone, which guarantees
i915 is not unregistering the pmu with live events.
Previously, running the following sequence would crash the system after
~2 tries:
1) bind device to i915
2) wait events to show up on sysfs
3) start perf stat -I 1000 -e i915/rcs0-busy/
4) unbind driver
5) kill perf
Most of the time this crashes in perf_pmu_disable() while accessing the
percpu pmu_disable_count. This happens because perf_pmu_unregister()
destroys it with free_percpu(pmu->pmu_disable_count).
With a lazy unbind, the pmu is only unregistered after (5) as opposed to
after (4). The downside is that if a new bind operation is attempted for
the same device/driver without killing the perf process, i915 will fail
to register the pmu (but still load successfully). This seems better
than completely crashing the system.
So effectively allows unbind to succeed without fully unbinding the
driver from the device? That sounds like a significant drawback and if
so, I wonder if a more complicated solution wouldn't be better after
all. Or is there precedence for allowing userspace keeping their paws
on unbound devices in this way?
keeping the resources alive but "unplunged" while the hardware
disappeared is a common thing to do... it's the whole point of the
drmm-managed resource for example. If you bind the driver and then
unbind it while userspace is holding a ref, next time you try to bind it
will come up with a different card number. A similar thing that could be
done is to adjust the name of the event - currently we add the mangled
pci slot.
Yes.. but what my point was this from your commit message:
"""
The downside is that if a new bind operation is attempted for
the same device/driver without killing the perf process, i915 will fail
to register the pmu (but still load successfully).
"""
So the subsequent bind does not "come up with a different card number".
Statement is it will come up with an error if we look at the PMU subset
of functionality. I was wondering if there was precedent for that kind
of situation.
Mangling the PMU driver name probably also wouldn't be great.
That said, I agree a better approach would be to allow
perf_pmu_unregister() to do its job even when there are open events. On
top of that (or as a way to help achieve that), make perf core replace
the callbacks with stubs when pmu is unregistered - that would even kill
the need for i915's checks on pmu->closed (and fix the lack thereof in
other drivers).
It can be a can of worms though and may be pushed back by perf core
maintainers, so it'd be good have their feedback.
Yeah definitely would be essential.
Regards,
Tvrtko
Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_pmu.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c
b/drivers/gpu/drm/i915/i915_pmu.c
index 8708f905f4f4..df53a8fe53ec 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -1158,18 +1158,21 @@ static void free_pmu(struct drm_device *dev,
void *res)
struct i915_pmu *pmu = res;
struct drm_i915_private *i915 = pmu_to_i915(pmu);
+ perf_pmu_unregister(&pmu->base);
free_event_attributes(pmu);
kfree(pmu->base.attr_groups);
if (IS_DGFX(i915))
kfree(pmu->name);
+
+ /*
+ * Make sure all currently running (but shortcut on pmu->closed)
are
+ * gone before proceeding with free'ing the pmu object embedded
in i915.
+ */
+ synchronize_rcu();
}
static int i915_pmu_cpu_online(unsigned int cpu, struct hlist_node
*node)
{
- struct i915_pmu *pmu = hlist_entry_safe(node, typeof(*pmu),
cpuhp.node);
-
- GEM_BUG_ON(!pmu->base.event_init);
-
/* Select the first online CPU as a designated reader. */
if (cpumask_empty(&i915_pmu_cpumask))
cpumask_set_cpu(cpu, &i915_pmu_cpumask);
@@ -1182,8 +1185,6 @@ static int i915_pmu_cpu_offline(unsigned int
cpu, struct hlist_node *node)
struct i915_pmu *pmu = hlist_entry_safe(node, typeof(*pmu),
cpuhp.node);
unsigned int target = i915_pmu_target_cpu;
- GEM_BUG_ON(!pmu->base.event_init);
-
/*
* Unregistering an instance generates a CPU offline event which
we must
* ignore to avoid incorrectly modifying the shared
i915_pmu_cpumask.
@@ -1337,21 +1338,14 @@ void i915_pmu_unregister(struct
drm_i915_private *i915)
{
struct i915_pmu *pmu = &i915->pmu;
- if (!pmu->base.event_init)
- return;
-
/*
- * "Disconnect" the PMU callbacks - since all are atomic
synchronize_rcu
- * ensures all currently executing ones will have exited before we
- * proceed with unregistration.
+ * "Disconnect" the PMU callbacks - unregistering the pmu will
be done
+ * later when all currently open events are gone
*/
pmu->closed = true;
- synchronize_rcu();
hrtimer_cancel(&pmu->timer);
-
i915_pmu_unregister_cpuhp_state(pmu);
- perf_pmu_unregister(&pmu->base);
pmu->base.event_init = NULL;
}
