Message ID | 20160209030830.GA4815@us.ibm.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
On Tue, 2016-09-02 at 03:08:30 UTC, Sukadev Bhattiprolu wrote: > >From 31edd352fb7c2a72913f1977fa1bf168109089ad Mon Sep 17 00:00:00 2001 > From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> > Date: Tue, 9 Feb 2016 02:47:45 -0500 > Subject: [PATCH] powerpc/perf/hv-gpci: Increase request buffer size > > The GPCI hcall allows for a 4K buffer but we limit the buffer > to 1K. The problem with a 1K buffer is if a request results in > returning more values than can be accomodated in the 1K buffer > the request will fail. > > The buffer we are using is currently allocated on the stack and > hence limited in size. Instead use a per-CPU 4K buffer like we do > with 24x7 counters (hv-24x7.c). > > diff --git a/arch/powerpc/perf/hv-gpci.c b/arch/powerpc/perf/hv-gpci.c > index 856fe6e..e6fad73 100644 > --- a/arch/powerpc/perf/hv-gpci.c > +++ b/arch/powerpc/perf/hv-gpci.c > @@ -127,8 +127,16 @@ static const struct attribute_group *attr_groups[] = { > NULL, > }; > > +#define HGPCI_REQ_BUFFER_SIZE 4096 > #define GPCI_MAX_DATA_BYTES \ > - (1024 - sizeof(struct hv_get_perf_counter_info_params)) > + (HGPCI_REQ_BUFFER_SIZE - sizeof(struct hv_get_perf_counter_info_params)) > + > +DEFINE_PER_CPU(char, hv_gpci_reqb[HGPCI_REQ_BUFFER_SIZE]) __aligned(sizeof(uint64_t)); > + > +struct hv_gpci_request_buffer { > + struct hv_get_perf_counter_info_params params; > + uint8_t bytes[1]; bytes is 1 byte long, but .. > @@ -163,9 +168,11 @@ static unsigned long single_gpci_request(u32 req, u32 starting_index, > */ > count = 0; > for (i = offset; i < offset + length; i++) > - count |= arg.bytes[i] << (i - offset); > + count |= arg->bytes[i] << (i - offset); Here you read from bytes[i] where i can be > 1 (AFAICS). That's fishy at best, and newer GCCs just don't allow it. I think you could do this and it would work, but untested: struct hv_gpci_request_buffer { struct hv_get_perf_counter_info_params params; uint8_t bytes[4096 - sizeof(struct hv_get_perf_counter_info_parms)]; }; cheers
Michael Ellerman [mpe@ellerman.id.au] wrote: > Here you read from bytes[i] where i can be > 1 (AFAICS). Yes, buffer is large enough and I thought this construct of array was used in a several places. Maybe they are being changed out now (struct pid has one such usage). > > That's fishy at best, and newer GCCs just don't allow it. Ah, ok. > > I think you could do this and it would work, but untested: > > struct hv_gpci_request_buffer { > struct hv_get_perf_counter_info_params params; > uint8_t bytes[4096 - sizeof(struct hv_get_perf_counter_info_parms)]; There is a macro for this computation in that file. I could have used that. Will change it and repost. Thanks, Sukadev
diff --git a/arch/powerpc/perf/hv-gpci.c b/arch/powerpc/perf/hv-gpci.c index 856fe6e..e6fad73 100644 --- a/arch/powerpc/perf/hv-gpci.c +++ b/arch/powerpc/perf/hv-gpci.c @@ -127,8 +127,16 @@ static const struct attribute_group *attr_groups[] = { NULL, }; +#define HGPCI_REQ_BUFFER_SIZE 4096 #define GPCI_MAX_DATA_BYTES \ - (1024 - sizeof(struct hv_get_perf_counter_info_params)) + (HGPCI_REQ_BUFFER_SIZE - sizeof(struct hv_get_perf_counter_info_params)) + +DEFINE_PER_CPU(char, hv_gpci_reqb[HGPCI_REQ_BUFFER_SIZE]) __aligned(sizeof(uint64_t)); + +struct hv_gpci_request_buffer { + struct hv_get_perf_counter_info_params params; + uint8_t bytes[1]; +} __packed; static unsigned long single_gpci_request(u32 req, u32 starting_index, u16 secondary_index, u8 version_in, u32 offset, u8 length, @@ -137,24 +145,21 @@ static unsigned long single_gpci_request(u32 req, u32 starting_index, unsigned long ret; size_t i; u64 count; + struct hv_gpci_request_buffer *arg; + + arg = (void *)get_cpu_var(hv_gpci_reqb); + memset(arg, 0, HGPCI_REQ_BUFFER_SIZE); - struct { - struct hv_get_perf_counter_info_params params; - uint8_t bytes[GPCI_MAX_DATA_BYTES]; - } __packed __aligned(sizeof(uint64_t)) arg = { - .params = { - .counter_request = cpu_to_be32(req), - .starting_index = cpu_to_be32(starting_index), - .secondary_index = cpu_to_be16(secondary_index), - .counter_info_version_in = version_in, - } - }; + arg->params.counter_request = cpu_to_be32(req); + arg->params.starting_index = cpu_to_be32(starting_index); + arg->params.secondary_index = cpu_to_be16(secondary_index); + arg->params.counter_info_version_in = version_in; ret = plpar_hcall_norets(H_GET_PERF_COUNTER_INFO, - virt_to_phys(&arg), sizeof(arg)); + virt_to_phys(arg), HGPCI_REQ_BUFFER_SIZE); if (ret) { pr_devel("hcall failed: 0x%lx\n", ret); - return ret; + goto out; } /* @@ -163,9 +168,11 @@ static unsigned long single_gpci_request(u32 req, u32 starting_index, */ count = 0; for (i = offset; i < offset + length; i++) - count |= arg.bytes[i] << (i - offset); + count |= arg->bytes[i] << (i - offset); *value = count; +out: + put_cpu_var(hv_gpci_reqb); return ret; }