Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1557672 > unrolled thread

Re: [PATCH v7 7/7] perf/amd/iommu: Enable support for multiple IOMMUs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v7 7/7] perf/amd/iommu: Enable support for multiple IOMMUs Borislav Petkov <bp@alien8.de> - 2017-01-12 19:00 +0100
    Re: [PATCH v7 7/7] perf/amd/iommu: Enable support for multiple IOMMUs Borislav Petkov <bp@alien8.de> - 2017-01-13 12:50 +0100
      Re: [PATCH v7 7/7] perf/amd/iommu: Enable support for multiple IOMMUs Borislav Petkov <bp@alien8.de> - 2017-01-14 11:40 +0100

#1557672 — Re: [PATCH v7 7/7] perf/amd/iommu: Enable support for multiple IOMMUs

On Mon, Jan 09, 2017 at 09:33:47PM -0600, Suravee Suthikulpanit wrote:
> This patch adds multi-IOMMU support for perf by exposing
> an AMD IOMMU PMU for each IOMMU found in the system via:
> 
>   /sys/device/amd_iommu_x     /* where x is the IOMMU index. */

Straight into the top-level devices hierarchy?

Why not add a proper hierarchy to

/sys/devices/system/iommu/ ...

?

Jörg, don't you have a sane iommu hierarchy already in sysfs?

