Re: [PATCH v9 8/8] perf/amd/iommu: Enable support for multiple IOMMUs

[Suravee Suthikulpanit]

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@xxxxxxx>
> >  [......]
> > +	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