Messages in this thread | | | Subject | Re: [PATCH v9 8/8] perf/amd/iommu: Enable support for multiple IOMMUs | From | Suravee Suthikulpanit <> | Date | Wed, 15 Feb 2017 14:13:29 +0700 |
| |
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
| |