Re: [PATCH 0/8] Support for 24x7 hcall interface version 2

[Michael Ellerman]

Thiago Jung Bauermann  writes:

> Michael Ellerman  writes:
>> Thiago Jung Bauermann  writes:
>>> The hypervisor interface to access 24x7 performance counters (which collect
>>> performance information from system power on to system power off) has been
>>> extended in POWER9 adding new fields to the request and result element
>>> structures.
>>>
>>> Also, results for some domains now return more than one result element and
>>> those need to be added to get a total count.
>>>
>>> The first two patches fix bugs in the existing code. The following 4
>>> patches are code improvements and the last two finally implement support
>>> for the changes in POWER9 described above.
>>>
>>> POWER8 systems only support version 1 of the interface, while POWER9
>>> systems only support version 2. I tested these patches on POWER8 to verify
>>> that there are no regressions, and also on POWER9 DD1.
>>
>> Where is version 2 documented?
>
> Only in an internal specification.

Presumably a PAPR ACR, what's the title? And is it approved/accepted?

>> And what happens when we boot on a POWER9 in POWER8 compatibility mode?
>
> It will still use version 2 of the API, and still require aggregation of
> result elements. Does this mean that hv_24x7_init should check
> cur_cpu_spec->oprofile_cpu_type for "ppc64/power9" instead of
> cpu_has_feature(CPU_FTR_ARCH_300)?

Yes.

cheers

Re: [PATCH 0/8] Support for 24x7 hcall interface version 2

[Thiago Jung Bauermann]

Michael Ellerman  writes:
> Thiago Jung Bauermann  writes:
>> The hypervisor interface to access 24x7 performance counters (which collect
>> performance information from system power on to system power off) has been
>> extended in POWER9 adding new fields to the request and result element
>> structures.
>>
>> Also, results for some domains now return more than one result element and
>> those need to be added to get a total count.
>>
>> The first two patches fix bugs in the existing code. The following 4
>> patches are code improvements and the last two finally implement support
>> for the changes in POWER9 described above.
>>
>> POWER8 systems only support version 1 of the interface, while POWER9
>> systems only support version 2. I tested these patches on POWER8 to verify
>> that there are no regressions, and also on POWER9 DD1.
>
> Where is version 2 documented?

Only in an internal specification.

> And what happens when we boot on a POWER9 in POWER8 compatibility mode?

It will still use version 2 of the API, and still require aggregation of
result elements. Does this mean that hv_24x7_init should check
cur_cpu_spec->oprofile_cpu_type for "ppc64/power9" instead of
cpu_has_feature(CPU_FTR_ARCH_300)?

There were a couple of comments from Suka which still needed resolving:

