linux-kernel - Re: [PATCH] perf/x86/intel/uncore: allocate pmu index for pci device dynamically

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 18 May 2018 10:52:25 -0400
From:   "Liang, Kan" <kan.liang@...ux.intel.com>
To:     Eric Ren <renzhen@...ux.alibaba.com>, x86@...nel.org
Cc:     tglx@...utronix.de, mingo@...hat.com, kan.liang@...el.com,
        ak@...ux.intel.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf/x86/intel/uncore: allocate pmu index for pci device
 dynamically



On 5/18/2018 1:23 AM, Eric Ren wrote:
> Some boxes/devices of uncore are exported as pcie devices. However,
> the box number is different on different micro-architecture. For
> example, the max memory channels for Broadwell is up to 8. However,
> there are only 2 channels for Broadwell-DE, 4 channels for Broadwell-EP,
> and 8 channels for Broadwell-EX.
> 
> The current code allocates pmu index statically so that on Broadwell-EP
> machine "perf list|grep uncore" shows discontinuous iMC number, which
> doesn't look nice:
> 
> Test on Broadwell-EP using "ls /sys/devices | grep -i imc":
> 
> Without this patch,
>      uncore_imc_0
>      uncore_imc_1
>      uncore_imc_4
>      uncore_imc_5
> 
> To maintain pmu index dynamically, move index allocation logic to
> uncore_pci_probe(). As a result, we can get continuous index of iMC
> devices under /sys/devices directory:
> 
> Applied this patch:,
>      uncore_imc_0
>      uncore_imc_1
>      uncore_imc_2
>      uncore_imc_3
> 
> Signed-off-by: Shanpei Chen <shanpeic@...ux.alibaba.com>
> Signed-off-by: Eric Ren <renzhen@...ux.alibaba.com>
> ---

I only have one small suggestion.
Except that, the patch looks good to me.
Reviewed-by: Kan Liang <kan.liang@...ux.intel.com>

>   arch/x86/events/intel/uncore.c | 7 ++++++-
>   arch/x86/events/intel/uncore.h | 1 +
>   2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index a7956fc..88d390e 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
> @@ -818,7 +818,9 @@ static int __init uncore_type_init(struct intel_uncore_type *type, bool setid)
>   
>   	for (i = 0; i < type->num_boxes; i++) {
>   		pmus[i].func_id	= setid ? i : -1;
> -		pmus[i].pmu_idx	= i;
> +		/* The pmu idx will be decided at probe for pci device. */
> +		if (setid)
> +			pmus[i].pmu_idx	= i;

I think we may use the same way as func_id to check the pmu_idx.
pmus[i].pmu_idx	= setid ? i : -1;

>   		pmus[i].type	= type;
>   		pmus[i].boxes	= kzalloc(size, GFP_KERNEL);
>   		if (!pmus[i].boxes)
> @@ -957,6 +959,9 @@ static int uncore_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id
>   	if (atomic_inc_return(&pmu->activeboxes) > 1)
>   		return 0;
>   
> +	/*  Count the real number of pmus for pci uncore device */
> +	pmu->pmu_idx = type->num_pmus++;
> +

	if (pmu->pmu_idx < 0)
		pmu->pmu_idx = type->num_pmus++;
	else
		WARN_ON_ONCE(1);

>   	/* First active box registers the pmu */
>   	ret = uncore_pmu_register(pmu);
>   	if (ret) {
> diff --git a/arch/x86/events/intel/uncore.h b/arch/x86/events/intel/uncore.h
> index 414dc7e..c4f54fb 100644
> --- a/arch/x86/events/intel/uncore.h
> +++ b/arch/x86/events/intel/uncore.h
> @@ -40,6 +40,7 @@ struct intel_uncore_type {
>   	const char *name;
>   	int num_counters;
>   	int num_boxes;
> +	int num_pmus; /* for pci uncore device */
>   	int perf_ctr_bits;
>   	int fixed_ctr_bits;
>   	unsigned perf_ctr;
>