On Wed, 2014-24-09 at 19:24:38 UTC, suka...@linux.vnet.ibm.com wrote:
> From: Cody P Schafer <c...@linux.vnet.ibm.com>
>
> Ian pointed out the use of __aligned(4096) caused rather large stack
> consumption in single_24x7_request(), so use the kmem_cache
> hv_page_cache (which we've already got set up for other allocations)
> insead of allocating locally.
>
> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
> index 70d4f74..2f2215c 100644
> --- a/arch/powerpc/perf/hv-24x7.c
> +++ b/arch/powerpc/perf/hv-24x7.c
> @@ -294,7 +294,7 @@ static unsigned long single_24x7_request(u8 domain, u32
> offset, u16 ix,
> u16 lpar, u64 *res,
> bool success_expected)
> {
> - unsigned long ret;
> + unsigned long ret = -ENOMEM;
>
> /*
> * request_buffer and result_buffer are not required to be 4k aligned,
> @@ -304,7 +304,27 @@ static unsigned long single_24x7_request(u8 domain, u32
> offset, u16 ix,
> struct reqb {
> struct hv_24x7_request_buffer buf;
> struct hv_24x7_request req;
> - } __packed __aligned(4096) request_buffer = {
> + } __packed * request_buffer;
No space after the * please.
> + struct resb {
You never use the struct name so this can be anonymous, eg:
struct {
struct hv_24x7_data_result_buffer buf;
...
> + struct hv_24x7_data_result_buffer buf;
> + struct hv_24x7_result res;
> + struct hv_24x7_result_element elem;
> + __be64 result;
> + } __packed * result_buffer;
No space again.
> + BUILD_BUG_ON(sizeof(*request_buffer) > 4096);
> + BUILD_BUG_ON(sizeof(*result_buffer) > 4096);
> +
> + request_buffer = kmem_cache_alloc(hv_page_cache, GFP_USER);
Why aren't we using kzalloc here?
It looks like we're initialising everything below except the reserved fields,
but are they allowed to be non-zero? It's probably safer to just kzalloc it.
> + if (!request_buffer)
> + goto out_reqb;
If prefer labels to be named for what they do, so this would be just "out".
> +
> + result_buffer = kmem_cache_zalloc(hv_page_cache, GFP_USER);
> + if (!result_buffer)
> + goto out_resb;
And this would be "out_free_request_buffer".
> +
> + *request_buffer = (struct reqb) {
> .buf = {
> .interface_version = HV_24X7_IF_VERSION_CURRENT,
> .num_requests = 1,
> @@ -320,28 +340,30 @@ static unsigned long single_24x7_request(u8 domain, u32
> offset, u16 ix,
> }
> };
>
> - struct resb {
> - struct hv_24x7_data_result_buffer buf;
> - struct hv_24x7_result res;
> - struct hv_24x7_result_element elem;
> - __be64 result;
> - } __packed __aligned(4096) result_buffer = {};
> -
> ret = plpar_hcall_norets(H_GET_24X7_DATA,
> - virt_to_phys(&request_buffer), sizeof(request_buffer),
> - virt_to_phys(&result_buffer), sizeof(result_buffer));
> + virt_to_phys(request_buffer), sizeof(*request_buffer),
> + virt_to_phys(result_buffer), sizeof(*result_buffer));
>
> if (ret) {
> if (success_expected)
> pr_err_ratelimited("hcall failed: %d %#x %#x %d =>
> 0x%lx (%ld) detail=0x%x failing ix=%x\n",
> domain, offset, ix, lpar,
> ret, ret,
> - result_buffer.buf.detailed_rc,
> - result_buffer.buf.failing_request_ix);
> - return ret;
> + result_buffer->buf.detailed_rc,
> + result_buffer->buf.failing_request_ix);
> + goto out_hcall;
> }
>
> - *res = be64_to_cpu(result_buffer.result);
> + *res = be64_to_cpu(result_buffer->result);
> + kfree(result_buffer);
> + kfree(request_buffer);
> + return ret;
> +
> +out_hcall:
> + kfree(result_buffer);
> +out_resb:
> + kfree(request_buffer);
> +out_reqb:
> return ret;
> }
Wouldn't this be better as:
*res = be64_to_cpu(result_buffer->result);
out_free_result_buffer:
kfree(result_buffer);
out_free_request_buffer:
kfree(request_buffer);
out:
return ret;
}
cheers
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