Sukadev Bhattiprolu  writes:
> Thiago Jung Bauermann [bauer...@linux.vnet.ibm.com] wrote:
>> +/**
>> + * get_count_from_result - get event count from the given result
>> + *
>> + * @event:  Event associated with @res.
>> + * @resb:   Result buffer containing @res.
>> + * @res:Result to work on.
>> + * @countp: Output variable containing the event count.
>> + * @next:   Optional output variable pointing to the next result in @resb.
>> + */
>> +static int get_count_from_result(struct perf_event *event,
>> + struct hv_24x7_data_result_buffer *resb,
>> + struct hv_24x7_result *res, u64 *countp,
>> + struct hv_24x7_result **next)
>> +{
>> +u16 num_elements = be16_to_cpu(res->num_elements_returned);
>> +u16 data_size = be16_to_cpu(res->result_element_data_size);
>> +unsigned int data_offset;
>> +void *element_data;
>> +int ret = 0;
>> +
>> +/*
>> + * We can bail out early if the result is empty.
>> + */
>> +if (!num_elements) {
>> +pr_debug("Result of request %hhu is empty, nothing to do\n",
>> + res->result_ix);
>> +
>> +if (next)
>> +*next = (struct hv_24x7_result *) res->elements;
>> +
>> +return -ENODATA;
>> +}
>> +
>> +/*
>> + * This code assumes that a result has only one element.
>> + */
>> +if (num_elements != 1) {
>> +pr_debug("Error: result of request %hhu has %hu elements\n",
>> + res->result_ix, num_elements);
>
> Could this happen due to an user request or would this indicate a bug
> in the way we submitted the request (perf should submit separate request
> for each lpar/index - we set ->max_ix and ->max_num_lpars to cpu_be16(1).

By specifying 1 as the maximum for the smallest resource we're
requesting, we guarantee one element per result. Since that's what we
do this would indeed be a bug. For v2 (which I'll send shortly) I
changed the message above to a pr_err, and changed the returned error to
-EIO instead of -ENOTSUPP.

> Minor inconsistency with proceeding, is that if the next element passes,
> this return code is lost/over written. IOW, h_24x7_event_commit_txn()'s
> return value depends on the last element we process, even if intermediate
> ones encounter an error.

For v2, I changed h_24x7_event_commit_txn to break out of the loop if
there's an error in any processed result. Also, since in that case @next
isn't used anymore, get_count_from_result will exit early instead of
going through the motions.

>> +
>> +if (!next)
>> +return -ENOTSUPP;
>> +
>> +/*
>> + * We still need to go through the motions so that we can return
>> + * a pointer to the next result.
>> + */
>> +ret = -ENOTSUPP;
>> +}
>> +
>> +if (data_size != sizeof(u64)) {
>> +pr_debug("Error: result of request %hhu has data of %hu 
>> bytes\n",
>> + res->result_ix, data_size);
>> +
>> +if (!next)
>> +return -ENOTSUPP;
>> +
>> +ret = -ENOTSUPP;
>> +}
>> +
>> +if (resb->interface_version == 1)
>> +data_offset = offsetof(struct hv_24x7_result_element_v1,
>> +   element_data);
>> +else
>> +data_offset = offsetof(struct hv_24x7_result_element_v2,
>> +   element_data);
>> +
>> +element_data = res->elements + data_offset;
>> +
>> +if (!ret)
>> +*countp 

Re: [PATCH 0/8] Support for 24x7 hcall interface version 2

[Michael Ellerman]

Thiago Jung Bauermann  writes:

> Hello,
>
> The hypervisor interface to access 24x7 performance counters (which collect
> performance information from system power on to system power off) has been
> extended in POWER9 adding new fields to the request and result element
> structures.
>
> Also, results for some domains now return more than one result element and
> those need to be added to get a total count.
>
> The first two patches fix bugs in the existing code. The following 4
> patches are code improvements and the last two finally implement support
> for the changes in POWER9 described above.
>
> POWER8 systems only support version 1 of the interface, while POWER9
> systems only support version 2. I tested these patches on POWER8 to verify
> that there are no regressions, and also on POWER9 DD1.

Where is version 2 documented?

And what happens when we boot on a POWER9 in POWER8 compatibility mode?

cheers

Re: [PATCH 0/8] Support for 24x7 hcall interface version 2

[Sukadev Bhattiprolu]

Thiago Jung Bauermann [bauer...@linux.vnet.ibm.com] wrote:
> Hello,
> 
> The hypervisor interface to access 24x7 performance counters (which collect
> performance information from system power on to system power off) has been
> extended in POWER9 adding new fields to the request and result element
> structures.
> 
> Also, results for some domains now return more than one result element and
> those need to be added to get a total count.
> 
> The first two patches fix bugs in the existing code. The following 4
> patches are code improvements and the last two finally implement support
> for the changes in POWER9 described above.
> 
> POWER8 systems only support version 1 of the interface, while POWER9
> systems only support version 2. I tested these patches on POWER8 to verify
> that there are no regressions, and also on POWER9 DD1.
> 
> Thiago Jung Bauermann (8):
>   powerpc/perf/hv-24x7: Fix passing of catalog version number
>   powerpc/perf/hv-24x7: Fix off-by-one error in request_buffer check
>   powerpc/perf/hv-24x7: Properly iterate through results
>   powerpc-perf/hx-24x7: Don't log failed hcall twice
>   powerpc/perf/hv-24x7: Fix return value of hcalls
>   powerpc/perf/hv-24x7: Minor improvements
>   powerpc/perf/hv-24x7: Support v2 of the hypervisor API
>   powerpc/perf/hv-24x7: Aggregate result elements on POWER9 SMT8
> 
>  arch/powerpc/perf/hv-24x7.c| 255 
> +
>  arch/powerpc/perf/hv-24x7.h|  70 +++--
>  arch/powerpc/platforms/pseries/Kconfig |   2 +-
>  3 files changed, 255 insertions(+), 72 deletions(-)

Reviewed-by: Sukadev Bhattiprolu 
> 
> -- 
> 2.7.4

[PATCH 0/8] Support for 24x7 hcall interface version 2

[Thiago Jung Bauermann]

Hello,

The hypervisor interface to access 24x7 performance counters (which collect
performance information from system power on to system power off) has been
extended in POWER9 adding new fields to the request and result element
structures.

Also, results for some domains now return more than one result element and
those need to be added to get a total count.

The first two patches fix bugs in the existing code. The following 4
patches are code improvements and the last two finally implement support
for the changes in POWER9 described above.

POWER8 systems only support version 1 of the interface, while POWER9
systems only support version 2. I tested these patches on POWER8 to verify
that there are no regressions, and also on POWER9 DD1.

Thiago Jung Bauermann (8):
  powerpc/perf/hv-24x7: Fix passing of catalog version number
  powerpc/perf/hv-24x7: Fix off-by-one error in request_buffer check
  powerpc/perf/hv-24x7: Properly iterate through results
  powerpc-perf/hx-24x7: Don't log failed hcall twice
  powerpc/perf/hv-24x7: Fix return value of hcalls
  powerpc/perf/hv-24x7: Minor improvements
  powerpc/perf/hv-24x7: Support v2 of the hypervisor API
  powerpc/perf/hv-24x7: Aggregate result elements on POWER9 SMT8

 arch/powerpc/perf/hv-24x7.c| 255 +
 arch/powerpc/perf/hv-24x7.h|  70 +++--
 arch/powerpc/platforms/pseries/Kconfig |   2 +-
 3 files changed, 255 insertions(+), 72 deletions(-)

-- 
2.7.4