Re: [PATCH v3 0/5] arm64: SPE ACPI enablement

From: Will Deacon <will.deacon@arm.com>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: Hanjun Guo <guohanjun@huawei.com>,
	Jeremy Linton <jeremy.linton@arm.com>,
	linux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org,
	catalin.marinas@arm.com, rjw@rjwysocki.net, lenb@kernel.org,
	mark.rutland@arm.com, lorenzo.pieralisi@arm.com,
	linuxarm@huawei.com, john.garry@huawei.com,
	Hongbo Yao <yaohongbo@huawei.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>
Subject: Re: [PATCH v3 0/5] arm64: SPE ACPI enablement
Date: Mon, 13 May 2019 11:56:31 +0100	[thread overview]
Message-ID: <20190513105631.GB6711@fuggles.cambridge.arm.com> (raw)
In-Reply-To: <20190509141350.GF8239@e107155-lin>

Hi Sudeep,

On Thu, May 09, 2019 at 03:13:50PM +0100, Sudeep Holla wrote:
> On Thu, May 09, 2019 at 11:35:59AM +0100, Sudeep Holla wrote:
> > On Thu, May 09, 2019 at 10:28:11AM +0100, Will Deacon wrote:
> 
> [...]
> 
> > >
> > > Since SPE uses virtual addressing, we don't really care about the underlying
> > > page layout so there's no need to use higher-order allocations. I suppose we
> > > could theoretically map them at the pmd level in some cases, but ignoring
> > > them should also be harmless and I suspect you can delete the check.
> > >
> >
> > Yes, I did a quick look to see if we can do that, but couldn't find a clue.
> > Not sure if that's any optimisation, we can use order from page_private
> > and set the values accordingly ?
> >
> And I forgot to add the diff that I mentioned above, something like the
> patch below.
> 
> Regards,
> Sudeep
> 
> -->8
> 
> diff --git i/drivers/perf/arm_spe_pmu.c w/drivers/perf/arm_spe_pmu.c
> index 7cb766dafe85..45cd62517080 100644
> --- i/drivers/perf/arm_spe_pmu.c
> +++ w/drivers/perf/arm_spe_pmu.c
> @@ -827,7 +827,7 @@ static void arm_spe_pmu_read(struct perf_event *event)
>  static void *arm_spe_pmu_setup_aux(struct perf_event *event, void **pages,
>  				   int nr_pages, bool snapshot)
>  {
> -	int i, cpu = event->cpu;
> +	int i, j, cpu = event->cpu;
>  	struct page **pglist;
>  	struct arm_spe_pmu_buf *buf;
>  
> @@ -859,11 +859,12 @@ static void *arm_spe_pmu_setup_aux(struct perf_event *event, void **pages,
>  		struct page *page = virt_to_page(pages[i]);
>  
>  		if (PagePrivate(page)) {
> -			pr_warn("unexpected high-order page for auxbuf!");
> -			goto out_free_pglist;
> +			for (j = 0; j < 1 << page_private(page); j++)
> +				pglist[i + j] = page++;
> +			i += j - 1;
> +		} else {
> +			pglist[i] = page;

Hmm. Given that vmap() doesn't do anything special for high-order pages
and rb_alloc_aux()/rb_alloc_aux_page() already split the allocation up
for the page array, what does your change accomplish on top of that?

Will