Re: [PATCH v2 2/4] perf record: bind the AIO user space buffers to nodes

[Alexey Budankov]

On 09.01.2019 19:49, Jiri Olsa wrote:
> On Wed, Jan 09, 2019 at 12:12:37PM +0300, Alexey Budankov wrote:
>> Hi,
>>
>> On 02.01.2019 0:41, Jiri Olsa wrote:
>>> On Mon, Dec 24, 2018 at 03:24:36PM +0300, Alexey Budankov wrote:
>>>
>>> SNIP
>>>
>>>> +static void perf_mmap__aio_free(void **data, size_t len __maybe_unused)
>>>> +{
>>>> +	zfree(data);
>>>> +}
>>>> +
>>>> +static void perf_mmap__aio_bind(void *data __maybe_unused, size_t len __maybe_unused,
>>>> +                                int cpu __maybe_unused, int affinity __maybe_unused)
>>>> +{
>>>> +}
>>>> +#endif
>>>> +
>>>>  static int perf_mmap__aio_mmap(struct perf_mmap *map, struct mmap_params *mp)
>>>>  {
>>>>  	int delta_max, i, prio;
>>>> @@ -177,11 +220,13 @@ static int perf_mmap__aio_mmap(struct perf_mmap *map, struct mmap_params *mp)
>>>>  		}
>>>>  		delta_max = sysconf(_SC_AIO_PRIO_DELTA_MAX);
>>>>  		for (i = 0; i < map->aio.nr_cblocks; ++i) {
>>>> -			map->aio.data[i] = malloc(perf_mmap__mmap_len(map));
>>>> +			size_t mmap_len = perf_mmap__mmap_len(map);
>>>> +			perf_mmap__aio_alloc(&(map->aio.data[i]), mmap_len);
>>>>  			if (!map->aio.data[i]) {
>>>>  				pr_debug2("failed to allocate data buffer area, error %m");
>>>>  				return -1;
>>>>  			}
>>>> +			perf_mmap__aio_bind(map->aio.data[i], mmap_len, map->cpu, mp->affinity);
>>>
>>> this all does not work if bind fails.. I think we need to
>>> propagate the error value here and fail
>>
>> Proceeding further from this point still makes sense because 
>> the buffer is available for operations and thread migration 
>> alone can bring performance benefits. So the error is not fatal 
>> and an explicit warning is implemented in v3. If you still think 
>> it is better to propagate error from here it can be implemented.
> 
> so if that fails that the aio buffers won't be bound to node,
> while mmaps are, so I guess the speedup is from there?
> 
> if I use:
> 
> # perf record --aio --affinity=node
> 
> and see:
>   "failed to bind..."
> 
> I can still see the benefit..? I guess the warning is ok then,

It still can bring benefits. kernel buffers are allocated locally, 
tool thread migrates to make sure it reads data locally from the buffers.
Even if aio buffers failed to be mapped locally, what is quite rare case,
--affinity=node could still be not slower than without thread migration.

> another option seems confusing

Do you mean --affinity=cpu?

So, well, that cases - bind fail + affinity=node or affinity=cpu - could 
be tested, of course, but it looks simpler and safer to implement error 
reporting and stop, because, again, the cases are quite rare. 
So let's stay on the safer side, in v4. :)

Thanks,
Alexey

> 
> jirka
>