> This allows users to specify different events to be programed
> 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>
> ---
>  arch/x86/events/amd/iommu.c | 119 ++++++++++++++++++++++++--------------------
>  1 file changed, 64 insertions(+), 55 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index 2403c78..5fd97b5 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -35,10 +35,13 @@
>  #define _GET_PASID_MASK(ev) ((ev->hw.extra_reg.config >> 16) & 0xFFFFULL)
>  #define _GET_DOMID_MASK(ev) ((ev->hw.extra_reg.config >> 32) & 0xFFFFULL)
>  
> -static struct perf_amd_iommu __perf_iommu;
> +#define PERF_AMD_IOMMU_NAME_SZ 16
>  
>  struct perf_amd_iommu {
> +	struct list_head list;
>  	struct pmu pmu;
> +	uint idx;

This uint thing has crept in:

$ git grep "uint " arch/x86/
arch/x86/crypto/crc32-pclmul_asm.S:101: *      uint crc32_pclmul_le_16(unsigned char const *buffer,
arch/x86/crypto/crc32-pclmul_asm.S:102: *                            size_t len, uint crc32)
arch/x86/events/amd/iommu.h:32:extern u8 amd_iommu_pc_get_max_banks(uint idx);
arch/x86/events/amd/iommu.h:34:extern u8 amd_iommu_pc_get_max_counters(uint idx);
arch/x86/events/amd/iommu.h:36:extern int amd_iommu_pc_set_reg(uint idx, u16 devid, u8 bank, u8 cntr,
arch/x86/events/amd/iommu.h:39:extern int amd_iommu_pc_set_counter(uint idx, u8 bank, u8 cntr, u64 *value);
arch/x86/events/amd/iommu.h:41:extern int amd_iommu_pc_get_counter(uint idx, u8 bank, u8 cntr, u64 *value);

"unsigned int" please.

> +	char name[PERF_AMD_IOMMU_NAME_SZ];
>  	u8 max_banks;
>  	u8 max_counters;
>  	u64 cntr_assign_mask;
> @@ -46,6 +49,8 @@ struct perf_amd_iommu {
>  	const struct attribute_group *attr_groups[4];
>  };
>  
> +LIST_HEAD(perf_amd_iommu_list);

static

> +
>  #define format_group	attr_groups[0]
>  #define cpumask_group	attr_groups[1]
>  #define events_group	attr_groups[2]
> @@ -204,8 +209,7 @@ static int clear_avail_iommu_bnk_cntr(struct perf_amd_iommu *perf_iommu,
>  static int perf_iommu_event_init(struct perf_event *event)
>  {
>  	struct hw_perf_event *hwc = &event->hw;
> -	struct perf_amd_iommu *perf_iommu;
> -	u64 config, config1;
> +	struct perf_amd_iommu *pi = container_of(event->pmu, struct perf_amd_iommu, pmu);

Do this deref when you actually need it:

	struct perf_amd_iommu *pi;

because you might exit earlier and end up doing it for nothing.

>  
>  	/* test the event attr type check for PMU enumeration */
>  	if (event->attr.type != event->pmu->type)
> @@ -227,27 +231,17 @@ static int perf_iommu_event_init(struct perf_event *event)
>  	if (event->cpu < 0)
>  		return -EINVAL;
>  
> -	perf_iommu = &__perf_iommu;
> -
> -	if (event->pmu != &perf_iommu->pmu)
> -		return -ENOENT;
> -
> -	if (perf_iommu) {
> -		config = event->attr.config;
> -		config1 = event->attr.config1;
> -	} else {
> -		return -EINVAL;
> -	}
> -
>  	/* update the hw_perf_event struct with the iommu config data */
> -	hwc->config = config;
> -	hwc->extra_reg.config = config1;

---> here:

	pi = container_of(event->pmu, struct perf_amd_iommu, pmu);

> +	hwc->idx = pi->idx;
> +	hwc->config = event->attr.config;
> +	hwc->extra_reg.config = event->attr.config1;

Align vertically on the = sign.

>  
>  	return 0;
>  }
>  
>  static void perf_iommu_enable_event(struct perf_event *ev)
>  {
> +	struct hw_perf_event *hwc = &ev->hw;
>  	u8 csource = _GET_CSOURCE(ev);
>  	u16 devid = _GET_DEVID(ev);
>  	u8 bank = _GET_BANK(ev);

...

>  static void perf_iommu_disable_event(struct perf_event *event)
>  {
> +	struct hw_perf_event *hwc = &event->hw;
>  	u64 reg = 0ULL;
>  
> -	amd_iommu_pc_set_reg(0, _GET_DEVID(event), _GET_BANK(event),
> +	amd_iommu_pc_set_reg(hwc->idx, _GET_DEVID(event), _GET_BANK(event),
>  			     _GET_CNTR(event), IOMMU_PC_COUNTER_SRC_REG, &reg);
>  }
>  
> @@ -301,7 +296,7 @@ static void perf_iommu_start(struct perf_event *event, int flags)
>  
>  	val = local64_read(&hwc->prev_count);
>  
> -	amd_iommu_pc_set_counter(0, _GET_BANK(event), _GET_CNTR(event), &val);
> +	amd_iommu_pc_set_counter(hwc->idx, _GET_BANK(event), _GET_CNTR(event), &val);
>  enable:
>  	perf_iommu_enable_event(event);
>  	perf_event_update_userpage(event);
> @@ -314,7 +309,7 @@ static void perf_iommu_read(struct perf_event *event)
>  	s64 delta;
>  	struct hw_perf_event *hwc = &event->hw;
>  
> -	if (amd_iommu_pc_get_counter(0, _GET_BANK(event), _GET_CNTR(event), &cnt))
> +	if (amd_iommu_pc_get_counter(hwc->idx, _GET_BANK(event), _GET_CNTR(event), &cnt))
>  		return;
>  
>  	/* IOMMU pc counter register is only 48 bits */
> @@ -417,14 +412,20 @@ static __init int _init_events_attrs(struct perf_amd_iommu *perf_iommu)
>  
>  static __init void amd_iommu_pc_exit(void)
>  {
> -	if (__perf_iommu.events_group != NULL) {
> -		kfree(__perf_iommu.events_group);
> -		__perf_iommu.events_group = NULL;
> +	struct perf_amd_iommu *pi, *next;
> +
> +	list_for_each_entry_safe(pi, next, &perf_amd_iommu_list, list) {
> +		list_del(&pi->list);
> +
> +		kfree(pi->events_group);
> +		pi->events_group = NULL;

Why that assignment if you're going to free it in the next line?

> +
> +		kfree(pi);
>  	}
>  }
>  
> -static __init int _init_perf_amd_iommu(
> -	struct perf_amd_iommu *perf_iommu, char *name)
> +static __init int
> +init_one_perf_amd_iommu(struct perf_amd_iommu *perf_iommu, uint idx)
>  {
>  	int ret;
>  
> @@ -441,54 +442,62 @@ static __init int _init_perf_amd_iommu(
>  	if (_init_events_attrs(perf_iommu) != 0)
>  		pr_err("Only support raw events.\n");


Here's also

        /* Init cpumask attributes to only core 0 */
        cpumask_set_cpu(0, &iommu_cpumask);

is that still correct for all IOMMUs?

>  
> -	perf_iommu->max_banks = amd_iommu_pc_get_max_banks(0);
> -	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(0);
> +	snprintf(perf_iommu->name, PERF_AMD_IOMMU_NAME_SZ, "amd_iommu_%u", idx);

Do this ^^

> +	perf_iommu->idx = idx;
> +	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;


... here, after the test has passed.

>  	/* Init null attributes */
>  	perf_iommu->null_group = NULL;

No need for this - you're getting zeroed memory from kzalloc().

> +	/* Setting up PMU */
> +	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 = perf_iommu->attr_groups;

Align vertically on the =

> -	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");
>  		amd_iommu_pc_exit();
>  	} 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 %s, w/ %d banks, %d counters/bank\n",

I'd prefer:

		"Detected AMD IOMMU #%d (%d banks, %d counters/bank)"

where the "#%d" is idx.

>  static __init int amd_iommu_pc_init(void)
>  {
> +	uint i;
> +
>  	/* Make sure the IOMMU PC resource is available */
>  	if (!amd_iommu_pc_supported())
>  		return -ENODEV;
>  
> -	_init_perf_amd_iommu(&__perf_iommu, "amd_iommu");
> +	for (i = 0 ; i < amd_iommu_get_num_iommus(); i++) {
> +		int ret;
> +		struct perf_amd_iommu *pi;
> +
> +		pi = kzalloc(sizeof(struct perf_amd_iommu), GFP_KERNEL);
> +		if (!pi)
> +			return -ENOMEM;
> +
> +		ret = init_one_perf_amd_iommu(pi, i);
> +		if (ret)
> +			return ret;

You're leaking here memory if some IOMMUs fail and that function returns
-EINVAL from the max_banks and counters test above.

So you need to unwind all that setup init_one_perf_amd_iommu() did
for the iommus from 0 up to i - 1; IOW; you need to move the call to
amd_iommu_pc_exit() here and not in the init function.

That'll be all for now. :-)

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [next] | [standalone]

#1558283

On Fri, Jan 13, 2017 at 05:24:01PM +0700, Suravee Suthikulpanit wrote:
> IIUC, Perf tools looks at the /sys/devices/xxxxx to identify
> availalble PMUs. Are you planning to have perf tools look at
> /sys/devices/system/iommu/xxx instead?

No, I'm planning to understand what do you mean exactly. Because the
PMUs are, as I'm being pointed to, here:

#define EVENT_SOURCE_DEVICE_PATH "/bus/event_source/devices/"

Do you mean that, per chance?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1558942

On Sat, Jan 14, 2017 at 09:58:29AM +0700, Suravee Suthikulpanit wrote:
> I'll update the commit log to mention
> /bus/event_source/devices/amd_iommu_X instead.

Yes, so /sys/devices/ contains *all* devices on the system and the iommu
ones appear there too but since in the commit message you were talking
about PMUs but pointing at generic devices, it did confuse me.

But you're doing perf_pmu_register() anyway so the hierarchy will be fine.

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web