| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 5 Oct 2018 10:03:20 +0300
From: Alexey Budankov <alexey.budankov@...ux.intel.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...nel.org>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...hat.com>, Andi Kleen <ak@...ux.intel.com>,
linux-kernel <linux-kernel@...r.kernel.org>, kernel-team@....com
Subject: Re: [PATCH v9 1/3]: perf util: map data buffer for preserving
collected data
Hi,
On 05.10.2018 9:23, Namhyung Kim wrote:
> On Wed, Oct 03, 2018 at 07:01:26PM +0300, Alexey Budankov wrote:
>>
>> The map->data buffer is used to preserve map->base profiling data
>> for writing to disk. AIO map->cblock is used to queue corresponding
>> map->data buffer for asynchronous writing.
>>
>> Signed-off-by: Alexey Budankov <alexey.budankov@...ux.intel.com>
>> ---
>> Changes in v9:
>> - implemented NO_AIO and HAVE_AIO_SUPPORT defines to cover cases of
>> libc implementations without Posix AIO API support
>> Changes in v7:
>> - implemented handling record.aio setting from perfconfig file
>> Changes in v6:
>> - adjusted setting of priorities for cblocks;
>> Changes in v5:
>> - reshaped layout of data structures;
>> - implemented --aio option;
>> Changes in v4:
>> - converted mmap()/munmap() to malloc()/free() for mmap->data buffer management
>> Changes in v2:
>> - converted zalloc() to calloc() for allocation of mmap_aio array,
>> - cleared typo and adjusted fallback branch code;
>> ---
>> tools/perf/Makefile.config | 5 +++++
>> tools/perf/Makefile.perf | 7 ++++++-
>> tools/perf/util/evlist.c | 4 +++-
>> tools/perf/util/mmap.c | 31 +++++++++++++++++++++++++++++++
>> tools/perf/util/mmap.h | 11 +++++++++++
>> 5 files changed, 56 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
>> index f6d1a03c7523..2e90f4ce9214 100644
>> --- a/tools/perf/Makefile.config
>> +++ b/tools/perf/Makefile.config
>> @@ -355,6 +355,11 @@ endif # NO_LIBELF
>>
>> ifeq ($(feature-glibc), 1)
>> CFLAGS += -DHAVE_GLIBC_SUPPORT
>> + ifndef NO_AIO
>> + ifndef BIONIC
>> + CFLAGS += -DHAVE_AIO_SUPPORT
>> + endif
>> + endif
>> endif
>>
>> ifdef NO_DWARF
>> diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
>> index 92514fb3689f..7becc6a72cf2 100644
>> --- a/tools/perf/Makefile.perf
>> +++ b/tools/perf/Makefile.perf
>> @@ -97,8 +97,13 @@ include ../scripts/utilities.mak
>> # Define LIBCLANGLLVM if you DO want builtin clang and llvm support.
>> # When selected, pass LLVM_CONFIG=/path/to/llvm-config to `make' if
>> # llvm-config is not in $PATH.
>> -
>> +#
>> # Define NO_CORESIGHT if you do not want support for CoreSight trace decoding.
>> +#
>> +# Define NO_AIO if you do not want support of Posix AIO based trace
>> +# streaming for record mode. Currently Posix AIO trace streaming is
>> +# supported only when linking with glibc.
>> +#
>>
>> # As per kernel Makefile, avoid funny character set dependencies
>> unexport LC_ALL
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index be440df29615..af2f8c965d7a 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -1029,7 +1029,9 @@ int perf_evlist__mmap_ex(struct perf_evlist *evlist, unsigned int pages,
>> * So &mp should not be passed through const pointer.
>> */
>> struct mmap_params mp;
>> -
>> +#ifdef HAVE_AIO_SUPPORT
>> + mp.nr_cblocks = 0;
>> +#endif
>> if (!evlist->mmap)
>> evlist->mmap = perf_evlist__alloc_mmap(evlist, false);
>> if (!evlist->mmap)
>> diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
>> index cdb95b3a1213..5b3727b0c7c0 100644
>> --- a/tools/perf/util/mmap.c
>> +++ b/tools/perf/util/mmap.c
>> @@ -155,6 +155,10 @@ void __weak auxtrace_mmap_params__set_idx(struct auxtrace_mmap_params *mp __mayb
>>
>> void perf_mmap__munmap(struct perf_mmap *map)
>> {
>> +#ifdef HAVE_AIO_SUPPORT
>> + if (map->data)
>> + zfree(&map->data);
>> +#endif
>> if (map->base != NULL) {
>> munmap(map->base, perf_mmap__mmap_len(map));
>> map->base = NULL;
>> @@ -166,6 +170,9 @@ void perf_mmap__munmap(struct perf_mmap *map)
>>
>> int perf_mmap__mmap(struct perf_mmap *map, struct mmap_params *mp, int fd, int cpu)
>> {
>> +#ifdef HAVE_AIO_SUPPORT
>> + int delta_max;
>> +#endif
>> /*
>> * The last one will be done at perf_mmap__consume(), so that we
>> * make sure we don't prevent tools from consuming every last event in
>> @@ -190,6 +197,30 @@ int perf_mmap__mmap(struct perf_mmap *map, struct mmap_params *mp, int fd, int c
>> map->base = NULL;
>> return -1;
>> }
>> +#ifdef HAVE_AIO_SUPPORT
>> + map->nr_cblocks = mp->nr_cblocks;
>> + if (map->nr_cblocks) {
>> + map->data = malloc(perf_mmap__mmap_len(map));
>> + if (!map->data) {
>> + pr_debug2("failed to allocate data buffer, error %d\n",
>> + errno);
>
> I guess the error code is not very useful than the message. But in
> this case it's probably ENOMEM so we can simply omit it.
>
>
>> + return -1;
>> + }
>> + /*
>> + * Use cblock.aio_fildes value different from -1
>> + * to denote started aio write operation on the
>> + * cblock so it requires explicit record__aio_sync()
>> + * call prior the cblock may be reused again.
>> + */
>> + map->cblock.aio_fildes = -1;
>> + /*
>> + * Allocate cblock with max priority delta to
>> + * have faster aio write system calls.
>> + */
>> + delta_max = sysconf(_SC_AIO_PRIO_DELTA_MAX);
>> + map->cblock.aio_reqprio = delta_max;
>> + }
>> +#endif
>
> What about separating this part into a function with the #ifdef.
That's possible e.g. like this:
static int perf_mmap__aio_mmap(struct perf_mmap *map, struct mmap_params *mp);
static void perf_mmap__aio_munmap(struct perf_mmap *map);
Thanks,
Alexey
Powered by blists - more mailing lists