| 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: Thu, 9 Dec 2021 17:15:16 +0100
From: Daniel Bristot de Oliveira <bristot@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Tao Zhou <tao.zhou@...ux.dev>, Ingo Molnar <mingo@...hat.com>,
Tom Zanussi <zanussi@...nel.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Juri Lelli <juri.lelli@...hat.com>,
Clark Williams <williams@...hat.com>,
John Kacur <jkacur@...hat.com>,
Peter Zijlstra <peterz@...radead.org>,
Thomas Gleixner <tglx@...utronix.de>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
linux-rt-users@...r.kernel.org, linux-trace-devel@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH V8 03/14] rtla: Add osnoise tool
On 12/8/21 23:13, Steven Rostedt wrote:
> On Mon, 29 Nov 2021 12:07:41 +0100
> Daniel Bristot de Oliveira <bristot@...nel.org> wrote:
>
>> The osnoise tool is the interface for the osnoise tracer. The osnoise
>> tool will have multiple "modes" with different outputs. At this point,
>> no mode is included.
>>
>> The osnoise.c includes the osnoise_context abstraction. It serves to
>> read-save-change-restore the default values from tracing/osnoise/
>> directory. When the context is deleted, the default values are restored.
>>
>> It also includes some other helper functions for managing osnoise
>> tracer sessions.
>>
>> With these bits and pieces in place, we can start adding some
>> functionality to rtla.
>>
>> Cc: Steven Rostedt <rostedt@...dmis.org>
>> Cc: Ingo Molnar <mingo@...hat.com>
>> Cc: Tom Zanussi <zanussi@...nel.org>
>> Cc: Masami Hiramatsu <mhiramat@...nel.org>
>> Cc: Juri Lelli <juri.lelli@...hat.com>
>> Cc: Clark Williams <williams@...hat.com>
>> Cc: John Kacur <jkacur@...hat.com>
>> Cc: Peter Zijlstra <peterz@...radead.org>
>> Cc: Thomas Gleixner <tglx@...utronix.de>
>> Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
>> Cc: Daniel Bristot de Oliveira <bristot@...nel.org>
>> Cc: linux-rt-users@...r.kernel.org
>> Cc: linux-trace-devel@...r.kernel.org
>> Cc: linux-kernel@...r.kernel.org
>> Signed-off-by: Daniel Bristot de Oliveira <bristot@...nel.org>
>> ---
>> tools/tracing/rtla/Makefile | 2 +
>> tools/tracing/rtla/src/osnoise.c | 1013 ++++++++++++++++++++++++++++++
>> tools/tracing/rtla/src/osnoise.h | 95 +++
>> tools/tracing/rtla/src/rtla.c | 10 +
>> 4 files changed, 1120 insertions(+)
>> create mode 100644 tools/tracing/rtla/src/osnoise.c
>> create mode 100644 tools/tracing/rtla/src/osnoise.h
>>
>> diff --git a/tools/tracing/rtla/Makefile b/tools/tracing/rtla/Makefile
>> index d99ea2d8b87e..ba6f327e815a 100644
>> --- a/tools/tracing/rtla/Makefile
>> +++ b/tools/tracing/rtla/Makefile
>> @@ -60,6 +60,8 @@ install:
>> $(INSTALL) -d -m 755 $(DESTDIR)$(BINDIR)
>> $(INSTALL) rtla -m 755 $(DESTDIR)$(BINDIR)
>> $(STRIP) $(DESTDIR)$(BINDIR)/rtla
>> + @test ! -f $(DESTDIR)$(BINDIR)/osnoise || rm $(DESTDIR)$(BINDIR)/osnoise
>> + ln -s $(DESTDIR)$(BINDIR)/rtla $(DESTDIR)$(BINDIR)/osnoise
>>
>> .PHONY: clean tarball
>> clean:
>> diff --git a/tools/tracing/rtla/src/osnoise.c b/tools/tracing/rtla/src/osnoise.c
>> new file mode 100644
>> index 000000000000..7ef686dddc09
>> --- /dev/null
>> +++ b/tools/tracing/rtla/src/osnoise.c
>> @@ -0,0 +1,1013 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2021 Red Hat Inc, Daniel Bristot de Oliveira <bristot@...nel.org>
>> + */
>> +
>> +#include <sys/types.h>
>> +#include <sys/stat.h>
>> +#include <pthread.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <unistd.h>
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +#include <stdio.h>
>> +
>> +#include "osnoise.h"
>> +#include "utils.h"
>> +
>> +/*
>> + * osnoise_get_cpus - return the original "osnoise/cpus" content
>> + *
>> + * It also saves the value to be restored.
>> + */
>> +char *osnoise_get_cpus(struct osnoise_context *context)
>> +{
>> + char buffer[1024];
>> + char *cpus_path;
>> + int retval;
>> +
>> + if (context->curr_cpus)
>> + return context->curr_cpus;
>> +
>> + if (context->orig_cpus)
>> + return context->orig_cpus;
>> +
>> + cpus_path = tracefs_get_tracing_file("osnoise/cpus");
>> +
>> + context->cpus_fd = open(cpus_path, O_RDWR);
>> + if (context->cpus_fd < 0)
>> + goto out_err;
>> +
>> + retval = read(context->cpus_fd, &buffer, sizeof(buffer));
>> + if (retval <= 0)
>> + goto out_close;
>> +
>> + context->orig_cpus = strdup(buffer);
>
>
> Or you could have done:
>
> context->orig_cpus = tracefs_instance_read(NULL, "osnoise/cpus");
Yep, that would be better.
> as I doubt that reading and writing the cpus file you really care about
> keeping around the file descriptor.
Yep, I do not necessarly need it.... (do not ask me why I am not using, it is
obviosly better... I might have just missed it).
It's not something likely to be done
> where you care about "disrupting" the system. But if you really do care:
>
> context->cpus_fd = tracefs_instance_file_open(NULL, "osnoise/cpus",
> O_RDWR);
I will use tracefs helpers to read/write files, and remove the file descriptor
variables.
[...]
>> + snprintf(buffer, sizeof(buffer), "%llu\n", runtime);
>> +
>> + retval = write(context->runtime_fd, buffer, strlen(buffer) + 1);
>
> Again, how important is it to have the fd?
>
it is not...
[...]
>> +/*
>> + * osnoise_set_stop_total_us - set "stop_tracing_total_us"
>> + */
>> +int osnoise_set_stop_total_us(struct osnoise_context *context, long long stop_total_us)
>> +{
>> + long long curr_stop_total_us = osnoise_get_stop_total_us(context);
>> + char buffer[BUFF_U64_STR_SIZE];
>> + int retval;
>> +
>> + if (curr_stop_total_us == OSNOISE_OPTION_INIT_VAL)
>> + return -1;
>> +
>> + snprintf(buffer, BUFF_U64_STR_SIZE, "%lld\n", stop_total_us);
>> +
>> + retval = write(context->stop_total_us_fd, buffer, strlen(buffer) + 1);
>
> And here.
>
> Hmm, we should add a helper:
>
> tracefs_instance_file_printf(instance, fmt, ...)
>
> Where the above could be:
>
> tracefs_instance_file_printf(NULL, "%lld\n", stop_total_us);
>
> Of course, for now, you could just add a helper function that does that.
sounds good to me.
[...]
>> +/*
>> + * osnoise_context_alloc - alloc an osnoise_context
>> + *
>> + * The osnoise context contains the information of the "osnoise/" configs.
>> + * It is used to set and restore the config.
>> + */
>> +struct osnoise_context *osnoise_context_alloc(void)
>> +{
>> + struct osnoise_context *context;
>> +
>> + context = calloc(1, sizeof(*context));
>> + if (!context)
>> + goto out_err;
>> +
>> + context->cpus_fd = CLOSED_FD;
>> + context->runtime_fd = CLOSED_FD;
>> + context->period_fd = CLOSED_FD;
>> + context->stop_us_fd = CLOSED_FD;
>> + context->stop_total_us_fd = CLOSED_FD;
>> + context->timerlat_period_us_fd = CLOSED_FD;
>> + context->print_stack_fd = CLOSED_FD;
>
> You could save a lot of code by using the tracefs_instance_file_*()
> helpers. And do you really need to keep around the file descriptors?
I will use the helpers in v9, I am convincede... :-)
Thanks Steve
-- Daniel
Powered by blists - more mailing lists