[PATCH 1/2] perf/powerpc/hv-24x7: Use per-cpu page buffer

[PATCH 1/2] perf/powerpc/hv-24x7: Use per-cpu page buffer

[Sukadev Bhattiprolu]

>From 470c16c8955672103a9529c78dffbb239e9e27b8 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Tue, 9 Dec 2014 22:17:46 -0500
Subject: [PATCH 1/2] perf/poweprc/hv-24x7: Use per-cpu page buffer

The 24x7 counters are continuously running and not updated on an interrupt.
So we record the event counts when stopping the event or deleting it.

But to "read" a single counter in 24x7, we allocate a page and pass it
into the hypervisor (The HV returns the page full of counters from which
we extract the specific counter for this event).

We allocate a page using GFP_USER and when deleting the event, we end up
with the following warning because we are blocking in interrupt context.

	[  698.641709] BUG: scheduling while atomic: swapper/0/0/0x10010000

We could use GFP_ATOMIC but that could result in failures. Pre-allocate
a buffer so we don't have to allocate in interrupt context. Further as
Michael Ellerman suggested, use Per-CPU buffer so we only need to allocate
once per CPU.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 arch/powerpc/perf/hv-24x7.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index dba3408..18e1f49 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -217,11 +217,14 @@ static bool is_physical_domain(int domain)
 		domain == HV_24X7_PERF_DOMAIN_PHYSICAL_CORE;
 }
 
