Messages in this thread | | | Date | Wed, 8 Feb 2017 20:33:40 +0100 | From | Borislav Petkov <> | Subject | Re: [PATCH v9 8/8] perf/amd/iommu: Enable support for multiple IOMMUs |
| |
On Tue, Feb 07, 2017 at 02:40:36AM -0600, Suravee Suthikulpanit wrote: > From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > > Add multi-IOMMU support for perf by exposing an AMD IOMMU PMU > for each IOMMU found in the system via: > > /bus/event_source/devices/amd_iommu_x > > where x is the IOMMU index. This allows users to specify > different events to be programmed onto performance counters > of each IOMMU. > > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Borislav Petkov <bp@alien8.de> > Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > ---
...
> @@ -414,46 +417,46 @@ static __init void amd_iommu_pc_exit(void) > }; > > static __init int > -_init_perf_amd_iommu(struct perf_amd_iommu *perf_iommu, char *name) > +init_one_perf_amd_iommu(struct perf_amd_iommu *perf_iommu, unsigned int idx)
It's a static function - just call it "init_one_iommu".
> { > int ret; > > raw_spin_lock_init(&perf_iommu->lock); > > - /* Init cpumask attributes to only core 0 */ > - cpumask_set_cpu(0, &iommu_cpumask); > - > - perf_iommu->max_banks = amd_iommu_pc_get_max_banks(0); > - perf_iommu->max_counters = amd_iommu_pc_get_max_counters(0); > + perf_iommu->iommu = get_amd_iommu(idx);
This function can return NULL - you need to handle that here otherwise it will blow up later.
> + 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.
perf_iommu->pmu = pmu;
> - ret = perf_pmu_register(&perf_iommu->pmu, name, -1); > + > + ret = perf_pmu_register(&perf_iommu->pmu, perf_iommu->name, -1); > if (ret) > pr_err("Error initializing AMD IOMMU perf counters.\n"); > else > - pr_info("perf: amd_iommu: Detected. (%d banks, %d counters/bank)\n", > - amd_iommu_pc_get_max_banks(0), > - amd_iommu_pc_get_max_counters(0)); > + pr_info("Detected AMD IOMMU #%d (%d banks, %d counters/bank)\n", > + idx, amd_iommu_pc_get_max_banks(idx),
perf_iommu->max_banks
> + amd_iommu_pc_get_max_counters(idx));
perf_iommu->max_counters
You already read them above.
> return ret; > } > > -static struct perf_amd_iommu __perf_iommu = { > - .pmu = { > - .task_ctx_nr = perf_invalid_context, > - .event_init = perf_iommu_event_init, > - .add = perf_iommu_add, > - .del = perf_iommu_del, > - .start = perf_iommu_start, > - .stop = perf_iommu_stop, > - .read = perf_iommu_read, > - }, > -}; > - > static __init int amd_iommu_pc_init(void) > { > int ret; > + unsigned int i; > + > + /* Init cpumask attributes to only core 0 */ > + cpumask_set_cpu(0, &iommu_cpumask);
This belongs at the end of the function, once everything has been initialized successfully.
> /* Make sure the IOMMU PC resource is available */ > if (!amd_iommu_pc_supported())
> @@ -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.
-- Regards/Gruss, Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
| |