Re: [PATCH 4/4] perf tools: Support reading PERF_FORMAT_LOST

[Jiri Olsa]

On Tue, Aug 16, 2022 at 03:17:47PM -0700, Namhyung Kim wrote:

SNIP

> diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
> index a7b0931d5137..7753368d70d6 100644
> --- a/tools/perf/util/event.h
> +++ b/tools/perf/util/event.h
> @@ -65,7 +65,8 @@ struct stack_dump {
>  
>  struct sample_read_value {
>  	u64 value;
> -	u64 id;
> +	u64 id;   /* only if PERF_FORMAT_ID */
> +	u64 lost; /* only if PERF_FORMAT_LOST */
>  };

I was wondering why not to split this patch into smaller piece,
but once you change this struct you break all the places

SNIP

> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1541,7 +1541,7 @@ static int evsel__read_one(struct evsel *evsel, int cpu_map_idx, int thread)
>  }
>  
>  static void evsel__set_count(struct evsel *counter, int cpu_map_idx, int thread,
> -			     u64 val, u64 ena, u64 run)
> +			     u64 val, u64 ena, u64 run, u64 lost)
>  {
>  	struct perf_counts_values *count;
>  
> @@ -1550,6 +1550,7 @@ static void evsel__set_count(struct evsel *counter, int cpu_map_idx, int thread,
>  	count->val    = val;
>  	count->ena    = ena;
>  	count->run    = run;
> +	count->lost   = lost;
>  
>  	perf_counts__set_loaded(counter->counts, cpu_map_idx, thread, true);
>  }
> @@ -1558,7 +1559,7 @@ static int evsel__process_group_data(struct evsel *leader, int cpu_map_idx, int
>  {
>  	u64 read_format = leader->core.attr.read_format;
>  	struct sample_read_value *v;
> -	u64 nr, ena = 0, run = 0, i;
> +	u64 nr, ena = 0, run = 0, lost = 0, i;
>  
>  	nr = *data++;
>  
> @@ -1573,16 +1574,25 @@ static int evsel__process_group_data(struct evsel *leader, int cpu_map_idx, int
>  
>  	v = (struct sample_read_value *) data;
>  
> -	evsel__set_count(leader, cpu_map_idx, thread, v[0].value, ena, run);
> +	if (read_format & PERF_FORMAT_LOST)
> +		lost = v->lost;
> +
> +	evsel__set_count(leader, cpu_map_idx, thread, v[0].value, ena, run, lost);
> +
> +	v = next_sample_read_value(v, read_format);

oneway of making this simpler here and share with other places
could be adding something like:

  for_each_group_data(v, i, nr, read_format) {
  }

but not sure how would that turn out, thoughts?

>  
>  	for (i = 1; i < nr; i++) {
>  		struct evsel *counter;
>  
> -		counter = evlist__id2evsel(leader->evlist, v[i].id);
> +		counter = evlist__id2evsel(leader->evlist, v->id);
>  		if (!counter)
>  			return -EINVAL;
>  
> -		evsel__set_count(counter, cpu_map_idx, thread, v[i].value, ena, run);
> +		if (read_format & PERF_FORMAT_LOST)
> +			lost = v->lost;
> +
> +		evsel__set_count(counter, cpu_map_idx, thread, v->value, ena, run, lost);
> +		v = next_sample_read_value(v, read_format);
>  	}
>  
>  	return 0;
> @@ -2475,16 +2485,21 @@ int evsel__parse_sample(struct evsel *evsel, union perf_event *event,
>  
>  			if (data->read.group.nr > max_group_nr)
>  				return -EFAULT;
> -			sz = data->read.group.nr *
> -			     sizeof(struct sample_read_value);
> +
> +			sz = data->read.group.nr * sample_read_value_size(read_format);
>  			OVERFLOW_CHECK(array, sz, max_size);
> -			data->read.group.values =
> -					(struct sample_read_value *)array;
> +			data->read.group.values = (void *)array;

nit, is this void casting needed?

thanks,
jirka

>  			array = (void *)array + sz;
>  		} else {
>  			OVERFLOW_CHECK_u64(array);
>  			data->read.one.id = *array;
>  			array++;
> +
> +			if (read_format & PERF_FORMAT_LOST) {
> +				OVERFLOW_CHECK_u64(array);
> +				data->read.one.lost = *array;
> +				array++;
> +			}
>  		}
>  	}
>  

SNIP