+DEFINE_PER_CPU(char, hv_24x7_reqb[4096]);
+DEFINE_PER_CPU(char, hv_24x7_resb[4096]);
+
 static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
 					 u16 lpar, u64 *res,
 					 bool success_expected)
 {
-	unsigned long ret = -ENOMEM;
+	unsigned long ret;
 
 	/*
 	 * request_buffer and result_buffer are not required to be 4k aligned,
@@ -243,13 +246,11 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
 	BUILD_BUG_ON(sizeof(*request_buffer) > 4096);
 	BUILD_BUG_ON(sizeof(*result_buffer) > 4096);
 
-	request_buffer = kmem_cache_zalloc(hv_page_cache, GFP_USER);
-	if (!request_buffer)
-		goto out;
+	request_buffer = (void *)get_cpu_var(hv_24x7_reqb);
+	result_buffer = (void *)get_cpu_var(hv_24x7_resb);
 
-	result_buffer = kmem_cache_zalloc(hv_page_cache, GFP_USER);
-	if (!result_buffer)
-		goto out_free_request_buffer;
+	memset(request_buffer, 0, 4096);
+	memset(result_buffer, 0, 4096);
 
 	*request_buffer = (struct reqb) {
 		.buf = {
@@ -278,15 +279,11 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
 				domain, offset, ix, lpar, ret, ret,
 				result_buffer->buf.detailed_rc,
 				result_buffer->buf.failing_request_ix);
-		goto out_free_result_buffer;
+		goto out;
 	}
 
 	*res = be64_to_cpu(result_buffer->result);
 
-out_free_result_buffer:
-	kfree(result_buffer);
-out_free_request_buffer:
-	kfree(request_buffer);
 out:
 	return ret;
 }
-- 
1.8.3.1

[PATCH 2/2] perf/power/hv-24x7: Use kmem_cache_free() instead of kfree

[Sukadev Bhattiprolu]

>From 982bf5a660ae33fbe2f9187187caa6752c66783d Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Wed, 10 Dec 2014 01:43:34 -0500
Subject: [PATCH 2/2] power/perf: hv-24x7: Use kmem_cache_free() instead of
 kfree

Use kmem_cache_free() to free a buffer allocated with kmem_cache_alloc().

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>

p#	set2.log
---
 arch/powerpc/perf/hv-24x7.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 18e1f49..a2c6211 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -177,7 +177,7 @@ static ssize_t _name##_show(struct device *dev,			\
 	}							\
 	ret = sprintf(buf, _fmt, _expr);			\
 e_free:								\
-	kfree(page);						\
+	kmem_cache_free(hv_page_cache, page);			\
 	return ret;						\
 }								\
 static DEVICE_ATTR_RO(_name)
-- 
1.8.3.1

Re: [PATCH 1/2] perf/powerpc/hv-24x7: Use per-cpu page buffer

[Michael Ellerman]

On Tue, 2014-12-09 at 23:06 -0800, Sukadev Bhattiprolu wrote:
> From 470c16c8955672103a9529c78dffbb239e9e27b8 Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Tue, 9 Dec 2014 22:17:46 -0500
> Subject: [PATCH 1/2] perf/poweprc/hv-24x7: Use per-cpu page buffer
> 
> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
> index dba3408..18e1f49 100644
> --- a/arch/powerpc/perf/hv-24x7.c
> +++ b/arch/powerpc/perf/hv-24x7.c
> @@ -217,11 +217,14 @@ static bool is_physical_domain(int domain)
>  		domain == HV_24X7_PERF_DOMAIN_PHYSICAL_CORE;
>  }
>  
> +DEFINE_PER_CPU(char, hv_24x7_reqb[4096]);
> +DEFINE_PER_CPU(char, hv_24x7_resb[4096]);

Do we need it to be 4K aligned also? I would guess so.

Rather than declaring these as char arrays and then casting below, can you pull
the struct definitions up and then declare the per cpu variables with the
proper type.


>  static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
>  					 u16 lpar, u64 *res,
>  					 bool success_expected)
>  {
> -	unsigned long ret = -ENOMEM;
> +	unsigned long ret;
>  
>  	/*
>  	 * request_buffer and result_buffer are not required to be 4k aligned,
> @@ -243,13 +246,11 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
>  	BUILD_BUG_ON(sizeof(*request_buffer) > 4096);
>  	BUILD_BUG_ON(sizeof(*result_buffer) > 4096);
>  
> -	request_buffer = kmem_cache_zalloc(hv_page_cache, GFP_USER);
> -	if (!request_buffer)
> -		goto out;
> +	request_buffer = (void *)get_cpu_var(hv_24x7_reqb);
> +	result_buffer = (void *)get_cpu_var(hv_24x7_resb);
>  
> -	result_buffer = kmem_cache_zalloc(hv_page_cache, GFP_USER);
> -	if (!result_buffer)
> -		goto out_free_request_buffer;
> +	memset(request_buffer, 0, 4096);
> +	memset(result_buffer, 0, 4096);

Do we have to memset them? That's not going to speed things up.


cheers

Re: [PATCH 2/2] perf/power/hv-24x7: Use kmem_cache_free() instead of kfree

[Michael Ellerman]

On Tue, 2014-12-09 at 23:09 -0800, Sukadev Bhattiprolu wrote:
> From 982bf5a660ae33fbe2f9187187caa6752c66783d Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Wed, 10 Dec 2014 01:43:34 -0500
> Subject: [PATCH 2/2] power/perf: hv-24x7: Use kmem_cache_free() instead of
>  kfree

This got a bit munged, ...

> Use kmem_cache_free() to free a buffer allocated with kmem_cache_alloc().
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> 
> p#	set2.log

.. and here too.

I can fix it up this time.

cheers

Re: [PATCH 1/2] perf/powerpc/hv-24x7: Use per-cpu page buffer

[Sukadev Bhattiprolu]

Michael Ellerman [mpe@ellerman.id.au] wrote:
| On Tue, 2014-12-09 at 23:06 -0800, Sukadev Bhattiprolu wrote:
| > From 470c16c8955672103a9529c78dffbb239e9e27b8 Mon Sep 17 00:00:00 2001
| > From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
| > Date: Tue, 9 Dec 2014 22:17:46 -0500
| > Subject: [PATCH 1/2] perf/poweprc/hv-24x7: Use per-cpu page buffer
| > 
| > diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
| > index dba3408..18e1f49 100644
| > --- a/arch/powerpc/perf/hv-24x7.c
| > +++ b/arch/powerpc/perf/hv-24x7.c
| > @@ -217,11 +217,14 @@ static bool is_physical_domain(int domain)
| >  		domain == HV_24X7_PERF_DOMAIN_PHYSICAL_CORE;
| >  }
| >  
| > +DEFINE_PER_CPU(char, hv_24x7_reqb[4096]);
| > +DEFINE_PER_CPU(char, hv_24x7_resb[4096]);
| 
| Do we need it to be 4K aligned also? I would guess so.

Yes, fixed in the patch below.
| 
| Rather than declaring these as char arrays and then casting below, can you pull
| the struct definitions up and then declare the per cpu variables with the
| proper type.

Well, the structures, used for communication with HV, have variable length
arrays, like:

	struct hv_24x7_request_buffer {
		...
		struct hv_24x7_request requests[];
	};

i.e the buffer needs to be larger than reported by sizeof(). So we
allocate a large buffer and cast it. Not sure if there is a trick to
get DEFINE_PER_CPU() to do that. We could add code to allocate pages
per cpu during init, but that would mean more code, error handling etc.

| 
| 
| >  static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
| >  					 u16 lpar, u64 *res,
| >  					 bool success_expected)
| >  {
| > -	unsigned long ret = -ENOMEM;
| > +	unsigned long ret;
| >  
| >  	/*
| >  	 * request_buffer and result_buffer are not required to be 4k aligned,
| > @@ -243,13 +246,11 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
| >  	BUILD_BUG_ON(sizeof(*request_buffer) > 4096);
| >  	BUILD_BUG_ON(sizeof(*result_buffer) > 4096);
| >  
| > -	request_buffer = kmem_cache_zalloc(hv_page_cache, GFP_USER);
| > -	if (!request_buffer)
| > -		goto out;
| > +	request_buffer = (void *)get_cpu_var(hv_24x7_reqb);
| > +	result_buffer = (void *)get_cpu_var(hv_24x7_resb);
| >  
| > -	result_buffer = kmem_cache_zalloc(hv_page_cache, GFP_USER);
| > -	if (!result_buffer)
| > -		goto out_free_request_buffer;
| > +	memset(request_buffer, 0, 4096);
| > +	memset(result_buffer, 0, 4096);
| 
| Do we have to memset them? That's not going to speed things up.

I agree about the speed, specially since we have a larger buffer. But we
are reusing the buffer for independent events and some fields need to be 0
(hence the zalloc in the current code).

---


>From 545ad0771ef8f22085ed8bb486f0a81b3afc5757 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Tue, 9 Dec 2014 22:17:46 -0500
Subject: [PATCH 1/2] powerpc/perf/hv-24x7: Use per-cpu page buffer

The 24x7 counters are continuously running and not updated on an interrupt.
So we record the event counts when stopping the event or deleting it.

But to "read" a single counter in 24x7, we allocate a page and pass it
into the hypervisor (The HV returns the page full of counters from which
we extract the specific counter for this event).

We allocate a page using GFP_USER and when deleting the event, we end up
with the following warning because we are blocking in interrupt context.

	[  698.641709] BUG: scheduling while atomic: swapper/0/0/0x10010000

We could use GFP_ATOMIC but that could result in failures. Pre-allocate
a buffer so we don't have to allocate in interrupt context. Further as
Michael Ellerman suggested, use Per-CPU buffer so we only need to allocate
once per CPU.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 arch/powerpc/perf/hv-24x7.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

Changelog[v2]:
	[Michael Ellerman] Ensure buffer is aligned to 4K.


diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index dba3408..d073e06 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -217,11 +217,14 @@ static bool is_physical_domain(int domain)
 		domain == HV_24X7_PERF_DOMAIN_PHYSICAL_CORE;
 }
 
+DEFINE_PER_CPU(char, hv_24x7_reqb[4096]) __aligned(4096);
+DEFINE_PER_CPU(char, hv_24x7_resb[4096]) __aligned(4096);
+
 static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
 					 u16 lpar, u64 *res,
 					 bool success_expected)
 {
-	unsigned long ret = -ENOMEM;
+	unsigned long ret;
 
 	/*
 	 * request_buffer and result_buffer are not required to be 4k aligned,
@@ -243,13 +246,11 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
 	BUILD_BUG_ON(sizeof(*request_buffer) > 4096);
 	BUILD_BUG_ON(sizeof(*result_buffer) > 4096);
 
-	request_buffer = kmem_cache_zalloc(hv_page_cache, GFP_USER);
-	if (!request_buffer)
-		goto out;
+	request_buffer = (void *)get_cpu_var(hv_24x7_reqb);
+	result_buffer = (void *)get_cpu_var(hv_24x7_resb);
 
-	result_buffer = kmem_cache_zalloc(hv_page_cache, GFP_USER);
-	if (!result_buffer)
-		goto out_free_request_buffer;
+	memset(request_buffer, 0, 4096);
+	memset(result_buffer, 0, 4096);
 
 	*request_buffer = (struct reqb) {
 		.buf = {
@@ -278,15 +279,11 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
 				domain, offset, ix, lpar, ret, ret,
 				result_buffer->buf.detailed_rc,
 				result_buffer->buf.failing_request_ix);
-		goto out_free_result_buffer;
+		goto out;
 	}
 
 	*res = be64_to_cpu(result_buffer->result);
 
-out_free_result_buffer:
-	kfree(result_buffer);
-out_free_request_buffer:
-	kfree(request_buffer);
 out:
 	return ret;
 }
-- 
1.8.3.1

Re: [1/2] perf/powerpc/hv-24x7: Use per-cpu page buffer

[Michael Ellerman]

On Wed, 2014-10-12 at 22:29:13 UTC, sukadev@linux.vnet.ibm.com wrote:
> Michael Ellerman [mpe@ellerman.id.au] wrote:
> | On Tue, 2014-12-09 at 23:06 -0800, Sukadev Bhattiprolu wrote:
> | > From 470c16c8955672103a9529c78dffbb239e9e27b8 Mon Sep 17 00:00:00 2001
> | > From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> | > Date: Tue, 9 Dec 2014 22:17:46 -0500
> | > Subject: [PATCH 1/2] perf/poweprc/hv-24x7: Use per-cpu page buffer
> | > 
> | > diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
> | > index dba3408..18e1f49 100644
> | > --- a/arch/powerpc/perf/hv-24x7.c
> | > +++ b/arch/powerpc/perf/hv-24x7.c
> | > @@ -217,11 +217,14 @@ static bool is_physical_domain(int domain)
> | >  		domain == HV_24X7_PERF_DOMAIN_PHYSICAL_CORE;
> | >  }
> | >  
> | > +DEFINE_PER_CPU(char, hv_24x7_reqb[4096]);
> | > +DEFINE_PER_CPU(char, hv_24x7_resb[4096]);
> | 
> | Do we need it to be 4K aligned also? I would guess so.
> 
> Yes, fixed in the patch below.

OK.

> | 
> | Rather than declaring these as char arrays and then casting below, can you pull
> | the struct definitions up and then declare the per cpu variables with the
> | proper type.
> 
> Well, the structures, used for communication with HV, have variable length
> arrays, like:
> 
> 	struct hv_24x7_request_buffer {
> 		...
> 		struct hv_24x7_request requests[];
> 	};
> 
> i.e the buffer needs to be larger than reported by sizeof(). So we
> allocate a large buffer and cast it. Not sure if there is a trick to
> get DEFINE_PER_CPU() to do that.

So the array is variable length, but no larger than 4K - at least I hope
because you're using a 4K buffer :)

The neatest way to handle that is to make it a union, with the struct and a 4K
char buffer.

But we can do that as a cleanup later.

> | > +	memset(request_buffer, 0, 4096);
> | > +	memset(result_buffer, 0, 4096);
> | 
> | Do we have to memset them? That's not going to speed things up.
> 
> I agree about the speed, specially since we have a larger buffer. But we
> are reusing the buffer for independent events and some fields need to be 0
> (hence the zalloc in the current code).

Sure, so you could explicitly initialise those fields to zero.

But that also can be another cleanup.

I'll take this as it is.

cheers