Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jiri Olsa <olsajiri@gmail.com>
Date:   Wed, 29 Jun 2022 11:31:07 +0200
To:     Ian Rogers <irogers@google.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Namhyung Kim <namhyung@kernel.org>,
        James Clark <james.clark@arm.com>,
        Kees Cook <keescook@chromium.org>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Riccardo Mancini <rickyman7@gmail.com>,
        German Gomez <german.gomez@arm.com>,
        Colin Ian King <colin.king@intel.com>,
        Song Liu <songliubraving@fb.com>,
        Dave Marchevsky <davemarchevsky@fb.com>,
        Athira Rajeev <atrajeev@linux.vnet.ibm.com>,
        Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>,
        Leo Yan <leo.yan@linaro.org>, linux-perf-users@vger.kernel.org,
        linux-kernel@vger.kernel.org, Stephane Eranian <eranian@google.com>
Subject: Re: [PATCH v2 6/6] perf cpumap: Add range data encoding
Message-ID: <Yrwb28xHsPCZzSw7@krava>
References: <20220614143353.1559597-1-irogers@google.com>
 <20220614143353.1559597-7-irogers@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220614143353.1559597-7-irogers@google.com>
Precedence: bulk

On Tue, Jun 14, 2022 at 07:33:53AM -0700, Ian Rogers wrote:

SNIP

> -static size_t cpus_size(const struct perf_cpu_map *map)
> +static void synthesize_range_cpus(struct synthesize_cpu_map_data *data)
>  {
> -	return sizeof(struct cpu_map_entries) + perf_cpu_map__nr(map) * sizeof(u16);
> +	data->data->type = PERF_CPU_MAP__RANGE_CPUS;
> +	data->data->range_cpu_data.any_cpu = data->has_any_cpu;
> +	data->data->range_cpu_data.start_cpu = data->min_cpu;
> +	data->data->range_cpu_data.end_cpu = data->max_cpu;
>  }
>  
> -static size_t mask_size(const struct perf_cpu_map *map, int *max)
> -{
> -	*max = perf_cpu_map__max(map).cpu;
> -	return sizeof(struct perf_record_mask_cpu_map32) + BITS_TO_U32(*max) * sizeof(__u32);
> -}
> -
> -static void *cpu_map_data__alloc(const struct perf_cpu_map *map, size_t *size,
> -				 u16 *type, int *max)
> +static void *cpu_map_data__alloc(struct synthesize_cpu_map_data *syn_data,
> +				 size_t header_size)
>  {
>  	size_t size_cpus, size_mask;
> -	bool is_dummy = perf_cpu_map__empty(map);
>  
> -	/*
> -	 * Both array and mask data have variable size based
> -	 * on the number of cpus and their actual values.
> -	 * The size of the 'struct perf_record_cpu_map_data' is:
> -	 *
> -	 *   array = size of 'struct cpu_map_entries' +
> -	 *           number of cpus * sizeof(u64)
> -	 *
> -	 *   mask  = size of 'struct perf_record_record_cpu_map' +
> -	 *           maximum cpu bit converted to size of longs
> -	 *
> -	 * and finally + the size of 'struct perf_record_cpu_map_data'.
> -	 */
> -	size_cpus = cpus_size(map);
> -	size_mask = mask_size(map, max);
> +	syn_data->nr = perf_cpu_map__nr(syn_data->map);
> +	syn_data->has_any_cpu = (perf_cpu_map__cpu(syn_data->map, 0).cpu == -1) ? 1 : 0;

I'm bit lost in the logic in here.. should it be '.cpu != -1' ?
has_any_cpu is named as bool but used as index below ;-)

could you please keep/update the comment above and explain
the conditions when each cpu_map type is taken

thanks,
jirka

>  
> -	if (is_dummy || (size_cpus < size_mask)) {
> -		*size += size_cpus;
> -		*type  = PERF_CPU_MAP__CPUS;
> -	} else {
> -		*size += size_mask;
> -		*type  = PERF_CPU_MAP__MASK;
> +	syn_data->min_cpu = perf_cpu_map__cpu(syn_data->map, syn_data->has_any_cpu).cpu;
> +	syn_data->max_cpu = perf_cpu_map__max(syn_data->map).cpu;
> +	if (syn_data->max_cpu - syn_data->min_cpu + 1 == syn_data->nr - syn_data->has_any_cpu) {
> +		/* A consecutive range of CPUs can be encoded using a range. */
> +		assert(sizeof(u16) + sizeof(struct perf_record_range_cpu_map) == sizeof(u64));
> +		syn_data->type = PERF_CPU_MAP__RANGE_CPUS;
> +		syn_data->size = header_size + sizeof(u64);
> +		return zalloc(syn_data->size);
>  	}
>  
> -	*size += sizeof(__u16); /* For perf_record_cpu_map_data.type. */
> -	*size = PERF_ALIGN(*size, sizeof(u64));
> -	return zalloc(*size);
> +	size_cpus = sizeof(u16) + sizeof(struct cpu_map_entries) + syn_data->nr * sizeof(u16);
> +	/* Due to padding, the 4bytes per entry mask variant is always smaller. */
> +	size_mask = sizeof(u16) + sizeof(struct perf_record_mask_cpu_map32) +
> +		BITS_TO_U32(syn_data->max_cpu) * sizeof(__u32);
> +	if (syn_data->has_any_cpu || size_cpus < size_mask) {
> +		/* Follow the CPU map encoding. */
> +		syn_data->type = PERF_CPU_MAP__CPUS;
> +		syn_data->size = header_size + PERF_ALIGN(size_cpus, sizeof(u64));
> +		return zalloc(syn_data->size);
> +	}
> +	/* Encode using a bitmask. */
> +	syn_data->type = PERF_CPU_MAP__MASK;
> +	syn_data->size = header_size + PERF_ALIGN(size_mask, sizeof(u64));
> +	return zalloc(syn_data->size);

SNIP