Re: [PATCH v9 1/4] perf, kvm/{x86, s390}: Remove dependency on uapi/kvm_perf.h

[Alexander Yarygin]

Hemant Kumar writes:

> Hi David,
>
>
> On 10/07/2015 09:41 PM, David Ahern wrote:
>> On 10/6/15 8:25 PM, Hemant Kumar wrote:
>>> @@ -358,7 +357,12 @@ static bool handle_end_event(struct
>>> perf_kvm_stat *kvm,
>>>   time_diff = sample->time - time_begin;
>>>
>>>   if (kvm->duration && time_diff > kvm->duration) {
>>> -char decode[DECODE_STR_LEN];
>>> +char *decode = zalloc(decode_str_len);
>>
>> decode can still be a stack variable even with variable length.
>>
>
> Yeah, we can do that. But, I am not sure whether its a standard way.
>

Well, I also vote for making them variable length arrays. I guess that
wouldn't be a problem because the "variable" here is actually a constant
compile time value, even if it's extern.

But if people are strongly against it, as an alternative I can suggest
to move the 'char *decode' variable to the perf_kvm_stat structure,
allocate it once e.g. in kvm_events_report() and just write to it via
decode_key(). If I'm not mistaken, we always write \0 trimmed strings,
so garbage after \0 shouldn't be a problem.

It's not a real problem anyway :)

For s390 parts:
Acked-by: Alexander Yarygin <yary...@linux.vnet.ibm.com>

>> -8<-
>>
>>> @@ -575,7 +581,7 @@ static void show_timeofday(void)
>>>
>>>   static void print_result(struct perf_kvm_stat *kvm)
>>>   {
>>> -char decode[DECODE_STR_LEN];
>>> +char *decode;
>>
>> and a stack variable here too.
>>
>
> Same here.
>
>> David
>> ___
>> Linuxppc-dev mailing list
>> Linuxppc-dev@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/linuxppc-dev

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v8 1/4] perf, kvm/{x86, s390}: Remove dependency on uapi/kvm_perf.h

[Alexander Yarygin]

Hemant Kumar  writes:

> Its better to remove the dependency on uapi/kvm_perf.h to allow dynamic
> discovery of kvm events (if its needed). To do this, some extern
> variables have been introduced with which we can keep the generic
> functions generic.
>
> Signed-off-by: Hemant Kumar 
> ---
> Changes since v7:
> - Removed __maybe_unused for some parameters which weren't needed.
>
>  tools/perf/arch/s390/util/kvm-stat.c | 10 -
>  tools/perf/arch/x86/util/kvm-stat.c  | 12 ++-
>  tools/perf/builtin-kvm.c | 39 
> +++-
>  tools/perf/util/kvm-stat.h   |  3 +++
>  4 files changed, 48 insertions(+), 16 deletions(-)

Hello,


The patchset doesn't break s390 code (and at least build on x86), but I
don't really like some things here (e.g. direct access to
kvm_events_tp), see below.

Thanks.

CC: David Ahern 

>
> diff --git a/tools/perf/arch/s390/util/kvm-stat.c 
> b/tools/perf/arch/s390/util/kvm-stat.c
> index a5dbc07..c2acb3e 100644
> --- a/tools/perf/arch/s390/util/kvm-stat.c
> +++ b/tools/perf/arch/s390/util/kvm-stat.c
> @@ -10,7 +10,11 @@
>   */
>
>  #include "../../util/kvm-stat.h"
> -#include 
> +#include 
> +
> +#define DECODE_STR_LEN 40
> +#define VCPU_ID "id"
> +#define KVM_EXIT_REASON "icptcode"
>

I would probably drop them. There are no users besides newly
introduced const char *vcpu_id_str and decore_str_len etc anyway.

>  define_exit_reasons_table(sie_exit_reasons, sie_intercept_code);
>  define_exit_reasons_table(sie_icpt_insn_codes, icpt_insn_codes);
> @@ -83,6 +87,10 @@ const char * const kvm_events_tp[] = {
>   NULL,
>  };
>
> +const char *vcpu_id_str = VCPU_ID;
> +const int decode_str_len = DECODE_STR_LEN;
> +const char *exit_reason_code = KVM_EXIT_REASON;
> +
>  struct kvm_reg_events_ops kvm_reg_events_ops[] = {
>   { .name = "vmexit", .ops = _events },
>   { NULL, NULL },
> diff --git a/tools/perf/arch/x86/util/kvm-stat.c 
> b/tools/perf/arch/x86/util/kvm-stat.c
> index 14e4e66..2d0d43b5 100644
> --- a/tools/perf/arch/x86/util/kvm-stat.c
> +++ b/tools/perf/arch/x86/util/kvm-stat.c
> @@ -1,5 +1,11 @@
>  #include "../../util/kvm-stat.h"
> -#include 
> +#include 
> +#include 
> +#include 
> +
> +#define DECODE_STR_LEN 20
> +#define VCPU_ID "vcpu_id"
> +#define KVM_EXIT_REASON "exit_reason"
>
>  define_exit_reasons_table(vmx_exit_reasons, VMX_EXIT_REASONS);
>  define_exit_reasons_table(svm_exit_reasons, SVM_EXIT_REASONS);
> @@ -129,6 +135,10 @@ const char * const kvm_events_tp[] = {
>   NULL,
>  };
>
> +const char *vcpu_id_str = VCPU_ID;
> +const int decode_str_len = DECODE_STR_LEN;
> +const char *exit_reason_code = KVM_EXIT_REASON;
> +
>  struct kvm_reg_events_ops kvm_reg_events_ops[] = {
>   { .name = "vmexit", .ops = _events },
>   { .name = "mmio", .ops = _events },
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index fc1cffb..ef25fcf 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -31,20 +31,18 @@
>  #include 
>
>  #ifdef HAVE_KVM_STAT_SUPPORT
> -#include 
>  #include "util/kvm-stat.h"
>
> -void exit_event_get_key(struct perf_evsel *evsel,
> - struct perf_sample *sample,
> +void exit_event_get_key(struct perf_evsel *evsel, struct perf_sample *sample,
>   struct event_key *key)
>  {
>   key->info = 0;
> - key->key = perf_evsel__intval(evsel, sample, KVM_EXIT_REASON);
> + key->key = perf_evsel__intval(evsel, sample, exit_reason_code);
>  }
>
>  bool kvm_exit_event(struct perf_evsel *evsel)
>  {
> - return !strcmp(evsel->name, KVM_EXIT_TRACE);
> + return !strncmp(evsel->name, kvm_events_tp[1], strlen(evsel->name));
>  }

Hmm, direct access to kvm_events_tp? Maybe add a getter for this or
something like extern char *kvm_exit_trace;?
/* why strncmp? */

>
>  bool exit_event_begin(struct perf_evsel *evsel,
> @@ -60,7 +58,7 @@ bool exit_event_begin(struct perf_evsel *evsel,
>
>  bool kvm_entry_event(struct perf_evsel *evsel)
>  {
> - return !strcmp(evsel->name, KVM_ENTRY_TRACE);
> + return !strncmp(evsel->name, kvm_events_tp[0], strlen(evsel->name));
>  }
>
>  bool exit_event_end(struct perf_evsel *evsel,
> @@ -71,8 +69,8 @@ bool exit_event_end(struct perf_evsel *evsel,
>  }
>
>  static const char *get_exit_reason(struct perf_kvm_stat *kvm,
> -struct exit_reasons_table *tbl,
> -u64 exit_code)
> + struct exit_reasons_table *tbl,
> + u64 exit_code)
>  {
>   while (tbl->reason != NULL) {
>   if (tbl->exit_code == exit_code)
> @@ -92,7 +90,7 @@ void exit_event_decode_key(struct perf_kvm_stat *kvm,
>   const char *exit_reason = get_exit_reason(kvm, key->exit_reasons,
> key->key);
>
> - scnprintf(decode, DECODE_STR_LEN, "%s", exit_reason);
> +