lkml.org 
[lkml]   [2017]   [Feb]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v9 8/8] perf/amd/iommu: Enable support for multiple IOMMUs
From
Date
Boris,

On 2/9/17 02:33, Borislav Petkov wrote:
> On Tue, Feb 07, 2017 at 02:40:36AM -0600, Suravee Suthikulpanit wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> [......]
>> + perf_iommu->max_banks = amd_iommu_pc_get_max_banks(idx);
>> + perf_iommu->max_counters = amd_iommu_pc_get_max_counters(idx);
>> if (!perf_iommu->max_banks || !perf_iommu->max_counters)
>> return -EINVAL;
>>
>> + snprintf(perf_iommu->name, PERF_AMD_IOMMU_NAME_SIZE, "amd_iommu_%u", idx);
>> +
>> + perf_iommu->pmu.event_init = perf_iommu_event_init;
>> + perf_iommu->pmu.add = perf_iommu_add;
>> + perf_iommu->pmu.del = perf_iommu_del;
>> + perf_iommu->pmu.start = perf_iommu_start;
>> + perf_iommu->pmu.stop = perf_iommu_stop;
>> + perf_iommu->pmu.read = perf_iommu_read;
>> + perf_iommu->pmu.task_ctx_nr = perf_invalid_context;
>> perf_iommu->pmu.attr_groups = amd_iommu_attr_groups;
>
> So you can define a static struct pmu in the driver and do struct
> assignment directly instead of writing them one-by-one.

I believe this is the same suggestion you have made in V8.
Here our previous discussion in V8:

On 2/7/17 08:42, Suravee Suthikulpanit wrote:
>
> On 1/23/17 02:55, Borislav Petkov wrote:
>> Because otherwise you're carrying a struct pmu in each struct
>> perf_amd_iommu which has identical contents.
>
> Actually, only the callbacks above will be identical on each pmu, but
> there are other parts of the structure which are different
> (e.g. pmu->name, pmu->type, etc.) Also, we need one pmu instance per
> IOMMU since each pmu reference will get assigned to perf_event, and
> also used to reference back to struct perf_amd_iommu. Note that each
> pmu can also have different events.

So, I still don't think we can have just one static PMU structure and
assign it to each IOMMU. Lemme know if I am missing your point here.


>> [...]
>> @@ -463,7 +466,24 @@ static __init int amd_iommu_pc_init(void)
>> if (ret)
>> return ret;
>>
>> - ret = _init_perf_amd_iommu(&__perf_iommu, "amd_iommu");
>> + for (i = 0 ; i < amd_iommu_get_num_iommus(); i++) {
>> + struct perf_amd_iommu *pi;
>> +
>> + pi = kzalloc(sizeof(struct perf_amd_iommu), GFP_KERNEL);
>> + if (!pi) {
>> + ret = -ENOMEM;
>> + break;
>> + }
>> +
>> + ret = init_one_perf_amd_iommu(pi, i);
>> + if (ret) {
>> + kfree(pi);
>> + break;
>
> What happens with the iommus that have been initialized successfully
> before this one fails? They remain in use?
>
> I think we need at least a warning saying here:
>
> pr_warning("Error initializing IOMMU %d ...")
>
> so that we at least know why some are missing.
>

The initialized ones should be functioning independently (as separate PMUs).
So, it should be alright to just leave them. I'll add the warning message
as you suggested.

Thanks,
Suravee

\
 
 \ /
  Last update: 2017-02-15 08:14    [W:0.070 / U:0.840 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site