[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <upndskqxwe5qwfho4ta4v42mdsfggi4r6vpdjjc3welk6g7dzf@emnor45tvhqf>
Date: Fri, 11 Oct 2024 18:03:07 -0500
From: Lucas De Marchi <lucas.demarchi@...el.com>
To: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@...el.com>
CC: <linux-kernel@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>, "Peter
Zijlstra" <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, "Arnaldo
Carvalho de Melo" <acme@...nel.org>, Ian Rogers <irogers@...gle.com>, "Tvrtko
Ursulin" <tvrtko.ursulin@...lia.com>
Subject: Re: [PATCH 0/5] perf: Fix pmu for drivers with bind/unbind
On Fri, Oct 11, 2024 at 03:21:18PM -0700, Umesh Nerlige Ramappa wrote:
>On Tue, Oct 08, 2024 at 01:34:56PM -0500, Lucas De Marchi wrote:
>>v2 of my attempt at fixing how i915 interacts with perf events.
>>
>>v1 - https://lore.kernel.org/all/20240722210648.80892-1-lucas.demarchi@intel.com/
>>
>>>From other people:
>>1) https://lore.kernel.org/lkml/20240115170120.662220-1-tvrtko.ursulin@linux.intel.com/
>>2) https://lore.kernel.org/all/20240213180302.47266-1-umesh.nerlige.ramappa@intel.com/
>>
>>WARNING: patches 1, 4 and 5 are NOT intended to be applied as is. More
>>on this below.
>>
>>This series basically builds on the idea of the first patch of my
>>previous series, but extends it in a way that
>>
>> a) the other patches are not needed (at least, not as is) and
>> b) driver can rebind just fine - no issues with the new call to
>> perf_pmu_register()
>
>I have 2 broad questions:
>
>(1) I am curious how (b) works. You seem to have a notion of instances
>of devices and then are you using the instance number to create the
>name used for the sysfs entry? Did I get that right?
humn... no. We just unregister the driver from pmu, so the name becomes
free for when the driver rebinds with the same event name.
>
>If so, should the application discover what the name is each time it
>is run? In the failure case that I am seeing, I am running an
>application that does not work when I rename the sysfs entry to
>something else.
>
>(2) Similar to Patch 1 of your v1 series where you modified _free_event:
>
>static void _free_event(struct perf_event *event)
>{
> struct module *module;
>...
> module = event->pmu->module;
>...
> if (event->destroy)
> event->destroy(event);
>...
> module_put(module);
>...
>}
>
>With the above code, the kref to i915->pmu can be taken from the below
>points in i915 code (just to point out the sequence):
>
>i915_pmu_register()
>{
> perf_pmu_register()
> drm_dev_get()
> kref_init()
>}
>
>i915_pmu_unregister()
>{
> kref_put(&ref, pmu_cleanup)
>}
>
>i915_pmu_event_init()
>{
> kref_get()
>}
>
>i915_pmu_event_destroy()
>{
> kref_put(&ref, pmu_cleanup)
>}
>
>void pmu_cleanup(struct kref *ref)
>{
> i915_pmu_unregister_cpuhp_state(pmu);
> perf_pmu_unregister(&pmu->base);
> pmu->base.event_init = NULL;
> kfree(pmu->base.attr_groups);
> if (!is_igp(i915))
> kfree(pmu->name);
> free_event_attributes(pmu);
> drm_dev_put(&i915->drm);
>}
>
>Would this work? Do you see any gaps that may need the ref counting
>code you added in perf?
well... I just posted the fixes for i915 on top of these patches :)
You may want to look at https://lore.kernel.org/intel-gfx/20241011225430.1219345-1-lucas.demarchi@intel.com/
It works for me on my machine with a DG2.
Lucas De Marchi
Powered by blists - more mailing lists