* [PATCH 01/24] perf daemon: Add daemon command
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-01-30 23:48 ` [PATCH 02/24] perf daemon: Add config option Jiri Olsa
` (22 subsequent siblings)
23 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Adding daemon skeleton with minimal base (non) functionality,
covering various setup in start command.
Adding empty perf-daemon.txt to skip compile warning. All the
features and man page are coming in following patches.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/Build | 1 +
tools/perf/Documentation/perf-daemon.txt | 0
tools/perf/builtin-daemon.c | 86 ++++++++++++++++++++++++
tools/perf/builtin.h | 1 +
tools/perf/command-list.txt | 1 +
tools/perf/perf.c | 1 +
6 files changed, 90 insertions(+)
create mode 100644 tools/perf/Documentation/perf-daemon.txt
create mode 100644 tools/perf/builtin-daemon.c
diff --git a/tools/perf/Build b/tools/perf/Build
index 5f392dbb88fc..db61dbe2b543 100644
--- a/tools/perf/Build
+++ b/tools/perf/Build
@@ -24,6 +24,7 @@ perf-y += builtin-mem.o
perf-y += builtin-data.o
perf-y += builtin-version.o
perf-y += builtin-c2c.o
+perf-y += builtin-daemon.o
perf-$(CONFIG_TRACE) += builtin-trace.o
perf-$(CONFIG_LIBELF) += builtin-probe.o
diff --git a/tools/perf/Documentation/perf-daemon.txt b/tools/perf/Documentation/perf-daemon.txt
new file mode 100644
index 000000000000..e69de29bb2d1
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
new file mode 100644
index 000000000000..8b13e455ac40
--- /dev/null
+++ b/tools/perf/builtin-daemon.c
@@ -0,0 +1,86 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <subcmd/parse-options.h>
+#include <linux/limits.h>
+#include <string.h>
+#include <signal.h>
+#include <stdio.h>
+#include <unistd.h>
+#include "builtin.h"
+#include "perf.h"
+#include "debug.h"
+#include "util.h"
+
+struct daemon {
+ char *base;
+ FILE *out;
+ char perf[PATH_MAX];
+};
+
+static struct daemon __daemon = { };
+
+static const char * const daemon_usage[] = {
+ "perf daemon start [<options>]",
+ "perf daemon [<options>]",
+ NULL
+};
+
+static bool done;
+
+static void sig_handler(int sig __maybe_unused)
+{
+ done = true;
+}
+
+static int __cmd_start(struct daemon *daemon, struct option parent_options[],
+ int argc, const char **argv)
+{
+ struct option start_options[] = {
+ OPT_PARENT(parent_options),
+ OPT_END()
+ };
+ int err = 0;
+
+ argc = parse_options(argc, argv, start_options, daemon_usage, 0);
+ if (argc)
+ usage_with_options(daemon_usage, start_options);
+
+ debug_set_file(daemon->out);
+ debug_set_display_time(true);
+
+ pr_info("daemon started (pid %d)\n", getpid());
+
+ signal(SIGINT, sig_handler);
+ signal(SIGTERM, sig_handler);
+
+ while (!done && !err) {
+ sleep(1);
+ }
+
+ pr_info("daemon exited\n");
+ fclose(daemon->out);
+ return err;
+}
+
+int cmd_daemon(int argc, const char **argv)
+{
+ struct option daemon_options[] = {
+ OPT_INCR('v', "verbose", &verbose, "be more verbose"),
+ OPT_END()
+ };
+
+ perf_exe(__daemon.perf, sizeof(__daemon.perf));
+ __daemon.out = stdout;
+
+ argc = parse_options(argc, argv, daemon_options, daemon_usage,
+ PARSE_OPT_STOP_AT_NON_OPTION);
+
+ if (argc) {
+ if (!strcmp(argv[0], "start"))
+ return __cmd_start(&__daemon, daemon_options, argc, argv);
+
+ pr_err("failed: unknown command '%s'\n", argv[0]);
+ return -1;
+ }
+
+ return -1;
+}
diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
index 14a2db622a7b..7303e80a639c 100644
--- a/tools/perf/builtin.h
+++ b/tools/perf/builtin.h
@@ -37,6 +37,7 @@ int cmd_inject(int argc, const char **argv);
int cmd_mem(int argc, const char **argv);
int cmd_data(int argc, const char **argv);
int cmd_ftrace(int argc, const char **argv);
+int cmd_daemon(int argc, const char **argv);
int find_scripts(char **scripts_array, char **scripts_path_array, int num,
int pathlen);
diff --git a/tools/perf/command-list.txt b/tools/perf/command-list.txt
index bc6c585f74fc..825a12e8d694 100644
--- a/tools/perf/command-list.txt
+++ b/tools/perf/command-list.txt
@@ -31,3 +31,4 @@ perf-timechart mainporcelain common
perf-top mainporcelain common
perf-trace mainporcelain audit
perf-version mainporcelain common
+perf-daemon mainporcelain common
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 27f94b0bb874..20cb91ef06ff 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -88,6 +88,7 @@ static struct cmd_struct commands[] = {
{ "mem", cmd_mem, 0 },
{ "data", cmd_data, 0 },
{ "ftrace", cmd_ftrace, 0 },
+ { "daemon", cmd_daemon, 0 },
};
struct pager_config {
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH 02/24] perf daemon: Add config option
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
2021-01-30 23:48 ` [PATCH 01/24] perf daemon: " Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-02-03 20:57 ` Arnaldo Carvalho de Melo
2021-01-30 23:48 ` [PATCH 03/24] perf daemon: Add base option Jiri Olsa
` (21 subsequent siblings)
23 siblings, 1 reply; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Adding config option and base functionality that takes the option
argument (if specified) and other system config locations and
produces 'acting' config file path.
The actual config file processing is coming in following patches.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 49 +++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index 8b13e455ac40..1f2393faad75 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -3,14 +3,18 @@
#include <linux/limits.h>
#include <string.h>
#include <signal.h>
+#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
#include "builtin.h"
#include "perf.h"
#include "debug.h"
+#include "config.h"
#include "util.h"
struct daemon {
+ const char *config;
+ char *config_real;
char *base;
FILE *out;
char perf[PATH_MAX];
@@ -31,6 +35,37 @@ static void sig_handler(int sig __maybe_unused)
done = true;
}
+static void daemon__free(struct daemon *daemon)
+{
+ free(daemon->config_real);
+}
+
+static void daemon__exit(struct daemon *daemon)
+{
+ daemon__free(daemon);
+}
+
+static int setup_config(struct daemon *daemon)
+{
+ if (daemon->config) {
+ char *real = realpath(daemon->config, NULL);
+
+ if (!real) {
+ perror("failed: realpath");
+ return -1;
+ }
+ daemon->config_real = real;
+ return 0;
+ }
+
+ if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK))
+ daemon->config_real = strdup(perf_etc_perfconfig());
+ else if (perf_config_global() && perf_home_perfconfig())
+ daemon->config_real = strdup(perf_home_perfconfig());
+
+ return daemon->config_real ? 0 : -1;
+}
+
static int __cmd_start(struct daemon *daemon, struct option parent_options[],
int argc, const char **argv)
{
@@ -44,6 +79,11 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
if (argc)
usage_with_options(daemon_usage, start_options);
+ if (setup_config(daemon)) {
+ pr_err("failed: config not found\n");
+ return -1;
+ }
+
debug_set_file(daemon->out);
debug_set_display_time(true);
@@ -56,6 +96,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
sleep(1);
}
+ daemon__exit(daemon);
+
pr_info("daemon exited\n");
fclose(daemon->out);
return err;
@@ -65,6 +107,8 @@ int cmd_daemon(int argc, const char **argv)
{
struct option daemon_options[] = {
OPT_INCR('v', "verbose", &verbose, "be more verbose"),
+ OPT_STRING(0, "config", &__daemon.config,
+ "config file", "config file path"),
OPT_END()
};
@@ -82,5 +126,10 @@ int cmd_daemon(int argc, const char **argv)
return -1;
}
+ if (setup_config(&__daemon)) {
+ pr_err("failed: config not found\n");
+ return -1;
+ }
+
return -1;
}
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [PATCH 02/24] perf daemon: Add config option
2021-01-30 23:48 ` [PATCH 02/24] perf daemon: Add config option Jiri Olsa
@ 2021-02-03 20:57 ` Arnaldo Carvalho de Melo
2021-02-04 14:42 ` Jiri Olsa
0 siblings, 1 reply; 72+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-03 20:57 UTC (permalink / raw)
To: Jiri Olsa
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Em Sun, Jan 31, 2021 at 12:48:34AM +0100, Jiri Olsa escreveu:
> Adding config option and base functionality that takes the option
> argument (if specified) and other system config locations and
> produces 'acting' config file path.
>
> The actual config file processing is coming in following patches.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/perf/builtin-daemon.c | 49 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
Missing update to tools/perf/Documentation/perf-daemon.txt for --config
> diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> index 8b13e455ac40..1f2393faad75 100644
> --- a/tools/perf/builtin-daemon.c
> +++ b/tools/perf/builtin-daemon.c
> @@ -3,14 +3,18 @@
> #include <linux/limits.h>
> #include <string.h>
> #include <signal.h>
> +#include <stdlib.h>
> #include <stdio.h>
> #include <unistd.h>
> #include "builtin.h"
> #include "perf.h"
> #include "debug.h"
> +#include "config.h"
> #include "util.h"
>
> struct daemon {
> + const char *config;
> + char *config_real;
> char *base;
> FILE *out;
> char perf[PATH_MAX];
> @@ -31,6 +35,37 @@ static void sig_handler(int sig __maybe_unused)
> done = true;
> }
>
> +static void daemon__free(struct daemon *daemon)
> +{
> + free(daemon->config_real);
> +}
> +
> +static void daemon__exit(struct daemon *daemon)
> +{
> + daemon__free(daemon);
> +}
> +
> +static int setup_config(struct daemon *daemon)
> +{
> + if (daemon->config) {
> + char *real = realpath(daemon->config, NULL);
> +
> + if (!real) {
> + perror("failed: realpath");
> + return -1;
> + }
> + daemon->config_real = real;
> + return 0;
> + }
> +
> + if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK))
> + daemon->config_real = strdup(perf_etc_perfconfig());
> + else if (perf_config_global() && perf_home_perfconfig())
> + daemon->config_real = strdup(perf_home_perfconfig());
> +
> + return daemon->config_real ? 0 : -1;
> +}
> +
> static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> int argc, const char **argv)
> {
> @@ -44,6 +79,11 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> if (argc)
> usage_with_options(daemon_usage, start_options);
>
> + if (setup_config(daemon)) {
> + pr_err("failed: config not found\n");
> + return -1;
> + }
> +
> debug_set_file(daemon->out);
> debug_set_display_time(true);
>
> @@ -56,6 +96,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> sleep(1);
> }
>
> + daemon__exit(daemon);
> +
> pr_info("daemon exited\n");
> fclose(daemon->out);
> return err;
> @@ -65,6 +107,8 @@ int cmd_daemon(int argc, const char **argv)
> {
> struct option daemon_options[] = {
> OPT_INCR('v', "verbose", &verbose, "be more verbose"),
> + OPT_STRING(0, "config", &__daemon.config,
> + "config file", "config file path"),
> OPT_END()
> };
>
> @@ -82,5 +126,10 @@ int cmd_daemon(int argc, const char **argv)
> return -1;
> }
>
> + if (setup_config(&__daemon)) {
> + pr_err("failed: config not found\n");
> + return -1;
> + }
> +
> return -1;
> }
> --
> 2.29.2
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 02/24] perf daemon: Add config option
2021-02-03 20:57 ` Arnaldo Carvalho de Melo
@ 2021-02-04 14:42 ` Jiri Olsa
0 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-02-04 14:42 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jiri Olsa, lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov
On Wed, Feb 03, 2021 at 05:57:02PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Jan 31, 2021 at 12:48:34AM +0100, Jiri Olsa escreveu:
> > Adding config option and base functionality that takes the option
> > argument (if specified) and other system config locations and
> > produces 'acting' config file path.
> >
> > The actual config file processing is coming in following patches.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > tools/perf/builtin-daemon.c | 49 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 49 insertions(+)
>
> Missing update to tools/perf/Documentation/perf-daemon.txt for --config
it's all added in the separate patch with man page
jirka
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH 03/24] perf daemon: Add base option
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
2021-01-30 23:48 ` [PATCH 01/24] perf daemon: " Jiri Olsa
2021-01-30 23:48 ` [PATCH 02/24] perf daemon: Add config option Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-02-03 20:57 ` Arnaldo Carvalho de Melo
2021-01-30 23:48 ` [PATCH 04/24] perf daemon: Add server socket support Jiri Olsa
` (20 subsequent siblings)
23 siblings, 1 reply; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Adding base option allowing user to specify base directory.
It will have precedence over config file base definition
coming in following patches.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index 1f2393faad75..8d0ac44ec808 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -6,6 +6,7 @@
#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
+#include <errno.h>
#include "builtin.h"
#include "perf.h"
#include "debug.h"
@@ -15,6 +16,7 @@
struct daemon {
const char *config;
char *config_real;
+ const char *base_user;
char *base;
FILE *out;
char perf[PATH_MAX];
@@ -38,6 +40,7 @@ static void sig_handler(int sig __maybe_unused)
static void daemon__free(struct daemon *daemon)
{
free(daemon->config_real);
+ free(daemon->base);
}
static void daemon__exit(struct daemon *daemon)
@@ -47,6 +50,12 @@ static void daemon__exit(struct daemon *daemon)
static int setup_config(struct daemon *daemon)
{
+ if (daemon->base_user) {
+ daemon->base = strdup(daemon->base_user);
+ if (!daemon->base)
+ return -ENOMEM;
+ }
+
if (daemon->config) {
char *real = realpath(daemon->config, NULL);
@@ -109,6 +118,8 @@ int cmd_daemon(int argc, const char **argv)
OPT_INCR('v', "verbose", &verbose, "be more verbose"),
OPT_STRING(0, "config", &__daemon.config,
"config file", "config file path"),
+ OPT_STRING(0, "base", &__daemon.base_user,
+ "directory", "base directory"),
OPT_END()
};
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [PATCH 03/24] perf daemon: Add base option
2021-01-30 23:48 ` [PATCH 03/24] perf daemon: Add base option Jiri Olsa
@ 2021-02-03 20:57 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 72+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-03 20:57 UTC (permalink / raw)
To: Jiri Olsa
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Em Sun, Jan 31, 2021 at 12:48:35AM +0100, Jiri Olsa escreveu:
> Adding base option allowing user to specify base directory.
> It will have precedence over config file base definition
> coming in following patches.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/perf/builtin-daemon.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
Missing doc update
> diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> index 1f2393faad75..8d0ac44ec808 100644
> --- a/tools/perf/builtin-daemon.c
> +++ b/tools/perf/builtin-daemon.c
> @@ -6,6 +6,7 @@
> #include <stdlib.h>
> #include <stdio.h>
> #include <unistd.h>
> +#include <errno.h>
> #include "builtin.h"
> #include "perf.h"
> #include "debug.h"
> @@ -15,6 +16,7 @@
> struct daemon {
> const char *config;
> char *config_real;
> + const char *base_user;
> char *base;
> FILE *out;
> char perf[PATH_MAX];
> @@ -38,6 +40,7 @@ static void sig_handler(int sig __maybe_unused)
> static void daemon__free(struct daemon *daemon)
> {
> free(daemon->config_real);
> + free(daemon->base);
> }
>
> static void daemon__exit(struct daemon *daemon)
> @@ -47,6 +50,12 @@ static void daemon__exit(struct daemon *daemon)
>
> static int setup_config(struct daemon *daemon)
> {
> + if (daemon->base_user) {
> + daemon->base = strdup(daemon->base_user);
> + if (!daemon->base)
> + return -ENOMEM;
> + }
> +
> if (daemon->config) {
> char *real = realpath(daemon->config, NULL);
>
> @@ -109,6 +118,8 @@ int cmd_daemon(int argc, const char **argv)
> OPT_INCR('v', "verbose", &verbose, "be more verbose"),
> OPT_STRING(0, "config", &__daemon.config,
> "config file", "config file path"),
> + OPT_STRING(0, "base", &__daemon.base_user,
> + "directory", "base directory"),
> OPT_END()
> };
>
> --
> 2.29.2
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH 04/24] perf daemon: Add server socket support
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (2 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 03/24] perf daemon: Add base option Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-02-03 21:04 ` Arnaldo Carvalho de Melo
2021-02-05 11:30 ` Namhyung Kim
2021-01-30 23:48 ` [PATCH 05/24] perf daemon: Add client " Jiri Olsa
` (19 subsequent siblings)
23 siblings, 2 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Add support to create server socket that listens for client
commands and process them.
This patch adds only the core support, all commands using
this functionality are coming in following patches.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 101 +++++++++++++++++++++++++++++++++++-
1 file changed, 100 insertions(+), 1 deletion(-)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index 8d0ac44ec808..756d60616d7d 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
#include <subcmd/parse-options.h>
+#include <api/fd/array.h>
#include <linux/limits.h>
#include <string.h>
#include <signal.h>
@@ -7,6 +8,10 @@
#include <stdio.h>
#include <unistd.h>
#include <errno.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+#include <poll.h>
#include "builtin.h"
#include "perf.h"
#include "debug.h"
@@ -37,6 +42,78 @@ static void sig_handler(int sig __maybe_unused)
done = true;
}
+static int setup_server_socket(struct daemon *daemon)
+{
+ struct sockaddr_un addr;
+ char path[100];
+ int fd;
+
+ fd = socket(AF_UNIX, SOCK_STREAM, 0);
+ if (fd < 0) {
+ fprintf(stderr, "socket: %s\n", strerror(errno));
+ return -1;
+ }
+
+ fcntl(fd, F_SETFD, FD_CLOEXEC);
+
+ scnprintf(path, PATH_MAX, "%s/control", daemon->base);
+
+ memset(&addr, 0, sizeof(addr));
+ addr.sun_family = AF_UNIX;
+
+ strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1);
+ unlink(path);
+
+ if (bind(fd, (struct sockaddr *)&addr, sizeof(addr)) == -1) {
+ perror("failed: bind");
+ return -1;
+ }
+
+ if (listen(fd, 1) == -1) {
+ perror("failed: listen");
+ return -1;
+ }
+
+ return fd;
+}
+
+union cmd {
+ int cmd;
+};
+
+static int handle_server_socket(struct daemon *daemon __maybe_unused, int sock_fd)
+{
+ int ret = -EINVAL, fd;
+ union cmd cmd;
+ FILE *out;
+
+ fd = accept(sock_fd, NULL, NULL);
+ if (fd < 0) {
+ fprintf(stderr, "accept: %s\n", strerror(errno));
+ return -1;
+ }
+
+ if (sizeof(cmd) != read(fd, &cmd, sizeof(cmd))) {
+ fprintf(stderr, "read: %s\n", strerror(errno));
+ return -1;
+ }
+
+ out = fdopen(fd, "w");
+ if (!out) {
+ perror("failed: fdopen");
+ return -1;
+ }
+
+ switch (cmd.cmd) {
+ default:
+ break;
+ }
+
+ fclose(out);
+ close(fd);
+ return ret;
+}
+
static void daemon__free(struct daemon *daemon)
{
free(daemon->config_real);
@@ -82,6 +159,9 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
OPT_PARENT(parent_options),
OPT_END()
};
+ int sock_fd = -1;
+ int sock_pos;
+ struct fdarray fda;
int err = 0;
argc = parse_options(argc, argv, start_options, daemon_usage, 0);
@@ -98,15 +178,34 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
pr_info("daemon started (pid %d)\n", getpid());
+ fdarray__init(&fda, 1);
+
+ sock_fd = setup_server_socket(daemon);
+ if (sock_fd < 0)
+ goto out;
+
+ sock_pos = fdarray__add(&fda, sock_fd, POLLIN|POLLERR|POLLHUP, 0);
+ if (sock_pos < 0)
+ goto out;
+
signal(SIGINT, sig_handler);
signal(SIGTERM, sig_handler);
while (!done && !err) {
- sleep(1);
+ if (fdarray__poll(&fda, -1)) {
+ if (fda.entries[sock_pos].revents & POLLIN)
+ err = handle_server_socket(daemon, sock_fd);
+ }
}
+out:
+ fdarray__exit(&fda);
+
daemon__exit(daemon);
+ if (sock_fd != -1)
+ close(sock_fd);
+
pr_info("daemon exited\n");
fclose(daemon->out);
return err;
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [PATCH 04/24] perf daemon: Add server socket support
2021-01-30 23:48 ` [PATCH 04/24] perf daemon: Add server socket support Jiri Olsa
@ 2021-02-03 21:04 ` Arnaldo Carvalho de Melo
2021-02-04 14:49 ` Jiri Olsa
2021-02-05 11:30 ` Namhyung Kim
1 sibling, 1 reply; 72+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-03 21:04 UTC (permalink / raw)
To: Jiri Olsa
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Em Sun, Jan 31, 2021 at 12:48:36AM +0100, Jiri Olsa escreveu:
> Add support to create server socket that listens for client
> commands and process them.
>
> This patch adds only the core support, all commands using
> this functionality are coming in following patches.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/perf/builtin-daemon.c | 101 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 100 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> index 8d0ac44ec808..756d60616d7d 100644
> --- a/tools/perf/builtin-daemon.c
> +++ b/tools/perf/builtin-daemon.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> #include <subcmd/parse-options.h>
> +#include <api/fd/array.h>
> #include <linux/limits.h>
> #include <string.h>
> #include <signal.h>
> @@ -7,6 +8,10 @@
> #include <stdio.h>
> #include <unistd.h>
> #include <errno.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <poll.h>
> #include "builtin.h"
> #include "perf.h"
> #include "debug.h"
> @@ -37,6 +42,78 @@ static void sig_handler(int sig __maybe_unused)
> done = true;
> }
>
> +static int setup_server_socket(struct daemon *daemon)
> +{
> + struct sockaddr_un addr;
> + char path[100];
> + int fd;
> +
> + fd = socket(AF_UNIX, SOCK_STREAM, 0);
Minor, combine decl with use, since line isn't long and its one after
the other, i.e.:
int fd = socket(AF_UNIX, SOCK_STREAM, 0);
> + if (fd < 0) {
> + fprintf(stderr, "socket: %s\n", strerror(errno));
> + return -1;
> + }
> +
> + fcntl(fd, F_SETFD, FD_CLOEXEC);
Don't we have to check its return?
> +
> + scnprintf(path, PATH_MAX, "%s/control", daemon->base);
Humm the safe thing here is to use:
scnprintf(path, sizeof(path), "%s/control", daemon->base);
Using it like that would avoid the bug in your code, as path has only
100 bytes, not PATH_MAX bytes ;-)
> +
> + memset(&addr, 0, sizeof(addr));
> + addr.sun_family = AF_UNIX;
> +
> + strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1);
strncpy may end up not adding the final \0 see the NOTES in its man
page. Consider using strlcpy instead. See:
bef0b8970f27da5c ("perf probe: Fix unchecked usage of strncpy()")
> + unlink(path);
> +
> + if (bind(fd, (struct sockaddr *)&addr, sizeof(addr)) == -1) {
> + perror("failed: bind");
> + return -1;
> + }
> +
> + if (listen(fd, 1) == -1) {
> + perror("failed: listen");
> + return -1;
> + }
> +
> + return fd;
> +}
> +
> +union cmd {
> + int cmd;
> +};
> +
> +static int handle_server_socket(struct daemon *daemon __maybe_unused, int sock_fd)
> +{
> + int ret = -EINVAL, fd;
> + union cmd cmd;
> + FILE *out;
> +
> + fd = accept(sock_fd, NULL, NULL);
> + if (fd < 0) {
> + fprintf(stderr, "accept: %s\n", strerror(errno));
> + return -1;
> + }
> +
> + if (sizeof(cmd) != read(fd, &cmd, sizeof(cmd))) {
> + fprintf(stderr, "read: %s\n", strerror(errno));
close fd
> + return -1;
> + }
> +
> + out = fdopen(fd, "w");
> + if (!out) {
> + perror("failed: fdopen");
close fd
I.e. goto out_close;
> + return -1;
> + }
> +
> + switch (cmd.cmd) {
> + default:
> + break;
> + }
> +
> + fclose(out);
out_close:
> + close(fd);
> + return ret;
> +}
> +
> static void daemon__free(struct daemon *daemon)
> {
> free(daemon->config_real);
> @@ -82,6 +159,9 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> OPT_PARENT(parent_options),
> OPT_END()
> };
> + int sock_fd = -1;
> + int sock_pos;
> + struct fdarray fda;
> int err = 0;
>
> argc = parse_options(argc, argv, start_options, daemon_usage, 0);
> @@ -98,15 +178,34 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
>
> pr_info("daemon started (pid %d)\n", getpid());
>
> + fdarray__init(&fda, 1);
> +
> + sock_fd = setup_server_socket(daemon);
> + if (sock_fd < 0)
> + goto out;
> +
> + sock_pos = fdarray__add(&fda, sock_fd, POLLIN|POLLERR|POLLHUP, 0);
> + if (sock_pos < 0)
> + goto out;
> +
> signal(SIGINT, sig_handler);
> signal(SIGTERM, sig_handler);
>
> while (!done && !err) {
> - sleep(1);
> + if (fdarray__poll(&fda, -1)) {
> + if (fda.entries[sock_pos].revents & POLLIN)
> + err = handle_server_socket(daemon, sock_fd);
> + }
> }
>
> +out:
> + fdarray__exit(&fda);
> +
> daemon__exit(daemon);
>
> + if (sock_fd != -1)
> + close(sock_fd);
> +
> pr_info("daemon exited\n");
> fclose(daemon->out);
> return err;
> --
> 2.29.2
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 04/24] perf daemon: Add server socket support
2021-02-03 21:04 ` Arnaldo Carvalho de Melo
@ 2021-02-04 14:49 ` Jiri Olsa
2021-02-04 15:53 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 72+ messages in thread
From: Jiri Olsa @ 2021-02-04 14:49 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jiri Olsa, lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov
On Wed, Feb 03, 2021 at 06:04:23PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Jan 31, 2021 at 12:48:36AM +0100, Jiri Olsa escreveu:
> > Add support to create server socket that listens for client
> > commands and process them.
> >
> > This patch adds only the core support, all commands using
> > this functionality are coming in following patches.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > tools/perf/builtin-daemon.c | 101 +++++++++++++++++++++++++++++++++++-
> > 1 file changed, 100 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> > index 8d0ac44ec808..756d60616d7d 100644
> > --- a/tools/perf/builtin-daemon.c
> > +++ b/tools/perf/builtin-daemon.c
> > @@ -1,5 +1,6 @@
> > // SPDX-License-Identifier: GPL-2.0
> > #include <subcmd/parse-options.h>
> > +#include <api/fd/array.h>
> > #include <linux/limits.h>
> > #include <string.h>
> > #include <signal.h>
> > @@ -7,6 +8,10 @@
> > #include <stdio.h>
> > #include <unistd.h>
> > #include <errno.h>
> > +#include <sys/types.h>
> > +#include <sys/socket.h>
> > +#include <sys/un.h>
> > +#include <poll.h>
> > #include "builtin.h"
> > #include "perf.h"
> > #include "debug.h"
> > @@ -37,6 +42,78 @@ static void sig_handler(int sig __maybe_unused)
> > done = true;
> > }
> >
> > +static int setup_server_socket(struct daemon *daemon)
> > +{
> > + struct sockaddr_un addr;
> > + char path[100];
> > + int fd;
> > +
> > + fd = socket(AF_UNIX, SOCK_STREAM, 0);
>
> Minor, combine decl with use, since line isn't long and its one after
> the other, i.e.:
>
> int fd = socket(AF_UNIX, SOCK_STREAM, 0);
hum, sure, but I'm missing the point.. I think it's less readable
>
> > + if (fd < 0) {
> > + fprintf(stderr, "socket: %s\n", strerror(errno));
> > + return -1;
> > + }
> > +
> > + fcntl(fd, F_SETFD, FD_CLOEXEC);
>
> Don't we have to check its return?
yep, will add
>
> > +
> > + scnprintf(path, PATH_MAX, "%s/control", daemon->base);
>
> Humm the safe thing here is to use:
>
> scnprintf(path, sizeof(path), "%s/control", daemon->base);
>
> Using it like that would avoid the bug in your code, as path has only
> 100 bytes, not PATH_MAX bytes ;-)
right, will change
>
> > +
> > + memset(&addr, 0, sizeof(addr));
> > + addr.sun_family = AF_UNIX;
> > +
> > + strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1);
>
> strncpy may end up not adding the final \0 see the NOTES in its man
> page. Consider using strlcpy instead. See:
>
> bef0b8970f27da5c ("perf probe: Fix unchecked usage of strncpy()")
hum, it's memset-ed to 0 for that an there's -1 in the size,
so I'd think there's zero at the end, but we can use strlcpy
to make it more obvious
SNIP
> > + fprintf(stderr, "accept: %s\n", strerror(errno));
> > + return -1;
> > + }
> > +
> > + if (sizeof(cmd) != read(fd, &cmd, sizeof(cmd))) {
> > + fprintf(stderr, "read: %s\n", strerror(errno));
>
> close fd
>
> > + return -1;
> > + }
> > +
> > + out = fdopen(fd, "w");
> > + if (!out) {
> > + perror("failed: fdopen");
>
> close fd
>
> I.e. goto out_close;
>
> > + return -1;
> > + }
> > +
> > + switch (cmd.cmd) {
> > + default:
> > + break;
> > + }
> > +
> > + fclose(out);
>
> out_close:
>
> > + close(fd);
> > + return ret;
ugh, I overlooked this one
thanks
jirka
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 04/24] perf daemon: Add server socket support
2021-02-04 14:49 ` Jiri Olsa
@ 2021-02-04 15:53 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 72+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-04 15:53 UTC (permalink / raw)
To: Jiri Olsa
Cc: Jiri Olsa, lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov
Em Thu, Feb 04, 2021 at 03:49:36PM +0100, Jiri Olsa escreveu:
> On Wed, Feb 03, 2021 at 06:04:23PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Sun, Jan 31, 2021 at 12:48:36AM +0100, Jiri Olsa escreveu:
> > > +static int setup_server_socket(struct daemon *daemon)
> > > +{
> > > + struct sockaddr_un addr;
> > > + char path[100];
> > > + int fd;
> > > +
> > > + fd = socket(AF_UNIX, SOCK_STREAM, 0);
> >
> > Minor, combine decl with use, since line isn't long and its one after
> > the other, i.e.:
> >
> > int fd = socket(AF_UNIX, SOCK_STREAM, 0);
>
> hum, sure, but I'm missing the point.. I think it's less readable
one line instead of three :-)
> >
> > > + if (fd < 0) {
> > > + fprintf(stderr, "socket: %s\n", strerror(errno));
> > > + return -1;
- Arnaldo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 04/24] perf daemon: Add server socket support
2021-01-30 23:48 ` [PATCH 04/24] perf daemon: Add server socket support Jiri Olsa
2021-02-03 21:04 ` Arnaldo Carvalho de Melo
@ 2021-02-05 11:30 ` Namhyung Kim
2021-02-06 19:04 ` Jiri Olsa
1 sibling, 1 reply; 72+ messages in thread
From: Namhyung Kim @ 2021-02-05 11:30 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, lkml, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov
On Sun, Jan 31, 2021 at 8:49 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Add support to create server socket that listens for client
> commands and process them.
>
> This patch adds only the core support, all commands using
> this functionality are coming in following patches.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/perf/builtin-daemon.c | 101 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 100 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> index 8d0ac44ec808..756d60616d7d 100644
> --- a/tools/perf/builtin-daemon.c
> +++ b/tools/perf/builtin-daemon.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> #include <subcmd/parse-options.h>
> +#include <api/fd/array.h>
> #include <linux/limits.h>
> #include <string.h>
> #include <signal.h>
> @@ -7,6 +8,10 @@
> #include <stdio.h>
> #include <unistd.h>
> #include <errno.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <poll.h>
> #include "builtin.h"
> #include "perf.h"
> #include "debug.h"
> @@ -37,6 +42,78 @@ static void sig_handler(int sig __maybe_unused)
> done = true;
> }
>
> +static int setup_server_socket(struct daemon *daemon)
> +{
> + struct sockaddr_un addr;
> + char path[100];
> + int fd;
> +
> + fd = socket(AF_UNIX, SOCK_STREAM, 0);
> + if (fd < 0) {
> + fprintf(stderr, "socket: %s\n", strerror(errno));
> + return -1;
> + }
> +
> + fcntl(fd, F_SETFD, FD_CLOEXEC);
> +
> + scnprintf(path, PATH_MAX, "%s/control", daemon->base);
I couldn't find where the default value of daemon->base is set.
Also 100 bytes seem not enough for the path name.
Thanks,
Namhyung
> +
> + memset(&addr, 0, sizeof(addr));
> + addr.sun_family = AF_UNIX;
> +
> + strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1);
> + unlink(path);
> +
> + if (bind(fd, (struct sockaddr *)&addr, sizeof(addr)) == -1) {
> + perror("failed: bind");
> + return -1;
> + }
> +
> + if (listen(fd, 1) == -1) {
> + perror("failed: listen");
> + return -1;
> + }
> +
> + return fd;
> +}
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 04/24] perf daemon: Add server socket support
2021-02-05 11:30 ` Namhyung Kim
@ 2021-02-06 19:04 ` Jiri Olsa
2021-02-07 1:36 ` Namhyung Kim
0 siblings, 1 reply; 72+ messages in thread
From: Jiri Olsa @ 2021-02-06 19:04 UTC (permalink / raw)
To: Namhyung Kim
Cc: Jiri Olsa, Arnaldo Carvalho de Melo, lkml, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Michael Petlan,
Ian Rogers, Stephane Eranian, Alexei Budankov
On Fri, Feb 05, 2021 at 08:30:10PM +0900, Namhyung Kim wrote:
> On Sun, Jan 31, 2021 at 8:49 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Add support to create server socket that listens for client
> > commands and process them.
> >
> > This patch adds only the core support, all commands using
> > this functionality are coming in following patches.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > tools/perf/builtin-daemon.c | 101 +++++++++++++++++++++++++++++++++++-
> > 1 file changed, 100 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> > index 8d0ac44ec808..756d60616d7d 100644
> > --- a/tools/perf/builtin-daemon.c
> > +++ b/tools/perf/builtin-daemon.c
> > @@ -1,5 +1,6 @@
> > // SPDX-License-Identifier: GPL-2.0
> > #include <subcmd/parse-options.h>
> > +#include <api/fd/array.h>
> > #include <linux/limits.h>
> > #include <string.h>
> > #include <signal.h>
> > @@ -7,6 +8,10 @@
> > #include <stdio.h>
> > #include <unistd.h>
> > #include <errno.h>
> > +#include <sys/types.h>
> > +#include <sys/socket.h>
> > +#include <sys/un.h>
> > +#include <poll.h>
> > #include "builtin.h"
> > #include "perf.h"
> > #include "debug.h"
> > @@ -37,6 +42,78 @@ static void sig_handler(int sig __maybe_unused)
> > done = true;
> > }
> >
> > +static int setup_server_socket(struct daemon *daemon)
> > +{
> > + struct sockaddr_un addr;
> > + char path[100];
> > + int fd;
> > +
> > + fd = socket(AF_UNIX, SOCK_STREAM, 0);
> > + if (fd < 0) {
> > + fprintf(stderr, "socket: %s\n", strerror(errno));
> > + return -1;
> > + }
> > +
> > + fcntl(fd, F_SETFD, FD_CLOEXEC);
> > +
> > + scnprintf(path, PATH_MAX, "%s/control", daemon->base);
>
> I couldn't find where the default value of daemon->base is set.
> Also 100 bytes seem not enough for the path name.
108 bytes is the limit of the unix socket path,
I'm adding more checks on the provided base,
so we display some reasonable error
thanks,
jirka
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 04/24] perf daemon: Add server socket support
2021-02-06 19:04 ` Jiri Olsa
@ 2021-02-07 1:36 ` Namhyung Kim
0 siblings, 0 replies; 72+ messages in thread
From: Namhyung Kim @ 2021-02-07 1:36 UTC (permalink / raw)
To: Jiri Olsa
Cc: Jiri Olsa, Arnaldo Carvalho de Melo, lkml, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Michael Petlan,
Ian Rogers, Stephane Eranian, Alexei Budankov
On Sun, Feb 7, 2021 at 4:04 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, Feb 05, 2021 at 08:30:10PM +0900, Namhyung Kim wrote:
> > On Sun, Jan 31, 2021 at 8:49 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Add support to create server socket that listens for client
> > > commands and process them.
> > >
> > > This patch adds only the core support, all commands using
> > > this functionality are coming in following patches.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > > tools/perf/builtin-daemon.c | 101 +++++++++++++++++++++++++++++++++++-
> > > 1 file changed, 100 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> > > index 8d0ac44ec808..756d60616d7d 100644
> > > --- a/tools/perf/builtin-daemon.c
> > > +++ b/tools/perf/builtin-daemon.c
> > > @@ -1,5 +1,6 @@
> > > // SPDX-License-Identifier: GPL-2.0
> > > #include <subcmd/parse-options.h>
> > > +#include <api/fd/array.h>
> > > #include <linux/limits.h>
> > > #include <string.h>
> > > #include <signal.h>
> > > @@ -7,6 +8,10 @@
> > > #include <stdio.h>
> > > #include <unistd.h>
> > > #include <errno.h>
> > > +#include <sys/types.h>
> > > +#include <sys/socket.h>
> > > +#include <sys/un.h>
> > > +#include <poll.h>
> > > #include "builtin.h"
> > > #include "perf.h"
> > > #include "debug.h"
> > > @@ -37,6 +42,78 @@ static void sig_handler(int sig __maybe_unused)
> > > done = true;
> > > }
> > >
> > > +static int setup_server_socket(struct daemon *daemon)
> > > +{
> > > + struct sockaddr_un addr;
> > > + char path[100];
> > > + int fd;
> > > +
> > > + fd = socket(AF_UNIX, SOCK_STREAM, 0);
> > > + if (fd < 0) {
> > > + fprintf(stderr, "socket: %s\n", strerror(errno));
> > > + return -1;
> > > + }
> > > +
> > > + fcntl(fd, F_SETFD, FD_CLOEXEC);
> > > +
> > > + scnprintf(path, PATH_MAX, "%s/control", daemon->base);
> >
> > I couldn't find where the default value of daemon->base is set.
> > Also 100 bytes seem not enough for the path name.
>
> 108 bytes is the limit of the unix socket path,
> I'm adding more checks on the provided base,
> so we display some reasonable error
Sgtm.
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH 05/24] perf daemon: Add client socket support
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (3 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 04/24] perf daemon: Add server socket support Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-02-03 21:05 ` Arnaldo Carvalho de Melo
2021-02-05 11:39 ` Namhyung Kim
2021-01-30 23:48 ` [PATCH 06/24] perf daemon: Add config file support Jiri Olsa
` (18 subsequent siblings)
23 siblings, 2 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Adding support for client socket side that will be used
to send commands to daemon server socket.
This patch adds only the core support, all commands using
this functionality are coming in following patches.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 105 ++++++++++++++++++++++++++++++++++++
1 file changed, 105 insertions(+)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index 756d60616d7d..eada3ceb9b0c 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -11,6 +11,7 @@
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/un.h>
+#include <sys/stat.h>
#include <poll.h>
#include "builtin.h"
#include "perf.h"
@@ -42,6 +43,50 @@ static void sig_handler(int sig __maybe_unused)
done = true;
}
+static int client_config(const char *var, const char *value, void *cb)
+{
+ struct daemon *daemon = cb;
+
+ if (!strcmp(var, "daemon.base") && !daemon->base_user) {
+ daemon->base = strdup(value);
+ if (!daemon->base)
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static int check_base(struct daemon *daemon)
+{
+ struct stat st;
+
+ if (!daemon->base) {
+ pr_err("failed: base not defined\n");
+ return -EINVAL;
+ }
+
+ if (stat(daemon->base, &st)) {
+ pr_err("failed: base '%s' does not exists\n", daemon->base);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int setup_client_config(struct daemon *daemon)
+{
+ struct perf_config_set *set;
+ int err = -ENOMEM;
+
+ set = perf_config_set__load_file(daemon->config_real);
+ if (set) {
+ err = perf_config_set(set, client_config, daemon);
+ perf_config_set__delete(set);
+ }
+
+ return err ?: check_base(daemon);
+}
+
static int setup_server_socket(struct daemon *daemon)
{
struct sockaddr_un addr;
@@ -114,6 +159,32 @@ static int handle_server_socket(struct daemon *daemon __maybe_unused, int sock_f
return ret;
}
+static int setup_client_socket(struct daemon *daemon)
+{
+ struct sockaddr_un addr;
+ char path[100];
+ int fd;
+
+ fd = socket(AF_UNIX, SOCK_STREAM, 0);
+ if (fd == -1) {
+ perror("failed: socket");
+ return -1;
+ }
+
+ scnprintf(path, PATH_MAX, "%s/control", daemon->base);
+
+ memset(&addr, 0, sizeof(addr));
+ addr.sun_family = AF_UNIX;
+ strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1);
+
+ if (connect(fd, (struct sockaddr *) &addr, sizeof(addr)) == -1) {
+ perror("failed: connect");
+ return -1;
+ }
+
+ return fd;
+}
+
static void daemon__free(struct daemon *daemon)
{
free(daemon->config_real);
@@ -211,6 +282,40 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
return err;
}
+__maybe_unused
+static int send_cmd(struct daemon *daemon, union cmd *cmd)
+{
+ char *line = NULL;
+ size_t len = 0;
+ ssize_t nread;
+ FILE *in;
+ int fd;
+
+ if (setup_client_config(daemon))
+ return -1;
+
+ fd = setup_client_socket(daemon);
+ if (fd < 0)
+ return -1;
+
+ if (sizeof(*cmd) != write(fd, cmd, sizeof(*cmd)))
+ return -1;
+
+ in = fdopen(fd, "r");
+ if (!in) {
+ perror("failed: fdopen");
+ return -1;
+ }
+
+ while ((nread = getline(&line, &len, in)) != -1) {
+ fwrite(line, nread, 1, stdout);
+ fflush(stdout);
+ }
+
+ fclose(in);
+ return 0;
+}
+
int cmd_daemon(int argc, const char **argv)
{
struct option daemon_options[] = {
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [PATCH 05/24] perf daemon: Add client socket support
2021-01-30 23:48 ` [PATCH 05/24] perf daemon: Add client " Jiri Olsa
@ 2021-02-03 21:05 ` Arnaldo Carvalho de Melo
2021-02-04 14:50 ` Jiri Olsa
2021-02-05 11:39 ` Namhyung Kim
1 sibling, 1 reply; 72+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-03 21:05 UTC (permalink / raw)
To: Jiri Olsa
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Em Sun, Jan 31, 2021 at 12:48:37AM +0100, Jiri Olsa escreveu:
> Adding support for client socket side that will be used
> to send commands to daemon server socket.
>
> This patch adds only the core support, all commands using
> this functionality are coming in following patches.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/perf/builtin-daemon.c | 105 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 105 insertions(+)
>
> diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> index 756d60616d7d..eada3ceb9b0c 100644
> --- a/tools/perf/builtin-daemon.c
> +++ b/tools/perf/builtin-daemon.c
> @@ -11,6 +11,7 @@
> #include <sys/types.h>
> #include <sys/socket.h>
> #include <sys/un.h>
> +#include <sys/stat.h>
> #include <poll.h>
> #include "builtin.h"
> #include "perf.h"
> @@ -42,6 +43,50 @@ static void sig_handler(int sig __maybe_unused)
> done = true;
> }
>
> +static int client_config(const char *var, const char *value, void *cb)
> +{
> + struct daemon *daemon = cb;
> +
> + if (!strcmp(var, "daemon.base") && !daemon->base_user) {
> + daemon->base = strdup(value);
> + if (!daemon->base)
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static int check_base(struct daemon *daemon)
> +{
> + struct stat st;
> +
> + if (!daemon->base) {
> + pr_err("failed: base not defined\n");
> + return -EINVAL;
> + }
> +
> + if (stat(daemon->base, &st)) {
> + pr_err("failed: base '%s' does not exists\n", daemon->base);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int setup_client_config(struct daemon *daemon)
> +{
> + struct perf_config_set *set;
> + int err = -ENOMEM;
> +
> + set = perf_config_set__load_file(daemon->config_real);
struct perf_config_set *set = perf_config_set__load_file(daemon->config_real);
> + if (set) {
> + err = perf_config_set(set, client_config, daemon);
> + perf_config_set__delete(set);
> + }
> +
> + return err ?: check_base(daemon);
> +}
> +
> static int setup_server_socket(struct daemon *daemon)
> {
> struct sockaddr_un addr;
> @@ -114,6 +159,32 @@ static int handle_server_socket(struct daemon *daemon __maybe_unused, int sock_f
> return ret;
> }
>
> +static int setup_client_socket(struct daemon *daemon)
> +{
> + struct sockaddr_un addr;
> + char path[100];
> + int fd;
> +
> + fd = socket(AF_UNIX, SOCK_STREAM, 0);
Ditto
> + if (fd == -1) {
> + perror("failed: socket");
> + return -1;
> + }
> +
> + scnprintf(path, PATH_MAX, "%s/control", daemon->base);
> +
> + memset(&addr, 0, sizeof(addr));
> + addr.sun_family = AF_UNIX;
> + strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1);
> +
> + if (connect(fd, (struct sockaddr *) &addr, sizeof(addr)) == -1) {
> + perror("failed: connect");
close fd
> + return -1;
> + }
> +
> + return fd;
> +}
> +
> static void daemon__free(struct daemon *daemon)
> {
> free(daemon->config_real);
> @@ -211,6 +282,40 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> return err;
> }
>
> +__maybe_unused
> +static int send_cmd(struct daemon *daemon, union cmd *cmd)
> +{
> + char *line = NULL;
> + size_t len = 0;
> + ssize_t nread;
> + FILE *in;
> + int fd;
> +
> + if (setup_client_config(daemon))
> + return -1;
> +
> + fd = setup_client_socket(daemon);
> + if (fd < 0)
> + return -1;
> +
> + if (sizeof(*cmd) != write(fd, cmd, sizeof(*cmd)))
close fd
> + return -1;
> +
> + in = fdopen(fd, "r");
> + if (!in) {
> + perror("failed: fdopen");
close fd
> + return -1;
> + }
> +
> + while ((nread = getline(&line, &len, in)) != -1) {
> + fwrite(line, nread, 1, stdout);
> + fflush(stdout);
> + }
> +
> + fclose(in);
> + return 0;
> +}
> +
> int cmd_daemon(int argc, const char **argv)
> {
> struct option daemon_options[] = {
> --
> 2.29.2
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 05/24] perf daemon: Add client socket support
2021-02-03 21:05 ` Arnaldo Carvalho de Melo
@ 2021-02-04 14:50 ` Jiri Olsa
0 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-02-04 14:50 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jiri Olsa, lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov
On Wed, Feb 03, 2021 at 06:05:47PM -0300, Arnaldo Carvalho de Melo wrote:
SNIP
>
> close fd
>
> > + return -1;
> > + }
> > +
> > + return fd;
> > +}
> > +
> > static void daemon__free(struct daemon *daemon)
> > {
> > free(daemon->config_real);
> > @@ -211,6 +282,40 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> > return err;
> > }
> >
> > +__maybe_unused
> > +static int send_cmd(struct daemon *daemon, union cmd *cmd)
> > +{
> > + char *line = NULL;
> > + size_t len = 0;
> > + ssize_t nread;
> > + FILE *in;
> > + int fd;
> > +
> > + if (setup_client_config(daemon))
> > + return -1;
> > +
> > + fd = setup_client_socket(daemon);
> > + if (fd < 0)
> > + return -1;
> > +
> > + if (sizeof(*cmd) != write(fd, cmd, sizeof(*cmd)))
>
> close fd
>
> > + return -1;
> > +
> > + in = fdopen(fd, "r");
> > + if (!in) {
> > + perror("failed: fdopen");
>
> close fd
ah right, thanks
jirka
>
> > + return -1;
> > + }
> > +
> > + while ((nread = getline(&line, &len, in)) != -1) {
> > + fwrite(line, nread, 1, stdout);
> > + fflush(stdout);
> > + }
> > +
> > + fclose(in);
> > + return 0;
> > +}
> > +
> > int cmd_daemon(int argc, const char **argv)
> > {
> > struct option daemon_options[] = {
> > --
> > 2.29.2
> >
>
> --
>
> - Arnaldo
>
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 05/24] perf daemon: Add client socket support
2021-01-30 23:48 ` [PATCH 05/24] perf daemon: Add client " Jiri Olsa
2021-02-03 21:05 ` Arnaldo Carvalho de Melo
@ 2021-02-05 11:39 ` Namhyung Kim
1 sibling, 0 replies; 72+ messages in thread
From: Namhyung Kim @ 2021-02-05 11:39 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, lkml, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov
On Sun, Jan 31, 2021 at 8:49 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support for client socket side that will be used
> to send commands to daemon server socket.
>
> This patch adds only the core support, all commands using
> this functionality are coming in following patches.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/perf/builtin-daemon.c | 105 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 105 insertions(+)
>
> diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> index 756d60616d7d..eada3ceb9b0c 100644
> --- a/tools/perf/builtin-daemon.c
> +++ b/tools/perf/builtin-daemon.c
> @@ -11,6 +11,7 @@
> #include <sys/types.h>
> #include <sys/socket.h>
> #include <sys/un.h>
> +#include <sys/stat.h>
> #include <poll.h>
> #include "builtin.h"
> #include "perf.h"
> @@ -42,6 +43,50 @@ static void sig_handler(int sig __maybe_unused)
> done = true;
> }
>
> +static int client_config(const char *var, const char *value, void *cb)
> +{
> + struct daemon *daemon = cb;
> +
> + if (!strcmp(var, "daemon.base") && !daemon->base_user) {
> + daemon->base = strdup(value);
> + if (!daemon->base)
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static int check_base(struct daemon *daemon)
> +{
> + struct stat st;
> +
> + if (!daemon->base) {
> + pr_err("failed: base not defined\n");
> + return -EINVAL;
> + }
> +
> + if (stat(daemon->base, &st)) {
> + pr_err("failed: base '%s' does not exists\n", daemon->base);
Well, it could be a permission error or something..
> + return -EINVAL;
> + }
You may also check whether it's a directory.
> +
> + return 0;
> +}
> +
> +static int setup_client_config(struct daemon *daemon)
> +{
> + struct perf_config_set *set;
> + int err = -ENOMEM;
> +
> + set = perf_config_set__load_file(daemon->config_real);
> + if (set) {
> + err = perf_config_set(set, client_config, daemon);
> + perf_config_set__delete(set);
> + }
> +
> + return err ?: check_base(daemon);
> +}
> +
> static int setup_server_socket(struct daemon *daemon)
> {
> struct sockaddr_un addr;
> @@ -114,6 +159,32 @@ static int handle_server_socket(struct daemon *daemon __maybe_unused, int sock_f
> return ret;
> }
>
> +static int setup_client_socket(struct daemon *daemon)
> +{
> + struct sockaddr_un addr;
> + char path[100];
> + int fd;
> +
> + fd = socket(AF_UNIX, SOCK_STREAM, 0);
> + if (fd == -1) {
> + perror("failed: socket");
> + return -1;
> + }
> +
> + scnprintf(path, PATH_MAX, "%s/control", daemon->base);
Same as the previous patch. The path is 100 bytes..
Thanks,
Namhyung
> +
> + memset(&addr, 0, sizeof(addr));
> + addr.sun_family = AF_UNIX;
> + strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1);
> +
> + if (connect(fd, (struct sockaddr *) &addr, sizeof(addr)) == -1) {
> + perror("failed: connect");
> + return -1;
> + }
> +
> + return fd;
> +}
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH 06/24] perf daemon: Add config file support
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (4 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 05/24] perf daemon: Add client " Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-02-03 21:12 ` Arnaldo Carvalho de Melo
` (2 more replies)
2021-01-30 23:48 ` [PATCH 07/24] perf daemon: Add config file change check Jiri Olsa
` (17 subsequent siblings)
23 siblings, 3 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Adding support to configure daemon with config file.
Each client or server invocation of perf daemon needs to know the
base directory, where all sessions data is stored.
The base is defined with:
daemon.base
Base path for daemon data. All sessions data are stored under
this path.
The daemon allows to create record sessions. Each session is a
record command spawned and monitored by perf daemon.
The session is defined with:
session-<NAME>.run
Defines new record session for daemon. The value is record's
command line without the 'record' keyword.
Example:
# cat ~/.perfconfig
[daemon]
base=/opt/perfdata
[session-cycles]
run = -m 10M -e cycles --overwrite --switch-output -a
[session-sched]
run = -m 20M -e sched:* --overwrite --switch-output -a
Example above defines '/opt/perfdata' as a base directory
and 2 record sessions.
# perf daemon start
[2021-01-28 19:47:33.454413] daemon started (pid 16015)
[2021-01-28 19:47:33.455910] reconfig: ruining session [cycles:16016]: -m 10M -e cycles --overwrite --switch-output -a
[2021-01-28 19:47:33.456599] reconfig: ruining session [sched:16017]: -m 20M -e sched:* --overwrite --switch-output -a
# ps -ef | grep perf
... perf daemon start
... /home/jolsa/.../perf record -m 20M -e cycles --overwrite --switch-output -a
... /home/jolsa/.../perf record -m 20M -e sched:* --overwrite --switch-output -a
The base directory is populated with:
# find /opt/perfdata/
/opt/perfdata/
/opt/perfdata/control <- control socket
/opt/perfdata/session-cycles <- data for session 'cycles':
/opt/perfdata/session-cycles/output <- perf record output
/opt/perfdata/session-cycles/perf.data <- perf data
/opt/perfdata/session-sched <- ditto for session 'sched'
/opt/perfdata/session-sched/output
/opt/perfdata/session-sched/perf.data
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 353 +++++++++++++++++++++++++++++++++++-
1 file changed, 351 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index eada3ceb9b0c..be2ade9967b3 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -1,6 +1,8 @@
// SPDX-License-Identifier: GPL-2.0
#include <subcmd/parse-options.h>
#include <api/fd/array.h>
+#include <linux/zalloc.h>
+#include <linux/string.h>
#include <linux/limits.h>
#include <string.h>
#include <signal.h>
@@ -13,22 +15,74 @@
#include <sys/un.h>
#include <sys/stat.h>
#include <poll.h>
+#include <sys/stat.h>
#include "builtin.h"
#include "perf.h"
#include "debug.h"
#include "config.h"
#include "util.h"
+#define SESSION_OUTPUT "output"
+
+/*
+ * Session states:
+ *
+ * OK - session is up and running
+ * RECONFIG - session is pending for reconfiguration,
+ * new values are already loaded in session object
+ * KILL - session is pending to be killed
+ *
+ * Session object life and its state is maintained by
+ * following functions:
+ *
+ * setup_server_config
+ * - reads config file and setup session objects
+ * with following states:
+ *
+ * OK - no change needed
+ * RECONFIG - session needs to be changed
+ * (run variable changed)
+ * KILL - session needs to be killed
+ * (session is no longer in config file)
+ *
+ * daemon__reconfig
+ * - scans session objects and does following actions
+ * for states:
+ *
+ * OK - skip
+ * RECONFIG - session is killed and re-run with new config
+ * KILL - session is killed
+ *
+ * - all sessions have OK state on the function exit
+ */
+enum session_state {
+ SESSION_STATE__OK,
+ SESSION_STATE__RECONFIG,
+ SESSION_STATE__KILL,
+};
+
+struct session {
+ char *base;
+ char *name;
+ char *run;
+ int pid;
+ struct list_head list;
+ enum session_state state;
+};
+
struct daemon {
const char *config;
char *config_real;
const char *base_user;
char *base;
+ struct list_head sessions;
FILE *out;
char perf[PATH_MAX];
};
-static struct daemon __daemon = { };
+static struct daemon __daemon = {
+ .sessions = LIST_HEAD_INIT(__daemon.sessions),
+};
static const char * const daemon_usage[] = {
"perf daemon start [<options>]",
@@ -43,6 +97,128 @@ static void sig_handler(int sig __maybe_unused)
done = true;
}
+static struct session*
+daemon__add_session(struct daemon *config, char *name)
+{
+ struct session *session;
+
+ session = zalloc(sizeof(*session));
+ if (!session)
+ return NULL;
+
+ session->name = strdup(name);
+ if (!session->name) {
+ free(session);
+ return NULL;
+ }
+
+ session->pid = -1;
+ list_add_tail(&session->list, &config->sessions);
+ return session;
+}
+
+static struct session*
+daemon__find_session(struct daemon *daemon, char *name)
+{
+ struct session *session;
+
+ list_for_each_entry(session, &daemon->sessions, list) {
+ if (!strcmp(session->name, name))
+ return session;
+ }
+
+ return NULL;
+}
+
+static int get_session_name(const char *var, char *session, int len)
+{
+ const char *p = var + sizeof("session-") - 1;
+
+ while (*p != '.' && *p != 0x0 && len--)
+ *session++ = *p++;
+
+ *session = 0;
+ return *p == '.' ? 0 : -EINVAL;
+}
+
+static int session_config(struct daemon *daemon, const char *var, const char *value)
+{
+ struct session *session;
+ char name[100];
+
+ if (get_session_name(var, name, sizeof(name)))
+ return -EINVAL;
+
+ var = strchr(var, '.');
+ if (!var)
+ return -EINVAL;
+
+ var++;
+
+ session = daemon__find_session(daemon, name);
+
+ if (!session) {
+ /* New session is defined. */
+ session = daemon__add_session(daemon, name);
+ if (!session)
+ return -ENOMEM;
+
+ pr_debug("reconfig: found new session %s\n", name);
+
+ /* Trigger reconfig to start it. */
+ session->state = SESSION_STATE__RECONFIG;
+ } else if (session->state == SESSION_STATE__KILL) {
+ /* Current session is defined, no action needed. */
+ pr_debug("reconfig: found current session %s\n", name);
+ session->state = SESSION_STATE__OK;
+ }
+
+ if (!strcmp(var, "run")) {
+ bool same = false;
+
+ if (session->run)
+ same = !strcmp(session->run, value);
+
+ if (!same) {
+ if (session->run) {
+ free(session->run);
+ pr_debug("reconfig: session %s is changed\n", name);
+ }
+
+ session->run = strdup(value);
+ if (!session->run)
+ return -ENOMEM;
+
+ /*
+ * Either new or changed run value is defined,
+ * trigger reconfig for the session.
+ */
+ session->state = SESSION_STATE__RECONFIG;
+ }
+ }
+
+ return 0;
+}
+
+static int server_config(const char *var, const char *value, void *cb)
+{
+ struct daemon *daemon = cb;
+
+ if (strstarts(var, "session-"))
+ return session_config(daemon, var, value);
+ else if (!strcmp(var, "daemon.base") && !daemon->base_user) {
+ if (daemon->base && strcmp(daemon->base, value)) {
+ pr_err("failed: can't redefine base, bailing out\n");
+ return -EINVAL;
+ }
+ daemon->base = strdup(value);
+ if (!daemon->base)
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
static int client_config(const char *var, const char *value, void *cb)
{
struct daemon *daemon = cb;
@@ -87,6 +263,91 @@ static int setup_client_config(struct daemon *daemon)
return err ?: check_base(daemon);
}
+static int setup_server_config(struct daemon *daemon)
+{
+ struct perf_config_set *set;
+ struct session *session;
+ int err = -ENOMEM;
+
+ pr_debug("reconfig: started\n");
+
+ /*
+ * Mark all sessions for kill, the server config
+ * will set following states, see explanation at
+ * enum session_state declaration.
+ */
+ list_for_each_entry(session, &daemon->sessions, list)
+ session->state = SESSION_STATE__KILL;
+
+ set = perf_config_set__load_file(daemon->config_real);
+ if (set) {
+ err = perf_config_set(set, server_config, daemon);
+ perf_config_set__delete(set);
+ }
+
+ return err ?: check_base(daemon);
+}
+
+static int session__run(struct session *session, struct daemon *daemon)
+{
+ char buf[PATH_MAX];
+ char **argv;
+ int argc, fd;
+
+ if (asprintf(&session->base, "%s/session-%s",
+ daemon->base, session->name) < 0) {
+ perror("failed: asprintf");
+ return -1;
+ }
+
+ if (mkdir(session->base, 0755) && errno != EEXIST) {
+ perror("failed: mkdir");
+ return -1;
+ }
+
+ session->pid = fork();
+ if (session->pid < 0)
+ return -1;
+ if (session->pid > 0) {
+ pr_info("reconfig: ruining session [%s:%d]: %s\n",
+ session->name, session->pid, session->run);
+ return 0;
+ }
+
+ if (chdir(session->base)) {
+ perror("failed: chdir");
+ return -1;
+ }
+
+ fd = open("/dev/null", O_RDONLY);
+ if (fd < 0) {
+ perror("failed: open /dev/null");
+ return -1;
+ }
+
+ dup2(fd, 0);
+ close(fd);
+
+ fd = open(SESSION_OUTPUT, O_RDWR|O_CREAT|O_TRUNC, 0644);
+ if (fd < 0) {
+ perror("failed: open session output");
+ return -1;
+ }
+
+ dup2(fd, 1);
+ dup2(fd, 2);
+ close(fd);
+
+ scnprintf(buf, sizeof(buf), "%s record %s", daemon->perf, session->run);
+
+ argv = argv_split(buf, &argc);
+ if (!argv)
+ exit(-1);
+
+ exit(execve(daemon->perf, argv, NULL));
+ return -1;
+}
+
static int setup_server_socket(struct daemon *daemon)
{
struct sockaddr_un addr;
@@ -185,17 +446,95 @@ static int setup_client_socket(struct daemon *daemon)
return fd;
}
+static int session__signal(struct session *session, int sig)
+{
+ if (session->pid < 0)
+ return -1;
+ return kill(session->pid, sig);
+}
+
+static void session__kill(struct session *session)
+{
+ session__signal(session, SIGTERM);
+}
+
+static void daemon__signal(struct daemon *daemon, int sig)
+{
+ struct session *session;
+
+ list_for_each_entry(session, &daemon->sessions, list)
+ session__signal(session, sig);
+}
+
+static void session__free(struct session *session)
+{
+ free(session->base);
+ free(session->name);
+ free(session->run);
+ free(session);
+}
+
+static void session__remove(struct session *session)
+{
+ list_del(&session->list);
+ session__free(session);
+}
+
+static void daemon__kill(struct daemon *daemon)
+{
+ daemon__signal(daemon, SIGTERM);
+}
+
static void daemon__free(struct daemon *daemon)
{
+ struct session *session, *h;
+
+ list_for_each_entry_safe(session, h, &daemon->sessions, list)
+ session__remove(session);
+
free(daemon->config_real);
free(daemon->base);
}
static void daemon__exit(struct daemon *daemon)
{
+ daemon__kill(daemon);
daemon__free(daemon);
}
+static int daemon__reconfig(struct daemon *daemon)
+{
+ struct session *session, *n;
+
+ list_for_each_entry_safe(session, n, &daemon->sessions, list) {
+ /* No change. */
+ if (session->state == SESSION_STATE__OK)
+ continue;
+
+ /* Remove session. */
+ if (session->state == SESSION_STATE__KILL) {
+ if (session->pid > 0) {
+ session__kill(session);
+ pr_info("reconfig: session '%s' killed\n", session->name);
+ }
+ session__remove(session);
+ continue;
+ }
+
+ /* Reconfig session. */
+ if (session->pid > 0) {
+ session__kill(session);
+ pr_info("reconfig: session '%s' killed\n", session->name);
+ }
+ if (session__run(session, daemon))
+ return -1;
+
+ session->state = SESSION_STATE__OK;
+ }
+
+ return 0;
+}
+
static int setup_config(struct daemon *daemon)
{
if (daemon->base_user) {
@@ -244,6 +583,9 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
return -1;
}
+ if (setup_server_config(daemon))
+ return -1;
+
debug_set_file(daemon->out);
debug_set_display_time(true);
@@ -263,9 +605,16 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
signal(SIGTERM, sig_handler);
while (!done && !err) {
- if (fdarray__poll(&fda, -1)) {
+ err = daemon__reconfig(daemon);
+
+ if (!err && fdarray__poll(&fda, -1)) {
+ bool reconfig = false;
+
if (fda.entries[sock_pos].revents & POLLIN)
err = handle_server_socket(daemon, sock_fd);
+
+ if (reconfig)
+ err = setup_server_config(daemon);
}
}
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [PATCH 06/24] perf daemon: Add config file support
2021-01-30 23:48 ` [PATCH 06/24] perf daemon: Add config file support Jiri Olsa
@ 2021-02-03 21:12 ` Arnaldo Carvalho de Melo
2021-02-04 15:08 ` Jiri Olsa
2021-02-04 12:42 ` Namhyung Kim
2021-02-05 12:14 ` Namhyung Kim
2 siblings, 1 reply; 72+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-03 21:12 UTC (permalink / raw)
To: Jiri Olsa
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Em Sun, Jan 31, 2021 at 12:48:38AM +0100, Jiri Olsa escreveu:
> Adding support to configure daemon with config file.
>
> Each client or server invocation of perf daemon needs to know the
> base directory, where all sessions data is stored.
>
> The base is defined with:
>
> daemon.base
> Base path for daemon data. All sessions data are stored under
> this path.
>
> The daemon allows to create record sessions. Each session is a
> record command spawned and monitored by perf daemon.
>
> The session is defined with:
>
> session-<NAME>.run
> Defines new record session for daemon. The value is record's
> command line without the 'record' keyword.
>
> Example:
>
> # cat ~/.perfconfig
> [daemon]
> base=/opt/perfdata
>
> [session-cycles]
> run = -m 10M -e cycles --overwrite --switch-output -a
>
> [session-sched]
> run = -m 20M -e sched:* --overwrite --switch-output -a
>
> Example above defines '/opt/perfdata' as a base directory
> and 2 record sessions.
>
> # perf daemon start
> [2021-01-28 19:47:33.454413] daemon started (pid 16015)
> [2021-01-28 19:47:33.455910] reconfig: ruining session [cycles:16016]: -m 10M -e cycles --overwrite --switch-output -a
> [2021-01-28 19:47:33.456599] reconfig: ruining session [sched:16017]: -m 20M -e sched:* --overwrite --switch-output -a
>
> # ps -ef | grep perf
> ... perf daemon start
> ... /home/jolsa/.../perf record -m 20M -e cycles --overwrite --switch-output -a
> ... /home/jolsa/.../perf record -m 20M -e sched:* --overwrite --switch-output -a
>
> The base directory is populated with:
>
> # find /opt/perfdata/
> /opt/perfdata/
> /opt/perfdata/control <- control socket
> /opt/perfdata/session-cycles <- data for session 'cycles':
> /opt/perfdata/session-cycles/output <- perf record output
> /opt/perfdata/session-cycles/perf.data <- perf data
> /opt/perfdata/session-sched <- ditto for session 'sched'
> /opt/perfdata/session-sched/output
> /opt/perfdata/session-sched/perf.data
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/perf/builtin-daemon.c | 353 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 351 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> index eada3ceb9b0c..be2ade9967b3 100644
> --- a/tools/perf/builtin-daemon.c
> +++ b/tools/perf/builtin-daemon.c
> @@ -1,6 +1,8 @@
> // SPDX-License-Identifier: GPL-2.0
> #include <subcmd/parse-options.h>
> #include <api/fd/array.h>
> +#include <linux/zalloc.h>
> +#include <linux/string.h>
> #include <linux/limits.h>
> #include <string.h>
> #include <signal.h>
> @@ -13,22 +15,74 @@
> #include <sys/un.h>
> #include <sys/stat.h>
> #include <poll.h>
> +#include <sys/stat.h>
> #include "builtin.h"
> #include "perf.h"
> #include "debug.h"
> #include "config.h"
> #include "util.h"
>
> +#define SESSION_OUTPUT "output"
> +
> +/*
> + * Session states:
> + *
> + * OK - session is up and running
> + * RECONFIG - session is pending for reconfiguration,
> + * new values are already loaded in session object
> + * KILL - session is pending to be killed
> + *
> + * Session object life and its state is maintained by
> + * following functions:
> + *
> + * setup_server_config
> + * - reads config file and setup session objects
> + * with following states:
> + *
> + * OK - no change needed
> + * RECONFIG - session needs to be changed
> + * (run variable changed)
> + * KILL - session needs to be killed
> + * (session is no longer in config file)
> + *
> + * daemon__reconfig
> + * - scans session objects and does following actions
> + * for states:
> + *
> + * OK - skip
> + * RECONFIG - session is killed and re-run with new config
> + * KILL - session is killed
> + *
> + * - all sessions have OK state on the function exit
> + */
> +enum session_state {
> + SESSION_STATE__OK,
> + SESSION_STATE__RECONFIG,
> + SESSION_STATE__KILL,
> +};
> +
> +struct session {
> + char *base;
> + char *name;
> + char *run;
> + int pid;
> + struct list_head list;
> + enum session_state state;
> +};
> +
> struct daemon {
> const char *config;
> char *config_real;
> const char *base_user;
> char *base;
> + struct list_head sessions;
> FILE *out;
> char perf[PATH_MAX];
> };
>
> -static struct daemon __daemon = { };
> +static struct daemon __daemon = {
> + .sessions = LIST_HEAD_INIT(__daemon.sessions),
> +};
>
> static const char * const daemon_usage[] = {
> "perf daemon start [<options>]",
> @@ -43,6 +97,128 @@ static void sig_handler(int sig __maybe_unused)
> done = true;
> }
>
> +static struct session*
> +daemon__add_session(struct daemon *config, char *name)
> +{
> + struct session *session;
> +
> + session = zalloc(sizeof(*session));
struct session *session = zalloc(sizeof(*session));
> + if (!session)
> + return NULL;
> +
> + session->name = strdup(name);
> + if (!session->name) {
> + free(session);
> + return NULL;
> + }
> +
> + session->pid = -1;
> + list_add_tail(&session->list, &config->sessions);
> + return session;
> +}
> +
> +static struct session*
add space after type name
static struct session *
And we could have it in the same line:
static struct session *daemon__find_session(struct daemon *daemon, char *name)
> +daemon__find_session(struct daemon *daemon, char *name)
> +{
> + struct session *session;
> +
> + list_for_each_entry(session, &daemon->sessions, list) {
> + if (!strcmp(session->name, name))
> + return session;
> + }
> +
> + return NULL;
> +}
> +
> +static int get_session_name(const char *var, char *session, int len)
> +{
> + const char *p = var + sizeof("session-") - 1;
> +
> + while (*p != '.' && *p != 0x0 && len--)
> + *session++ = *p++;
> +
> + *session = 0;
> + return *p == '.' ? 0 : -EINVAL;
> +}
> +
> +static int session_config(struct daemon *daemon, const char *var, const char *value)
> +{
> + struct session *session;
> + char name[100];
> +
> + if (get_session_name(var, name, sizeof(name)))
Good , using sizeof () :)
> + return -EINVAL;
> +
> + var = strchr(var, '.');
> + if (!var)
> + return -EINVAL;
> +
> + var++;
> +
> + session = daemon__find_session(daemon, name);
> +
> + if (!session) {
> + /* New session is defined. */
> + session = daemon__add_session(daemon, name);
> + if (!session)
> + return -ENOMEM;
> +
> + pr_debug("reconfig: found new session %s\n", name);
> +
> + /* Trigger reconfig to start it. */
> + session->state = SESSION_STATE__RECONFIG;
> + } else if (session->state == SESSION_STATE__KILL) {
> + /* Current session is defined, no action needed. */
> + pr_debug("reconfig: found current session %s\n", name);
> + session->state = SESSION_STATE__OK;
> + }
> +
> + if (!strcmp(var, "run")) {
> + bool same = false;
> +
> + if (session->run)
> + same = !strcmp(session->run, value);
> +
> + if (!same) {
> + if (session->run) {
> + free(session->run);
> + pr_debug("reconfig: session %s is changed\n", name);
> + }
> +
> + session->run = strdup(value);
> + if (!session->run)
> + return -ENOMEM;
> +
> + /*
> + * Either new or changed run value is defined,
> + * trigger reconfig for the session.
> + */
> + session->state = SESSION_STATE__RECONFIG;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int server_config(const char *var, const char *value, void *cb)
> +{
> + struct daemon *daemon = cb;
> +
> + if (strstarts(var, "session-"))
> + return session_config(daemon, var, value);
> + else if (!strcmp(var, "daemon.base") && !daemon->base_user) {
if else uses {}, if should too
> + if (daemon->base && strcmp(daemon->base, value)) {
> + pr_err("failed: can't redefine base, bailing out\n");
> + return -EINVAL;
> + }
> + daemon->base = strdup(value);
> + if (!daemon->base)
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> static int client_config(const char *var, const char *value, void *cb)
> {
> struct daemon *daemon = cb;
> @@ -87,6 +263,91 @@ static int setup_client_config(struct daemon *daemon)
> return err ?: check_base(daemon);
> }
>
> +static int setup_server_config(struct daemon *daemon)
> +{
> + struct perf_config_set *set;
> + struct session *session;
> + int err = -ENOMEM;
> +
> + pr_debug("reconfig: started\n");
> +
> + /*
> + * Mark all sessions for kill, the server config
> + * will set following states, see explanation at
> + * enum session_state declaration.
> + */
> + list_for_each_entry(session, &daemon->sessions, list)
> + session->state = SESSION_STATE__KILL;
> +
> + set = perf_config_set__load_file(daemon->config_real);
> + if (set) {
> + err = perf_config_set(set, server_config, daemon);
> + perf_config_set__delete(set);
> + }
> +
> + return err ?: check_base(daemon);
> +}
> +
> +static int session__run(struct session *session, struct daemon *daemon)
> +{
> + char buf[PATH_MAX];
> + char **argv;
> + int argc, fd;
> +
> + if (asprintf(&session->base, "%s/session-%s",
> + daemon->base, session->name) < 0) {
> + perror("failed: asprintf");
> + return -1;
> + }
> +
> + if (mkdir(session->base, 0755) && errno != EEXIST) {
> + perror("failed: mkdir");
> + return -1;
> + }
> +
> + session->pid = fork();
> + if (session->pid < 0)
> + return -1;
> + if (session->pid > 0) {
> + pr_info("reconfig: ruining session [%s:%d]: %s\n",
> + session->name, session->pid, session->run);
> + return 0;
> + }
> +
> + if (chdir(session->base)) {
> + perror("failed: chdir");
> + return -1;
> + }
> +
> + fd = open("/dev/null", O_RDONLY);
> + if (fd < 0) {
> + perror("failed: open /dev/null");
> + return -1;
> + }
> +
> + dup2(fd, 0);
> + close(fd);
> +
> + fd = open(SESSION_OUTPUT, O_RDWR|O_CREAT|O_TRUNC, 0644);
> + if (fd < 0) {
> + perror("failed: open session output");
> + return -1;
> + }
> +
> + dup2(fd, 1);
> + dup2(fd, 2);
> + close(fd);
> +
> + scnprintf(buf, sizeof(buf), "%s record %s", daemon->perf, session->run);
> +
> + argv = argv_split(buf, &argc);
> + if (!argv)
> + exit(-1);
> +
> + exit(execve(daemon->perf, argv, NULL));
> + return -1;
> +}
> +
> static int setup_server_socket(struct daemon *daemon)
> {
> struct sockaddr_un addr;
> @@ -185,17 +446,95 @@ static int setup_client_socket(struct daemon *daemon)
> return fd;
> }
>
> +static int session__signal(struct session *session, int sig)
> +{
> + if (session->pid < 0)
> + return -1;
> + return kill(session->pid, sig);
> +}
> +
> +static void session__kill(struct session *session)
> +{
> + session__signal(session, SIGTERM);
> +}
> +
> +static void daemon__signal(struct daemon *daemon, int sig)
> +{
> + struct session *session;
> +
> + list_for_each_entry(session, &daemon->sessions, list)
> + session__signal(session, sig);
> +}
> +
> +static void session__free(struct session *session)
> +{
> + free(session->base);
> + free(session->name);
> + free(session->run);
zfree() so that if there is some dangling pointer to session, we'll get
NULL derefs
> + free(session);
> +}
> +
> +static void session__remove(struct session *session)
> +{
> + list_del(&session->list);
list_del_init
> + session__free(session);
> +}
> +
> +static void daemon__kill(struct daemon *daemon)
> +{
> + daemon__signal(daemon, SIGTERM);
> +}
> +
> static void daemon__free(struct daemon *daemon)
> {
> + struct session *session, *h;
> +
> + list_for_each_entry_safe(session, h, &daemon->sessions, list)
> + session__remove(session);
Wouldn't be better to have:
list_for_each_entry_safe(session, h, &daemon->sessions, list) {
list_del_init(&session->list);
session__free(session);
}
Because naming that function "session__remove()" one thinks it is being
removed from some data structure, but not that it is being as well
deleted.
Please rename session__free() to session__delete() to keep it consistent
with other places.
> +
> free(daemon->config_real);
> free(daemon->base);
> }
>
> static void daemon__exit(struct daemon *daemon)
> {
> + daemon__kill(daemon);
> daemon__free(daemon);
Ditto for daemon__free(): please rename it to daemon__delete()
> }
>
> +static int daemon__reconfig(struct daemon *daemon)
> +{
> + struct session *session, *n;
> +
> + list_for_each_entry_safe(session, n, &daemon->sessions, list) {
> + /* No change. */
> + if (session->state == SESSION_STATE__OK)
> + continue;
> +
> + /* Remove session. */
> + if (session->state == SESSION_STATE__KILL) {
> + if (session->pid > 0) {
> + session__kill(session);
> + pr_info("reconfig: session '%s' killed\n", session->name);
> + }
> + session__remove(session);
> + continue;
> + }
> +
> + /* Reconfig session. */
> + if (session->pid > 0) {
> + session__kill(session);
> + pr_info("reconfig: session '%s' killed\n", session->name);
> + }
> + if (session__run(session, daemon))
> + return -1;
> +
> + session->state = SESSION_STATE__OK;
> + }
> +
> + return 0;
> +}
> +
> static int setup_config(struct daemon *daemon)
> {
> if (daemon->base_user) {
> @@ -244,6 +583,9 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> return -1;
> }
>
> + if (setup_server_config(daemon))
> + return -1;
> +
> debug_set_file(daemon->out);
> debug_set_display_time(true);
>
> @@ -263,9 +605,16 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> signal(SIGTERM, sig_handler);
>
> while (!done && !err) {
> - if (fdarray__poll(&fda, -1)) {
> + err = daemon__reconfig(daemon);
> +
> + if (!err && fdarray__poll(&fda, -1)) {
> + bool reconfig = false;
> +
> if (fda.entries[sock_pos].revents & POLLIN)
> err = handle_server_socket(daemon, sock_fd);
> +
> + if (reconfig)
> + err = setup_server_config(daemon);
> }
> }
>
> --
> 2.29.2
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 06/24] perf daemon: Add config file support
2021-02-03 21:12 ` Arnaldo Carvalho de Melo
@ 2021-02-04 15:08 ` Jiri Olsa
2021-02-06 20:25 ` Jiri Olsa
0 siblings, 1 reply; 72+ messages in thread
From: Jiri Olsa @ 2021-02-04 15:08 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jiri Olsa, lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov
On Wed, Feb 03, 2021 at 06:12:11PM -0300, Arnaldo Carvalho de Melo wrote:
SNIP
> > -static struct daemon __daemon = { };
> > +static struct daemon __daemon = {
> > + .sessions = LIST_HEAD_INIT(__daemon.sessions),
> > +};
> >
> > static const char * const daemon_usage[] = {
> > "perf daemon start [<options>]",
> > @@ -43,6 +97,128 @@ static void sig_handler(int sig __maybe_unused)
> > done = true;
> > }
> >
> > +static struct session*
> > +daemon__add_session(struct daemon *config, char *name)
> > +{
> > + struct session *session;
> > +
> > + session = zalloc(sizeof(*session));
>
>
> struct session *session = zalloc(sizeof(*session));
>
> > + if (!session)
> > + return NULL;
> > +
> > + session->name = strdup(name);
> > + if (!session->name) {
> > + free(session);
> > + return NULL;
> > + }
> > +
> > + session->pid = -1;
> > + list_add_tail(&session->list, &config->sessions);
> > + return session;
> > +}
> > +
> > +static struct session*
>
> add space after type name
>
> static struct session *
>
> And we could have it in the same line:
>
> static struct session *daemon__find_session(struct daemon *daemon, char *name)
ok
SNIP
> > + /*
> > + * Either new or changed run value is defined,
> > + * trigger reconfig for the session.
> > + */
> > + session->state = SESSION_STATE__RECONFIG;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int server_config(const char *var, const char *value, void *cb)
> > +{
> > + struct daemon *daemon = cb;
> > +
> > + if (strstarts(var, "session-"))
> > + return session_config(daemon, var, value);
> > + else if (!strcmp(var, "daemon.base") && !daemon->base_user) {
>
> if else uses {}, if should too
ok
SNIP
> > +
> > +static void session__free(struct session *session)
> > +{
> > + free(session->base);
> > + free(session->name);
> > + free(session->run);
>
> zfree() so that if there is some dangling pointer to session, we'll get
> NULL derefs
and won't be notified by crash about the error ;-) ok
>
> > + free(session);
> > +}
> > +
> > +static void session__remove(struct session *session)
> > +{
> > + list_del(&session->list);
>
> list_del_init
>
> > + session__free(session);
> > +}
> > +
> > +static void daemon__kill(struct daemon *daemon)
> > +{
> > + daemon__signal(daemon, SIGTERM);
> > +}
> > +
> > static void daemon__free(struct daemon *daemon)
> > {
> > + struct session *session, *h;
> > +
> > + list_for_each_entry_safe(session, h, &daemon->sessions, list)
> > + session__remove(session);
>
> Wouldn't be better to have:
>
> list_for_each_entry_safe(session, h, &daemon->sessions, list) {
> list_del_init(&session->list);
> session__free(session);
> }
>
> Because naming that function "session__remove()" one thinks it is being
> removed from some data structure, but not that it is being as well
> deleted.
>
> Please rename session__free() to session__delete() to keep it consistent
> with other places.
ok
>
> > +
> > free(daemon->config_real);
> > free(daemon->base);
> > }
> >
> > static void daemon__exit(struct daemon *daemon)
> > {
> > + daemon__kill(daemon);
> > daemon__free(daemon);
>
> Ditto for daemon__free(): please rename it to daemon__delete()
ok
thanks,
jirka
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 06/24] perf daemon: Add config file support
2021-02-04 15:08 ` Jiri Olsa
@ 2021-02-06 20:25 ` Jiri Olsa
0 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-02-06 20:25 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jiri Olsa, lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov
On Thu, Feb 04, 2021 at 04:08:50PM +0100, Jiri Olsa wrote:
SNIP
> > > +
> > > +static void session__free(struct session *session)
> > > +{
> > > + free(session->base);
> > > + free(session->name);
> > > + free(session->run);
> >
> > zfree() so that if there is some dangling pointer to session, we'll get
> > NULL derefs
>
> and won't be notified by crash about the error ;-) ok
oops, actualy it makes no sense to do it here, because we're
freeing session just in the next line
>
> >
> > > + free(session);
> > > +}
> > > +
> > > +static void session__remove(struct session *session)
> > > +{
> > > + list_del(&session->list);
> >
> > list_del_init
same here
> >
> > > + session__free(session);
> > > +}
> > > +
> > > +static void daemon__kill(struct daemon *daemon)
> > > +{
> > > + daemon__signal(daemon, SIGTERM);
> > > +}
> > > +
> > > static void daemon__free(struct daemon *daemon)
> > > {
> > > + struct session *session, *h;
> > > +
> > > + list_for_each_entry_safe(session, h, &daemon->sessions, list)
> > > + session__remove(session);
> >
> > Wouldn't be better to have:
> >
> > list_for_each_entry_safe(session, h, &daemon->sessions, list) {
> > list_del_init(&session->list);
> > session__free(session);
> > }
> >
> > Because naming that function "session__remove()" one thinks it is being
> > removed from some data structure, but not that it is being as well
> > deleted.
session__remove is being called also from daemon__reconfig,
so it's there not to repeat the code, I'm ok to rename it
thanks,
jirka
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 06/24] perf daemon: Add config file support
2021-01-30 23:48 ` [PATCH 06/24] perf daemon: Add config file support Jiri Olsa
2021-02-03 21:12 ` Arnaldo Carvalho de Melo
@ 2021-02-04 12:42 ` Namhyung Kim
2021-02-04 12:58 ` Arnaldo Carvalho de Melo
2021-02-05 12:14 ` Namhyung Kim
2 siblings, 1 reply; 72+ messages in thread
From: Namhyung Kim @ 2021-02-04 12:42 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, lkml, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov
Hi Jiri,
On Sun, Jan 31, 2021 at 8:49 AM Jiri Olsa <jolsa@kernel.org> wrote:
[SNIP]
> +#define SESSION_OUTPUT "output"
> +
> +/*
> + * Session states:
> + *
> + * OK - session is up and running
> + * RECONFIG - session is pending for reconfiguration,
> + * new values are already loaded in session object
> + * KILL - session is pending to be killed
> + *
> + * Session object life and its state is maintained by
> + * following functions:
> + *
> + * setup_server_config
> + * - reads config file and setup session objects
> + * with following states:
> + *
> + * OK - no change needed
> + * RECONFIG - session needs to be changed
> + * (run variable changed)
> + * KILL - session needs to be killed
> + * (session is no longer in config file)
> + *
> + * daemon__reconfig
> + * - scans session objects and does following actions
> + * for states:
> + *
> + * OK - skip
> + * RECONFIG - session is killed and re-run with new config
> + * KILL - session is killed
> + *
> + * - all sessions have OK state on the function exit
> + */
> +enum session_state {
> + SESSION_STATE__OK,
> + SESSION_STATE__RECONFIG,
> + SESSION_STATE__KILL,
> +};
> +
> +struct session {
> + char *base;
> + char *name;
> + char *run;
> + int pid;
> + struct list_head list;
> + enum session_state state;
> +};
Although I think calling it 'session' is intuitive, it's also confusing
as we already have struct perf_session...
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 06/24] perf daemon: Add config file support
2021-02-04 12:42 ` Namhyung Kim
@ 2021-02-04 12:58 ` Arnaldo Carvalho de Melo
2021-02-04 15:10 ` Jiri Olsa
0 siblings, 1 reply; 72+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-04 12:58 UTC (permalink / raw)
To: Namhyung Kim
Cc: Jiri Olsa, lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Em Thu, Feb 04, 2021 at 09:42:35PM +0900, Namhyung Kim escreveu:
> Hi Jiri,
>
> On Sun, Jan 31, 2021 at 8:49 AM Jiri Olsa <jolsa@kernel.org> wrote:
> [SNIP]
> > +#define SESSION_OUTPUT "output"
> > +
> > +/*
> > + * Session states:
> > + *
> > + * OK - session is up and running
> > + * RECONFIG - session is pending for reconfiguration,
> > + * new values are already loaded in session object
> > + * KILL - session is pending to be killed
> > + *
> > + * Session object life and its state is maintained by
> > + * following functions:
> > + *
> > + * setup_server_config
> > + * - reads config file and setup session objects
> > + * with following states:
> > + *
> > + * OK - no change needed
> > + * RECONFIG - session needs to be changed
> > + * (run variable changed)
> > + * KILL - session needs to be killed
> > + * (session is no longer in config file)
> > + *
> > + * daemon__reconfig
> > + * - scans session objects and does following actions
> > + * for states:
> > + *
> > + * OK - skip
> > + * RECONFIG - session is killed and re-run with new config
> > + * KILL - session is killed
> > + *
> > + * - all sessions have OK state on the function exit
> > + */
> > +enum session_state {
> > + SESSION_STATE__OK,
> > + SESSION_STATE__RECONFIG,
> > + SESSION_STATE__KILL,
> > +};
> > +
> > +struct session {
> > + char *base;
> > + char *name;
> > + char *run;
> > + int pid;
> > + struct list_head list;
> > + enum session_state state;
> > +};
>
> Although I think calling it 'session' is intuitive, it's also confusing
> as we already have struct perf_session...
Maybe 'struct server_session' ? If this ends up in tools/lib/perf, then
it gets renamed to 'struct perf_server_session', just like we have
'struct perf_evsel' in libperf and 'struct evsel' in tools/perf/, right?
- Arnaldo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 06/24] perf daemon: Add config file support
2021-02-04 12:58 ` Arnaldo Carvalho de Melo
@ 2021-02-04 15:10 ` Jiri Olsa
2021-02-06 8:06 ` Namhyung Kim
0 siblings, 1 reply; 72+ messages in thread
From: Jiri Olsa @ 2021-02-04 15:10 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Namhyung Kim, Jiri Olsa, lkml, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov
On Thu, Feb 04, 2021 at 09:58:19AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Feb 04, 2021 at 09:42:35PM +0900, Namhyung Kim escreveu:
> > Hi Jiri,
> >
> > On Sun, Jan 31, 2021 at 8:49 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > [SNIP]
> > > +#define SESSION_OUTPUT "output"
> > > +
> > > +/*
> > > + * Session states:
> > > + *
> > > + * OK - session is up and running
> > > + * RECONFIG - session is pending for reconfiguration,
> > > + * new values are already loaded in session object
> > > + * KILL - session is pending to be killed
> > > + *
> > > + * Session object life and its state is maintained by
> > > + * following functions:
> > > + *
> > > + * setup_server_config
> > > + * - reads config file and setup session objects
> > > + * with following states:
> > > + *
> > > + * OK - no change needed
> > > + * RECONFIG - session needs to be changed
> > > + * (run variable changed)
> > > + * KILL - session needs to be killed
> > > + * (session is no longer in config file)
> > > + *
> > > + * daemon__reconfig
> > > + * - scans session objects and does following actions
> > > + * for states:
> > > + *
> > > + * OK - skip
> > > + * RECONFIG - session is killed and re-run with new config
> > > + * KILL - session is killed
> > > + *
> > > + * - all sessions have OK state on the function exit
> > > + */
> > > +enum session_state {
> > > + SESSION_STATE__OK,
> > > + SESSION_STATE__RECONFIG,
> > > + SESSION_STATE__KILL,
> > > +};
> > > +
> > > +struct session {
> > > + char *base;
> > > + char *name;
> > > + char *run;
> > > + int pid;
> > > + struct list_head list;
> > > + enum session_state state;
> > > +};
> >
> > Although I think calling it 'session' is intuitive, it's also confusing
> > as we already have struct perf_session...
ok, how about daemon_session then?
>
> Maybe 'struct server_session' ? If this ends up in tools/lib/perf, then
> it gets renamed to 'struct perf_server_session', just like we have
> 'struct perf_evsel' in libperf and 'struct evsel' in tools/perf/, right?
let's have our grand-grand-grandkids worry about that ;-)
thanks,
jirka
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 06/24] perf daemon: Add config file support
2021-02-04 15:10 ` Jiri Olsa
@ 2021-02-06 8:06 ` Namhyung Kim
0 siblings, 0 replies; 72+ messages in thread
From: Namhyung Kim @ 2021-02-06 8:06 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, lkml, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Michael Petlan,
Ian Rogers, Stephane Eranian, Alexei Budankov
On Fri, Feb 5, 2021 at 12:10 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Thu, Feb 04, 2021 at 09:58:19AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Feb 04, 2021 at 09:42:35PM +0900, Namhyung Kim escreveu:
> > > Hi Jiri,
> > >
> > > On Sun, Jan 31, 2021 at 8:49 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > [SNIP]
> > > > +#define SESSION_OUTPUT "output"
> > > > +
> > > > +/*
> > > > + * Session states:
> > > > + *
> > > > + * OK - session is up and running
> > > > + * RECONFIG - session is pending for reconfiguration,
> > > > + * new values are already loaded in session object
> > > > + * KILL - session is pending to be killed
> > > > + *
> > > > + * Session object life and its state is maintained by
> > > > + * following functions:
> > > > + *
> > > > + * setup_server_config
> > > > + * - reads config file and setup session objects
> > > > + * with following states:
> > > > + *
> > > > + * OK - no change needed
> > > > + * RECONFIG - session needs to be changed
> > > > + * (run variable changed)
> > > > + * KILL - session needs to be killed
> > > > + * (session is no longer in config file)
> > > > + *
> > > > + * daemon__reconfig
> > > > + * - scans session objects and does following actions
> > > > + * for states:
> > > > + *
> > > > + * OK - skip
> > > > + * RECONFIG - session is killed and re-run with new config
> > > > + * KILL - session is killed
> > > > + *
> > > > + * - all sessions have OK state on the function exit
> > > > + */
> > > > +enum session_state {
> > > > + SESSION_STATE__OK,
> > > > + SESSION_STATE__RECONFIG,
> > > > + SESSION_STATE__KILL,
> > > > +};
> > > > +
> > > > +struct session {
> > > > + char *base;
> > > > + char *name;
> > > > + char *run;
> > > > + int pid;
> > > > + struct list_head list;
> > > > + enum session_state state;
> > > > +};
> > >
> > > Although I think calling it 'session' is intuitive, it's also confusing
> > > as we already have struct perf_session...
>
> ok, how about daemon_session then?
I'm ok with it.
>
> >
> > Maybe 'struct server_session' ? If this ends up in tools/lib/perf, then
> > it gets renamed to 'struct perf_server_session', just like we have
> > 'struct perf_evsel' in libperf and 'struct evsel' in tools/perf/, right?
>
> let's have our grand-grand-grandkids worry about that ;-)
:)
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 06/24] perf daemon: Add config file support
2021-01-30 23:48 ` [PATCH 06/24] perf daemon: Add config file support Jiri Olsa
2021-02-03 21:12 ` Arnaldo Carvalho de Melo
2021-02-04 12:42 ` Namhyung Kim
@ 2021-02-05 12:14 ` Namhyung Kim
2021-02-05 12:56 ` Jiri Olsa
2 siblings, 1 reply; 72+ messages in thread
From: Namhyung Kim @ 2021-02-05 12:14 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, lkml, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov
On Sun, Jan 31, 2021 at 8:49 AM Jiri Olsa <jolsa@kernel.org> wrote:
[SNIP]
> @@ -263,9 +605,16 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> signal(SIGTERM, sig_handler);
>
> while (!done && !err) {
> - if (fdarray__poll(&fda, -1)) {
> + err = daemon__reconfig(daemon);
I think it's confusing since you put the reconfig function here.
What not split normal and reconfig passes?
I mean something like below
__cmd_start()
{
setup_server_config();
daemon__run();
while (!done && !err) {
...
if (reconfig) {
daemon__kill();
setup_server_config();
daemon__reconfig();
}
}
Thanks,
Namhyung
> +
> + if (!err && fdarray__poll(&fda, -1)) {
> + bool reconfig = false;
> +
> if (fda.entries[sock_pos].revents & POLLIN)
> err = handle_server_socket(daemon, sock_fd);
> +
> + if (reconfig)
> + err = setup_server_config(daemon);
> }
> }
>
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 06/24] perf daemon: Add config file support
2021-02-05 12:14 ` Namhyung Kim
@ 2021-02-05 12:56 ` Jiri Olsa
2021-02-06 8:05 ` Namhyung Kim
0 siblings, 1 reply; 72+ messages in thread
From: Jiri Olsa @ 2021-02-05 12:56 UTC (permalink / raw)
To: Namhyung Kim
Cc: Jiri Olsa, Arnaldo Carvalho de Melo, lkml, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Michael Petlan,
Ian Rogers, Stephane Eranian, Alexei Budankov
On Fri, Feb 05, 2021 at 09:14:54PM +0900, Namhyung Kim wrote:
> On Sun, Jan 31, 2021 at 8:49 AM Jiri Olsa <jolsa@kernel.org> wrote:
> [SNIP]
> > @@ -263,9 +605,16 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> > signal(SIGTERM, sig_handler);
> >
> > while (!done && !err) {
> > - if (fdarray__poll(&fda, -1)) {
> > + err = daemon__reconfig(daemon);
>
> I think it's confusing since you put the reconfig function here.
> What not split normal and reconfig passes?
hum, not sure what's confusing in here? I've been known to
produce confusing code, but this one seems clear to me
>
> I mean something like below
>
> __cmd_start()
> {
> setup_server_config();
> daemon__run();
what's daemon__run? the daemon operates in the while loop below
>
> while (!done && !err) {
> ...
> if (reconfig) {
> daemon__kill();
you don't kill daemon for each reconfig change,
we detect changed sessions and kill/restart only them
> setup_server_config();
> daemon__reconfig();
> }
> }
so basically the current workflow is:
setup_server_config <--- reads config file, prepares session objects
while (!done) {
daemon__reconfig <--- check session objects states and run/stop them
if (fdarray__poll(&fda, -1)) {
handle_config_changes(&reconfig) <--- was there a config file change?
if (reconfig) <--- yes,
setup_server_config <--- change session objects/states
}
}
thanks,
jirka
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 06/24] perf daemon: Add config file support
2021-02-05 12:56 ` Jiri Olsa
@ 2021-02-06 8:05 ` Namhyung Kim
2021-02-06 22:35 ` Jiri Olsa
0 siblings, 1 reply; 72+ messages in thread
From: Namhyung Kim @ 2021-02-06 8:05 UTC (permalink / raw)
To: Jiri Olsa
Cc: Jiri Olsa, Arnaldo Carvalho de Melo, lkml, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Michael Petlan,
Ian Rogers, Stephane Eranian, Alexei Budankov
On Fri, Feb 5, 2021 at 9:56 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, Feb 05, 2021 at 09:14:54PM +0900, Namhyung Kim wrote:
> > On Sun, Jan 31, 2021 at 8:49 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > [SNIP]
> > > @@ -263,9 +605,16 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> > > signal(SIGTERM, sig_handler);
> > >
> > > while (!done && !err) {
> > > - if (fdarray__poll(&fda, -1)) {
> > > + err = daemon__reconfig(daemon);
> >
> > I think it's confusing since you put the reconfig function here.
> > What not split normal and reconfig passes?
>
> hum, not sure what's confusing in here? I've been known to
> produce confusing code, but this one seems clear to me
Maybe it's because of the name. I thought reconfig is a
special case when it detects some change. But you call
it in the loop unconditionally.
>
> >
> > I mean something like below
> >
> > __cmd_start()
> > {
> > setup_server_config();
> > daemon__run();
>
> what's daemon__run? the daemon operates in the while loop below
I thought about starting the sessions in the config.
>
> >
> > while (!done && !err) {
> > ...
> > if (reconfig) {
> > daemon__kill();
>
> you don't kill daemon for each reconfig change,
> we detect changed sessions and kill/restart only them
Yep, we can make it that way.
>
> > setup_server_config();
> > daemon__reconfig();
> > }
> > }
>
>
> so basically the current workflow is:
>
> setup_server_config <--- reads config file, prepares session objects
>
> while (!done) {
> daemon__reconfig <--- check session objects states and run/stop them
Hmm.. then how about rename it to daemon__handle_state()
or daemon__do_loop() or something?
Thanks,
Namhyung
>
> if (fdarray__poll(&fda, -1)) {
>
> handle_config_changes(&reconfig) <--- was there a config file change?
>
> if (reconfig) <--- yes,
> setup_server_config <--- change session objects/states
> }
> }
>
> thanks,
> jirka
>
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 06/24] perf daemon: Add config file support
2021-02-06 8:05 ` Namhyung Kim
@ 2021-02-06 22:35 ` Jiri Olsa
2021-02-07 1:38 ` Namhyung Kim
0 siblings, 1 reply; 72+ messages in thread
From: Jiri Olsa @ 2021-02-06 22:35 UTC (permalink / raw)
To: Namhyung Kim
Cc: Jiri Olsa, Arnaldo Carvalho de Melo, lkml, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Michael Petlan,
Ian Rogers, Stephane Eranian, Alexei Budankov
On Sat, Feb 06, 2021 at 05:05:04PM +0900, Namhyung Kim wrote:
> On Fri, Feb 5, 2021 at 9:56 PM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Fri, Feb 05, 2021 at 09:14:54PM +0900, Namhyung Kim wrote:
> > > On Sun, Jan 31, 2021 at 8:49 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > [SNIP]
> > > > @@ -263,9 +605,16 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> > > > signal(SIGTERM, sig_handler);
> > > >
> > > > while (!done && !err) {
> > > > - if (fdarray__poll(&fda, -1)) {
> > > > + err = daemon__reconfig(daemon);
> > >
> > > I think it's confusing since you put the reconfig function here.
> > > What not split normal and reconfig passes?
> >
> > hum, not sure what's confusing in here? I've been known to
> > produce confusing code, but this one seems clear to me
>
> Maybe it's because of the name. I thought reconfig is a
> special case when it detects some change. But you call
> it in the loop unconditionally.
>
> >
> > >
> > > I mean something like below
> > >
> > > __cmd_start()
> > > {
> > > setup_server_config();
> > > daemon__run();
> >
> > what's daemon__run? the daemon operates in the while loop below
>
> I thought about starting the sessions in the config.
>
> >
> > >
> > > while (!done && !err) {
> > > ...
> > > if (reconfig) {
> > > daemon__kill();
> >
> > you don't kill daemon for each reconfig change,
> > we detect changed sessions and kill/restart only them
>
> Yep, we can make it that way.
>
> >
> > > setup_server_config();
> > > daemon__reconfig();
> > > }
> > > }
> >
> >
> > so basically the current workflow is:
> >
> > setup_server_config <--- reads config file, prepares session objects
> >
> > while (!done) {
> > daemon__reconfig <--- check session objects states and run/stop them
>
> Hmm.. then how about rename it to daemon__handle_state()
> or daemon__do_loop() or something?
well it handles reconfig, so I don't think that there's
better name than daemon__reconfig ;-)
apart from handle_server_socket, all the other functions
we call after poll can change session state, so we need
to 'reconfig' sessions each time we do a loop
jirka
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 06/24] perf daemon: Add config file support
2021-02-06 22:35 ` Jiri Olsa
@ 2021-02-07 1:38 ` Namhyung Kim
0 siblings, 0 replies; 72+ messages in thread
From: Namhyung Kim @ 2021-02-07 1:38 UTC (permalink / raw)
To: Jiri Olsa
Cc: Jiri Olsa, Arnaldo Carvalho de Melo, lkml, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Michael Petlan,
Ian Rogers, Stephane Eranian, Alexei Budankov
On Sun, Feb 7, 2021 at 7:35 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Sat, Feb 06, 2021 at 05:05:04PM +0900, Namhyung Kim wrote:
> > On Fri, Feb 5, 2021 at 9:56 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Fri, Feb 05, 2021 at 09:14:54PM +0900, Namhyung Kim wrote:
> > > > On Sun, Jan 31, 2021 at 8:49 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > [SNIP]
> > > > > @@ -263,9 +605,16 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> > > > > signal(SIGTERM, sig_handler);
> > > > >
> > > > > while (!done && !err) {
> > > > > - if (fdarray__poll(&fda, -1)) {
> > > > > + err = daemon__reconfig(daemon);
> > > >
> > > > I think it's confusing since you put the reconfig function here.
> > > > What not split normal and reconfig passes?
> > >
> > > hum, not sure what's confusing in here? I've been known to
> > > produce confusing code, but this one seems clear to me
> >
> > Maybe it's because of the name. I thought reconfig is a
> > special case when it detects some change. But you call
> > it in the loop unconditionally.
> >
> > >
> > > >
> > > > I mean something like below
> > > >
> > > > __cmd_start()
> > > > {
> > > > setup_server_config();
> > > > daemon__run();
> > >
> > > what's daemon__run? the daemon operates in the while loop below
> >
> > I thought about starting the sessions in the config.
> >
> > >
> > > >
> > > > while (!done && !err) {
> > > > ...
> > > > if (reconfig) {
> > > > daemon__kill();
> > >
> > > you don't kill daemon for each reconfig change,
> > > we detect changed sessions and kill/restart only them
> >
> > Yep, we can make it that way.
> >
> > >
> > > > setup_server_config();
> > > > daemon__reconfig();
> > > > }
> > > > }
> > >
> > >
> > > so basically the current workflow is:
> > >
> > > setup_server_config <--- reads config file, prepares session objects
> > >
> > > while (!done) {
> > > daemon__reconfig <--- check session objects states and run/stop them
> >
> > Hmm.. then how about rename it to daemon__handle_state()
> > or daemon__do_loop() or something?
>
> well it handles reconfig, so I don't think that there's
> better name than daemon__reconfig ;-)
>
> apart from handle_server_socket, all the other functions
> we call after poll can change session state, so we need
> to 'reconfig' sessions each time we do a loop
OK then. Thanks for the explanation!
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH 07/24] perf daemon: Add config file change check
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (5 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 06/24] perf daemon: Add config file support Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-02-03 21:13 ` Arnaldo Carvalho de Melo
2021-01-30 23:48 ` [PATCH 08/24] perf daemon: Add background support Jiri Olsa
` (16 subsequent siblings)
23 siblings, 1 reply; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Adding support to detect daemon's config file changes
and re-read the configuration when that happens.
Using inotify file descriptor pluged into the main
fdarray object for polling.
Example:
# cat ~/.perfconfig
[daemon]
base=/opt/perfdata
[session-cycles]
run = -m 10M -e cycles --overwrite --switch-output -a
Starting the daemon:
# perf daemon start
Check sessions:
# perf daemon
[772262:daemon] base: /opt/perfdata
[772263:cycles] perf record -m 10M -e cycles --overwrite --switch-output -a
Change '-m 10M' to '-m 20M', and check daemon log:
# tail -f /opt/perfdata/output
[2021-01-02 20:31:41.234045] daemon started (pid 772262)
[2021-01-02 20:31:41.235072] reconfig: ruining session [cycles:772263]: -m 10M -e cycles --overwrite --switch-output -a
[2021-01-02 20:32:08.310137] reconfig: session 'cycles' killed
[2021-01-02 20:32:08.310847] reconfig: ruining session [cycles:772338]: -m 20M -e cycles --overwrite --switch-output -a
And the session list:
# perf daemon
[772262:daemon] base: /opt/perfdata
[772338:cycles] perf record -m 20M -e cycles --overwrite --switch-output -a
Note the changed '-m 20M' option is in place.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 99 +++++++++++++++++++++++++++++++++++--
1 file changed, 96 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index be2ade9967b3..d0a0a998e073 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -10,6 +10,8 @@
#include <stdio.h>
#include <unistd.h>
#include <errno.h>
+#include <sys/inotify.h>
+#include <libgen.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/un.h>
@@ -73,6 +75,7 @@ struct session {
struct daemon {
const char *config;
char *config_real;
+ char *config_base;
const char *base_user;
char *base;
struct list_head sessions;
@@ -493,6 +496,7 @@ static void daemon__free(struct daemon *daemon)
session__remove(session);
free(daemon->config_real);
+ free(daemon->config_base);
free(daemon->base);
}
@@ -535,6 +539,83 @@ static int daemon__reconfig(struct daemon *daemon)
return 0;
}
+static int setup_config_changes(struct daemon *daemon)
+{
+ char *basen = strdup(daemon->config_real);
+ char *dirn = strdup(daemon->config_real);
+ char *base, *dir;
+ int fd, wd;
+
+ if (!dirn || !basen)
+ return -ENOMEM;
+
+ fd = inotify_init1(IN_NONBLOCK|O_CLOEXEC);
+ if (fd < 0) {
+ perror("failed: inotify_init");
+ return -1;
+ }
+
+ dir = dirname(dirn);
+ base = basename(basen);
+ pr_debug("config file: %s, dir: %s\n", base, dir);
+
+ wd = inotify_add_watch(fd, dir, IN_CLOSE_WRITE);
+ if (wd >= 0) {
+ daemon->config_base = strdup(base);
+ if (!daemon->config_base) {
+ close(fd);
+ wd = -1;
+ }
+ } else {
+ perror("failed: inotify_add_watch");
+ }
+
+ free(basen);
+ free(dirn);
+ return wd < 0 ? -1 : fd;
+}
+
+static bool process_inotify_event(struct daemon *daemon, char *buf, ssize_t len)
+{
+ char *p = buf;
+
+ while (p < (buf + len)) {
+ struct inotify_event *event = (struct inotify_event *) p;
+
+ /*
+ * We monitor config directory, check if our
+ * config file was changes.
+ */
+ if ((event->mask & IN_CLOSE_WRITE) &&
+ !(event->mask & IN_ISDIR)) {
+ if (!strcmp(event->name, daemon->config_base))
+ return true;
+ }
+ p += sizeof(*event) + event->len;
+ }
+ return false;
+}
+
+static int handle_config_changes(struct daemon *daemon, int conf_fd,
+ bool *config_changed)
+{
+ char buf[4096];
+ ssize_t len;
+
+ while (!(*config_changed)) {
+ len = read(conf_fd, buf, sizeof(buf));
+ if (len == -1) {
+ if (errno != EAGAIN) {
+ perror("failed: read");
+ return -1;
+ }
+ return 0;
+ }
+ *config_changed = process_inotify_event(daemon, buf, len);
+ }
+ return 0;
+}
+
static int setup_config(struct daemon *daemon)
{
if (daemon->base_user) {
@@ -569,8 +650,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
OPT_PARENT(parent_options),
OPT_END()
};
- int sock_fd = -1;
- int sock_pos;
+ int sock_fd = -1, conf_fd = -1;
+ int sock_pos, file_pos;
struct fdarray fda;
int err = 0;
@@ -591,16 +672,24 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
pr_info("daemon started (pid %d)\n", getpid());
- fdarray__init(&fda, 1);
+ fdarray__init(&fda, 2);
sock_fd = setup_server_socket(daemon);
if (sock_fd < 0)
goto out;
+ conf_fd = setup_config_changes(daemon);
+ if (conf_fd < 0)
+ goto out;
+
sock_pos = fdarray__add(&fda, sock_fd, POLLIN|POLLERR|POLLHUP, 0);
if (sock_pos < 0)
goto out;
+ file_pos = fdarray__add(&fda, conf_fd, POLLIN|POLLERR|POLLHUP, 0);
+ if (file_pos < 0)
+ goto out;
+
signal(SIGINT, sig_handler);
signal(SIGTERM, sig_handler);
@@ -612,6 +701,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
if (fda.entries[sock_pos].revents & POLLIN)
err = handle_server_socket(daemon, sock_fd);
+ if (fda.entries[file_pos].revents & POLLIN)
+ err = handle_config_changes(daemon, conf_fd, &reconfig);
if (reconfig)
err = setup_server_config(daemon);
@@ -625,6 +716,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
if (sock_fd != -1)
close(sock_fd);
+ if (conf_fd != -1)
+ close(conf_fd);
pr_info("daemon exited\n");
fclose(daemon->out);
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [PATCH 07/24] perf daemon: Add config file change check
2021-01-30 23:48 ` [PATCH 07/24] perf daemon: Add config file change check Jiri Olsa
@ 2021-02-03 21:13 ` Arnaldo Carvalho de Melo
2021-02-04 14:51 ` Jiri Olsa
0 siblings, 1 reply; 72+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-03 21:13 UTC (permalink / raw)
To: Jiri Olsa
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Em Sun, Jan 31, 2021 at 12:48:39AM +0100, Jiri Olsa escreveu:
> Adding support to detect daemon's config file changes
> and re-read the configuration when that happens.
>
> Using inotify file descriptor pluged into the main
> fdarray object for polling.
>
> Example:
>
> # cat ~/.perfconfig
> [daemon]
> base=/opt/perfdata
>
> [session-cycles]
> run = -m 10M -e cycles --overwrite --switch-output -a
>
> Starting the daemon:
>
> # perf daemon start
>
> Check sessions:
>
> # perf daemon
> [772262:daemon] base: /opt/perfdata
> [772263:cycles] perf record -m 10M -e cycles --overwrite --switch-output -a
>
> Change '-m 10M' to '-m 20M', and check daemon log:
>
> # tail -f /opt/perfdata/output
> [2021-01-02 20:31:41.234045] daemon started (pid 772262)
> [2021-01-02 20:31:41.235072] reconfig: ruining session [cycles:772263]: -m 10M -e cycles --overwrite --switch-output -a
> [2021-01-02 20:32:08.310137] reconfig: session 'cycles' killed
> [2021-01-02 20:32:08.310847] reconfig: ruining session [cycles:772338]: -m 20M -e cycles --overwrite --switch-output -a
>
> And the session list:
>
> # perf daemon
> [772262:daemon] base: /opt/perfdata
> [772338:cycles] perf record -m 20M -e cycles --overwrite --switch-output -a
>
> Note the changed '-m 20M' option is in place.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/perf/builtin-daemon.c | 99 +++++++++++++++++++++++++++++++++++--
> 1 file changed, 96 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> index be2ade9967b3..d0a0a998e073 100644
> --- a/tools/perf/builtin-daemon.c
> +++ b/tools/perf/builtin-daemon.c
> @@ -10,6 +10,8 @@
> #include <stdio.h>
> #include <unistd.h>
> #include <errno.h>
> +#include <sys/inotify.h>
> +#include <libgen.h>
> #include <sys/types.h>
> #include <sys/socket.h>
> #include <sys/un.h>
> @@ -73,6 +75,7 @@ struct session {
> struct daemon {
> const char *config;
> char *config_real;
> + char *config_base;
> const char *base_user;
> char *base;
> struct list_head sessions;
> @@ -493,6 +496,7 @@ static void daemon__free(struct daemon *daemon)
> session__remove(session);
>
> free(daemon->config_real);
> + free(daemon->config_base);
> free(daemon->base);
Please replace those with zfree()
> }
>
> @@ -535,6 +539,83 @@ static int daemon__reconfig(struct daemon *daemon)
> return 0;
> }
>
> +static int setup_config_changes(struct daemon *daemon)
> +{
> + char *basen = strdup(daemon->config_real);
> + char *dirn = strdup(daemon->config_real);
> + char *base, *dir;
> + int fd, wd;
> +
> + if (!dirn || !basen)
> + return -ENOMEM;
This may leak one of them
> +
> + fd = inotify_init1(IN_NONBLOCK|O_CLOEXEC);
> + if (fd < 0) {
> + perror("failed: inotify_init");
> + return -1;
> + }
Cool that inotify is being used here :-)
> +
> + dir = dirname(dirn);
> + base = basename(basen);
> + pr_debug("config file: %s, dir: %s\n", base, dir);
> +
> + wd = inotify_add_watch(fd, dir, IN_CLOSE_WRITE);
> + if (wd >= 0) {
> + daemon->config_base = strdup(base);
> + if (!daemon->config_base) {
> + close(fd);
> + wd = -1;
> + }
> + } else {
> + perror("failed: inotify_add_watch");
> + }
> +
> + free(basen);
> + free(dirn);
> + return wd < 0 ? -1 : fd;
> +}
> +
> +static bool process_inotify_event(struct daemon *daemon, char *buf, ssize_t len)
> +{
> + char *p = buf;
> +
> + while (p < (buf + len)) {
> + struct inotify_event *event = (struct inotify_event *) p;
> +
> + /*
> + * We monitor config directory, check if our
> + * config file was changes.
> + */
> + if ((event->mask & IN_CLOSE_WRITE) &&
> + !(event->mask & IN_ISDIR)) {
> + if (!strcmp(event->name, daemon->config_base))
> + return true;
> + }
> + p += sizeof(*event) + event->len;
> + }
> + return false;
> +}
> +
> +static int handle_config_changes(struct daemon *daemon, int conf_fd,
> + bool *config_changed)
> +{
> + char buf[4096];
> + ssize_t len;
> +
> + while (!(*config_changed)) {
> + len = read(conf_fd, buf, sizeof(buf));
> + if (len == -1) {
> + if (errno != EAGAIN) {
> + perror("failed: read");
> + return -1;
> + }
> + return 0;
> + }
> + *config_changed = process_inotify_event(daemon, buf, len);
> + }
> + return 0;
> +}
> +
> static int setup_config(struct daemon *daemon)
> {
> if (daemon->base_user) {
> @@ -569,8 +650,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> OPT_PARENT(parent_options),
> OPT_END()
> };
> - int sock_fd = -1;
> - int sock_pos;
> + int sock_fd = -1, conf_fd = -1;
> + int sock_pos, file_pos;
> struct fdarray fda;
> int err = 0;
>
> @@ -591,16 +672,24 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
>
> pr_info("daemon started (pid %d)\n", getpid());
>
> - fdarray__init(&fda, 1);
> + fdarray__init(&fda, 2);
>
> sock_fd = setup_server_socket(daemon);
> if (sock_fd < 0)
> goto out;
>
> + conf_fd = setup_config_changes(daemon);
> + if (conf_fd < 0)
> + goto out;
> +
> sock_pos = fdarray__add(&fda, sock_fd, POLLIN|POLLERR|POLLHUP, 0);
> if (sock_pos < 0)
> goto out;
>
> + file_pos = fdarray__add(&fda, conf_fd, POLLIN|POLLERR|POLLHUP, 0);
> + if (file_pos < 0)
> + goto out;
> +
> signal(SIGINT, sig_handler);
> signal(SIGTERM, sig_handler);
>
> @@ -612,6 +701,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
>
> if (fda.entries[sock_pos].revents & POLLIN)
> err = handle_server_socket(daemon, sock_fd);
> + if (fda.entries[file_pos].revents & POLLIN)
> + err = handle_config_changes(daemon, conf_fd, &reconfig);
>
> if (reconfig)
> err = setup_server_config(daemon);
> @@ -625,6 +716,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
>
> if (sock_fd != -1)
> close(sock_fd);
> + if (conf_fd != -1)
> + close(conf_fd);
>
> pr_info("daemon exited\n");
> fclose(daemon->out);
> --
> 2.29.2
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 07/24] perf daemon: Add config file change check
2021-02-03 21:13 ` Arnaldo Carvalho de Melo
@ 2021-02-04 14:51 ` Jiri Olsa
0 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-02-04 14:51 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jiri Olsa, lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov
On Wed, Feb 03, 2021 at 06:13:59PM -0300, Arnaldo Carvalho de Melo wrote:
SNIP
> > #include <sys/types.h>
> > #include <sys/socket.h>
> > #include <sys/un.h>
> > @@ -73,6 +75,7 @@ struct session {
> > struct daemon {
> > const char *config;
> > char *config_real;
> > + char *config_base;
> > const char *base_user;
> > char *base;
> > struct list_head sessions;
> > @@ -493,6 +496,7 @@ static void daemon__free(struct daemon *daemon)
> > session__remove(session);
> >
> > free(daemon->config_real);
> > + free(daemon->config_base);
> > free(daemon->base);
>
> Please replace those with zfree()
ok
>
> > }
> >
> > @@ -535,6 +539,83 @@ static int daemon__reconfig(struct daemon *daemon)
> > return 0;
> > }
> >
> > +static int setup_config_changes(struct daemon *daemon)
> > +{
> > + char *basen = strdup(daemon->config_real);
> > + char *dirn = strdup(daemon->config_real);
> > + char *base, *dir;
> > + int fd, wd;
> > +
> > + if (!dirn || !basen)
> > + return -ENOMEM;
>
> This may leak one of them
right, will fix
thanks,
jirka
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH 08/24] perf daemon: Add background support
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (6 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 07/24] perf daemon: Add config file change check Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-02-03 21:16 ` Arnaldo Carvalho de Melo
2021-01-30 23:48 ` [PATCH 09/24] perf daemon: Add signalfd support Jiri Olsa
` (15 subsequent siblings)
23 siblings, 1 reply; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Adding support to put daemon process in the background.
It's now enabled by default and -f option is added to
keep daemon process on the console for debugging.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 66 +++++++++++++++++++++++++++++++++++--
1 file changed, 63 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index d0a0a998e073..324666058842 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -488,6 +488,13 @@ static void daemon__kill(struct daemon *daemon)
daemon__signal(daemon, SIGTERM);
}
+static void __daemon__free(struct daemon *daemon)
+{
+ free(daemon->config_real);
+ free(daemon->config_base);
+ free(daemon->base);
+}
+
static void daemon__free(struct daemon *daemon)
{
struct session *session, *h;
@@ -495,9 +502,7 @@ static void daemon__free(struct daemon *daemon)
list_for_each_entry_safe(session, h, &daemon->sessions, list)
session__remove(session);
- free(daemon->config_real);
- free(daemon->config_base);
- free(daemon->base);
+ __daemon__free(daemon);
}
static void daemon__exit(struct daemon *daemon)
@@ -643,10 +648,54 @@ static int setup_config(struct daemon *daemon)
return daemon->config_real ? 0 : -1;
}
+static int go_background(struct daemon *daemon)
+{
+ int pid, fd;
+
+ pid = fork();
+ if (pid < 0)
+ return -1;
+
+ if (pid > 0)
+ return 1;
+
+ if (setsid() < 0)
+ return -1;
+
+ umask(0);
+
+ if (chdir(daemon->base)) {
+ perror("failed: chdir");
+ return -1;
+ }
+
+ fd = open("output", O_RDWR|O_CREAT|O_TRUNC, 0644);
+ if (fd < 0) {
+ perror("failed: open");
+ return -1;
+ }
+
+ fcntl(fd, F_SETFD, FD_CLOEXEC);
+
+ close(0);
+ dup2(fd, 1);
+ dup2(fd, 2);
+ close(fd);
+
+ daemon->out = fdopen(1, "w");
+ if (!daemon->out)
+ return -1;
+
+ setbuf(daemon->out, NULL);
+ return 0;
+}
+
static int __cmd_start(struct daemon *daemon, struct option parent_options[],
int argc, const char **argv)
{
+ bool foreground = false;
struct option start_options[] = {
+ OPT_BOOLEAN('f', "foreground", &foreground, "stay on console"),
OPT_PARENT(parent_options),
OPT_END()
};
@@ -667,6 +716,17 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
if (setup_server_config(daemon))
return -1;
+ if (!foreground) {
+ err = go_background(daemon);
+ if (err) {
+ /* original process, exit normally */
+ if (err == 1)
+ err = 0;
+ __daemon__free(daemon);
+ return err;
+ }
+ }
+
debug_set_file(daemon->out);
debug_set_display_time(true);
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [PATCH 08/24] perf daemon: Add background support
2021-01-30 23:48 ` [PATCH 08/24] perf daemon: Add background support Jiri Olsa
@ 2021-02-03 21:16 ` Arnaldo Carvalho de Melo
2021-02-04 14:52 ` Jiri Olsa
0 siblings, 1 reply; 72+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-03 21:16 UTC (permalink / raw)
To: Jiri Olsa
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Em Sun, Jan 31, 2021 at 12:48:40AM +0100, Jiri Olsa escreveu:
> Adding support to put daemon process in the background.
>
> It's now enabled by default and -f option is added to
> keep daemon process on the console for debugging.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/perf/builtin-daemon.c | 66 +++++++++++++++++++++++++++++++++++--
> 1 file changed, 63 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> index d0a0a998e073..324666058842 100644
> --- a/tools/perf/builtin-daemon.c
> +++ b/tools/perf/builtin-daemon.c
> @@ -488,6 +488,13 @@ static void daemon__kill(struct daemon *daemon)
> daemon__signal(daemon, SIGTERM);
> }
>
> +static void __daemon__free(struct daemon *daemon)
> +{
> + free(daemon->config_real);
> + free(daemon->config_base);
> + free(daemon->base);
> +}
Please use zfree(), and also please rename it to __daemon__delete(), in
other cases this pattern would be daemon__exit(), as the daemon
structure itself is not being freed, just its members, ditto for
foo__new() calling foo__init().
> +
> static void daemon__free(struct daemon *daemon)
> {
> struct session *session, *h;
> @@ -495,9 +502,7 @@ static void daemon__free(struct daemon *daemon)
> list_for_each_entry_safe(session, h, &daemon->sessions, list)
> session__remove(session);
>
> - free(daemon->config_real);
> - free(daemon->config_base);
> - free(daemon->base);
> + __daemon__free(daemon);
> }
>
> static void daemon__exit(struct daemon *daemon)
> @@ -643,10 +648,54 @@ static int setup_config(struct daemon *daemon)
> return daemon->config_real ? 0 : -1;
> }
>
> +static int go_background(struct daemon *daemon)
> +{
> + int pid, fd;
> +
> + pid = fork();
> + if (pid < 0)
> + return -1;
> +
> + if (pid > 0)
> + return 1;
> +
> + if (setsid() < 0)
> + return -1;
> +
> + umask(0);
> +
> + if (chdir(daemon->base)) {
> + perror("failed: chdir");
> + return -1;
> + }
> +
> + fd = open("output", O_RDWR|O_CREAT|O_TRUNC, 0644);
> + if (fd < 0) {
> + perror("failed: open");
> + return -1;
> + }
> +
> + fcntl(fd, F_SETFD, FD_CLOEXEC);
> +
> + close(0);
> + dup2(fd, 1);
> + dup2(fd, 2);
> + close(fd);
> +
> + daemon->out = fdopen(1, "w");
> + if (!daemon->out)
> + return -1;
> +
> + setbuf(daemon->out, NULL);
> + return 0;
> +}
> +
> static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> int argc, const char **argv)
> {
> + bool foreground = false;
> struct option start_options[] = {
> + OPT_BOOLEAN('f', "foreground", &foreground, "stay on console"),
You forgot to add the entry to the man page
> OPT_PARENT(parent_options),
> OPT_END()
> };
> @@ -667,6 +716,17 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> if (setup_server_config(daemon))
> return -1;
>
> + if (!foreground) {
> + err = go_background(daemon);
> + if (err) {
> + /* original process, exit normally */
> + if (err == 1)
> + err = 0;
> + __daemon__free(daemon);
> + return err;
> + }
> + }
> +
> debug_set_file(daemon->out);
> debug_set_display_time(true);
>
> --
> 2.29.2
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 08/24] perf daemon: Add background support
2021-02-03 21:16 ` Arnaldo Carvalho de Melo
@ 2021-02-04 14:52 ` Jiri Olsa
0 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-02-04 14:52 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jiri Olsa, lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov
On Wed, Feb 03, 2021 at 06:16:11PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Jan 31, 2021 at 12:48:40AM +0100, Jiri Olsa escreveu:
> > Adding support to put daemon process in the background.
> >
> > It's now enabled by default and -f option is added to
> > keep daemon process on the console for debugging.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > tools/perf/builtin-daemon.c | 66 +++++++++++++++++++++++++++++++++++--
> > 1 file changed, 63 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> > index d0a0a998e073..324666058842 100644
> > --- a/tools/perf/builtin-daemon.c
> > +++ b/tools/perf/builtin-daemon.c
> > @@ -488,6 +488,13 @@ static void daemon__kill(struct daemon *daemon)
> > daemon__signal(daemon, SIGTERM);
> > }
> >
> > +static void __daemon__free(struct daemon *daemon)
> > +{
> > + free(daemon->config_real);
> > + free(daemon->config_base);
> > + free(daemon->base);
> > +}
>
> Please use zfree(), and also please rename it to __daemon__delete(), in
> other cases this pattern would be daemon__exit(), as the daemon
> structure itself is not being freed, just its members, ditto for
> foo__new() calling foo__init().
ok, will change
SNIP
> > static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> > int argc, const char **argv)
> > {
> > + bool foreground = false;
> > struct option start_options[] = {
> > + OPT_BOOLEAN('f', "foreground", &foreground, "stay on console"),
>
>
> You forgot to add the entry to the man page
it's in the man page patch later in the patchset
thanks,
jirka
>
> > OPT_PARENT(parent_options),
> > OPT_END()
> > };
> > @@ -667,6 +716,17 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> > if (setup_server_config(daemon))
> > return -1;
> >
> > + if (!foreground) {
> > + err = go_background(daemon);
> > + if (err) {
> > + /* original process, exit normally */
> > + if (err == 1)
> > + err = 0;
> > + __daemon__free(daemon);
> > + return err;
> > + }
> > + }
> > +
> > debug_set_file(daemon->out);
> > debug_set_display_time(true);
> >
> > --
> > 2.29.2
> >
>
> --
>
> - Arnaldo
>
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH 09/24] perf daemon: Add signalfd support
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (7 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 08/24] perf daemon: Add background support Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-02-03 21:24 ` Arnaldo Carvalho de Melo
2021-01-30 23:48 ` [PATCH 10/24] perf daemon: Add list command Jiri Olsa
` (14 subsequent siblings)
23 siblings, 1 reply; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Alexei Budankov, lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian
Using signalfd fd for tracking SIGCHLD signals as
notification for perf session termination.
This way we don't need to actively check for child
status, but we are notified if there's change.
Suggested-by: Alexei Budankov <abudankov@huawei.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 147 ++++++++++++++++++++++++++++++++++--
1 file changed, 141 insertions(+), 6 deletions(-)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index 324666058842..69d3af70b642 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -7,6 +7,7 @@
#include <string.h>
#include <signal.h>
#include <stdlib.h>
+#include <time.h>
#include <stdio.h>
#include <unistd.h>
#include <errno.h>
@@ -16,6 +17,8 @@
#include <sys/socket.h>
#include <sys/un.h>
#include <sys/stat.h>
+#include <sys/signalfd.h>
+#include <sys/wait.h>
#include <poll.h>
#include <sys/stat.h>
#include "builtin.h"
@@ -81,6 +84,7 @@ struct daemon {
struct list_head sessions;
FILE *out;
char perf[PATH_MAX];
+ int signal_fd;
};
static struct daemon __daemon = {
@@ -351,6 +355,103 @@ static int session__run(struct session *session, struct daemon *daemon)
return -1;
}
+static pid_t handle_signalfd(struct daemon *daemon)
+{
+ struct signalfd_siginfo si;
+ struct session *session;
+ ssize_t err;
+ int status;
+ pid_t pid;
+
+ err = read(daemon->signal_fd, &si, sizeof(struct signalfd_siginfo));
+ if (err != sizeof(struct signalfd_siginfo))
+ return -1;
+
+ list_for_each_entry(session, &daemon->sessions, list) {
+
+ if (session->pid != (int) si.ssi_pid)
+ continue;
+
+ pid = waitpid(session->pid, &status, 0);
+ if (pid == session->pid) {
+ if (WIFEXITED(status)) {
+ pr_info("session '%s' exited, status=%d\n",
+ session->name, WEXITSTATUS(status));
+ } else if (WIFSIGNALED(status)) {
+ pr_info("session '%s' killed (signal %d)\n",
+ session->name, WTERMSIG(status));
+ } else if (WIFSTOPPED(status)) {
+ pr_info("session '%s' stopped (signal %d)\n",
+ session->name, WSTOPSIG(status));
+ } else {
+ pr_info("session '%s' Unexpected status (0x%x)\n",
+ session->name, status);
+ }
+ }
+
+ session->state = SESSION_STATE__KILL;
+ session->pid = -1;
+ return pid;
+ }
+
+ return 0;
+}
+
+static int session__wait(struct session *session, struct daemon *daemon, int secs)
+{
+ struct pollfd pollfd = {
+ .fd = daemon->signal_fd,
+ .events = POLLIN,
+ };
+ pid_t wpid = 0, pid = session->pid;
+ time_t start;
+
+ start = time(NULL);
+
+ do {
+ if (poll(&pollfd, 1, 1000))
+ wpid = handle_signalfd(daemon);
+
+ if (start + secs < time(NULL))
+ return -1;
+ } while (wpid != pid);
+
+ return 0;
+}
+
+static bool daemon__has_alive_session(struct daemon *daemon)
+{
+ struct session *session;
+
+ list_for_each_entry(session, &daemon->sessions, list) {
+ if (session->pid != -1)
+ return true;
+ }
+
+ return false;
+}
+
+static int daemon__wait(struct daemon *daemon, int secs)
+{
+ struct pollfd pollfd = {
+ .fd = daemon->signal_fd,
+ .events = POLLIN,
+ };
+ time_t start;
+
+ start = time(NULL);
+
+ do {
+ if (poll(&pollfd, 1, 1000))
+ handle_signalfd(daemon);
+
+ if (start + secs < time(NULL))
+ return -1;
+ } while (daemon__has_alive_session(daemon));
+
+ return 0;
+}
+
static int setup_server_socket(struct daemon *daemon)
{
struct sockaddr_un addr;
@@ -456,9 +557,13 @@ static int session__signal(struct session *session, int sig)
return kill(session->pid, sig);
}
-static void session__kill(struct session *session)
+static void session__kill(struct session *session, struct daemon *daemon)
{
session__signal(session, SIGTERM);
+ if (session__wait(session, daemon, 10)) {
+ session__signal(session, SIGKILL);
+ session__wait(session, daemon, 10);
+ }
}
static void daemon__signal(struct daemon *daemon, int sig)
@@ -486,6 +591,10 @@ static void session__remove(struct session *session)
static void daemon__kill(struct daemon *daemon)
{
daemon__signal(daemon, SIGTERM);
+ if (daemon__wait(daemon, 10)) {
+ daemon__signal(daemon, SIGKILL);
+ daemon__wait(daemon, 10);
+ }
}
static void __daemon__free(struct daemon *daemon)
@@ -523,7 +632,7 @@ static int daemon__reconfig(struct daemon *daemon)
/* Remove session. */
if (session->state == SESSION_STATE__KILL) {
if (session->pid > 0) {
- session__kill(session);
+ session__kill(session, daemon);
pr_info("reconfig: session '%s' killed\n", session->name);
}
session__remove(session);
@@ -532,7 +641,7 @@ static int daemon__reconfig(struct daemon *daemon)
/* Reconfig session. */
if (session->pid > 0) {
- session__kill(session);
+ session__kill(session, daemon);
pr_info("reconfig: session '%s' killed\n", session->name);
}
if (session__run(session, daemon))
@@ -690,6 +799,20 @@ static int go_background(struct daemon *daemon)
return 0;
}
+static int setup_signalfd(struct daemon *daemon)
+{
+ sigset_t mask;
+
+ sigemptyset(&mask);
+ sigaddset(&mask, SIGCHLD);
+
+ if (sigprocmask(SIG_BLOCK, &mask, NULL) == -1)
+ return -1;
+
+ daemon->signal_fd = signalfd(-1, &mask, SFD_NONBLOCK|SFD_CLOEXEC);
+ return daemon->signal_fd;
+}
+
static int __cmd_start(struct daemon *daemon, struct option parent_options[],
int argc, const char **argv)
{
@@ -699,8 +822,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
OPT_PARENT(parent_options),
OPT_END()
};
- int sock_fd = -1, conf_fd = -1;
- int sock_pos, file_pos;
+ int sock_fd = -1, conf_fd = -1, signal_fd = -1;
+ int sock_pos, file_pos, signal_pos;
struct fdarray fda;
int err = 0;
@@ -732,7 +855,7 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
pr_info("daemon started (pid %d)\n", getpid());
- fdarray__init(&fda, 2);
+ fdarray__init(&fda, 3);
sock_fd = setup_server_socket(daemon);
if (sock_fd < 0)
@@ -742,6 +865,10 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
if (conf_fd < 0)
goto out;
+ signal_fd = setup_signalfd(daemon);
+ if (signal_fd < 0)
+ goto out;
+
sock_pos = fdarray__add(&fda, sock_fd, POLLIN|POLLERR|POLLHUP, 0);
if (sock_pos < 0)
goto out;
@@ -750,6 +877,10 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
if (file_pos < 0)
goto out;
+ signal_pos = fdarray__add(&fda, signal_fd, POLLIN|POLLERR|POLLHUP, 0);
+ if (signal_pos < 0)
+ goto out;
+
signal(SIGINT, sig_handler);
signal(SIGTERM, sig_handler);
@@ -763,6 +894,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
err = handle_server_socket(daemon, sock_fd);
if (fda.entries[file_pos].revents & POLLIN)
err = handle_config_changes(daemon, conf_fd, &reconfig);
+ if (fda.entries[signal_pos].revents & POLLIN)
+ err = handle_signalfd(daemon) < 0;
if (reconfig)
err = setup_server_config(daemon);
@@ -778,6 +911,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
close(sock_fd);
if (conf_fd != -1)
close(conf_fd);
+ if (conf_fd != -1)
+ close(signal_fd);
pr_info("daemon exited\n");
fclose(daemon->out);
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [PATCH 09/24] perf daemon: Add signalfd support
2021-01-30 23:48 ` [PATCH 09/24] perf daemon: Add signalfd support Jiri Olsa
@ 2021-02-03 21:24 ` Arnaldo Carvalho de Melo
2021-02-04 15:06 ` Jiri Olsa
0 siblings, 1 reply; 72+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-03 21:24 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Budankov, lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian
Em Sun, Jan 31, 2021 at 12:48:41AM +0100, Jiri Olsa escreveu:
> Using signalfd fd for tracking SIGCHLD signals as
> notification for perf session termination.
>
> This way we don't need to actively check for child
> status, but we are notified if there's change.
>
> Suggested-by: Alexei Budankov <abudankov@huawei.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/perf/builtin-daemon.c | 147 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 141 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> index 324666058842..69d3af70b642 100644
> --- a/tools/perf/builtin-daemon.c
> +++ b/tools/perf/builtin-daemon.c
> @@ -7,6 +7,7 @@
> #include <string.h>
> #include <signal.h>
> #include <stdlib.h>
> +#include <time.h>
> #include <stdio.h>
> #include <unistd.h>
> #include <errno.h>
> @@ -16,6 +17,8 @@
> #include <sys/socket.h>
> #include <sys/un.h>
> #include <sys/stat.h>
> +#include <sys/signalfd.h>
> +#include <sys/wait.h>
> #include <poll.h>
> #include <sys/stat.h>
> #include "builtin.h"
> @@ -81,6 +84,7 @@ struct daemon {
> struct list_head sessions;
> FILE *out;
> char perf[PATH_MAX];
> + int signal_fd;
> };
>
> static struct daemon __daemon = {
> @@ -351,6 +355,103 @@ static int session__run(struct session *session, struct daemon *daemon)
> return -1;
> }
>
> +static pid_t handle_signalfd(struct daemon *daemon)
> +{
> + struct signalfd_siginfo si;
> + struct session *session;
> + ssize_t err;
> + int status;
> + pid_t pid;
> +
> + err = read(daemon->signal_fd, &si, sizeof(struct signalfd_siginfo));
I saw these and recalled we have a readn() function, shouldn't we be
usint it in this series?
Its even in tools/lib/perf/lib.c
> + if (err != sizeof(struct signalfd_siginfo))
> + return -1;
> +
> + list_for_each_entry(session, &daemon->sessions, list) {
> +
> + if (session->pid != (int) si.ssi_pid)
> + continue;
> +
> + pid = waitpid(session->pid, &status, 0);
> + if (pid == session->pid) {
> + if (WIFEXITED(status)) {
> + pr_info("session '%s' exited, status=%d\n",
> + session->name, WEXITSTATUS(status));
> + } else if (WIFSIGNALED(status)) {
> + pr_info("session '%s' killed (signal %d)\n",
> + session->name, WTERMSIG(status));
> + } else if (WIFSTOPPED(status)) {
> + pr_info("session '%s' stopped (signal %d)\n",
> + session->name, WSTOPSIG(status));
> + } else {
> + pr_info("session '%s' Unexpected status (0x%x)\n",
> + session->name, status);
> + }
> + }
> +
> + session->state = SESSION_STATE__KILL;
> + session->pid = -1;
> + return pid;
> + }
> +
> + return 0;
> +}
> +
> +static int session__wait(struct session *session, struct daemon *daemon, int secs)
> +{
> + struct pollfd pollfd = {
> + .fd = daemon->signal_fd,
> + .events = POLLIN,
> + };
> + pid_t wpid = 0, pid = session->pid;
> + time_t start;
> +
> + start = time(NULL);
time_t start = time(NULL);
> +
> + do {
> + if (poll(&pollfd, 1, 1000))
> + wpid = handle_signalfd(daemon);
Shouldn't we check for -1 and handle that differently?
> +
> + if (start + secs < time(NULL))
> + return -1;
> + } while (wpid != pid);
> +
> + return 0;
> +}
> +
> +static bool daemon__has_alive_session(struct daemon *daemon)
> +{
> + struct session *session;
> +
> + list_for_each_entry(session, &daemon->sessions, list) {
> + if (session->pid != -1)
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static int daemon__wait(struct daemon *daemon, int secs)
> +{
> + struct pollfd pollfd = {
> + .fd = daemon->signal_fd,
> + .events = POLLIN,
> + };
> + time_t start;
> +
> + start = time(NULL);
> +
> + do {
> + if (poll(&pollfd, 1, 1000))
> + handle_signalfd(daemon);
ditto
> +
> + if (start + secs < time(NULL))
> + return -1;
> + } while (daemon__has_alive_session(daemon));
> +
> + return 0;
> +}
> +
> static int setup_server_socket(struct daemon *daemon)
> {
> struct sockaddr_un addr;
> @@ -456,9 +557,13 @@ static int session__signal(struct session *session, int sig)
> return kill(session->pid, sig);
> }
>
> -static void session__kill(struct session *session)
> +static void session__kill(struct session *session, struct daemon *daemon)
> {
> session__signal(session, SIGTERM);
> + if (session__wait(session, daemon, 10)) {
> + session__signal(session, SIGKILL);
> + session__wait(session, daemon, 10);
> + }
> }
>
> static void daemon__signal(struct daemon *daemon, int sig)
> @@ -486,6 +591,10 @@ static void session__remove(struct session *session)
> static void daemon__kill(struct daemon *daemon)
> {
> daemon__signal(daemon, SIGTERM);
> + if (daemon__wait(daemon, 10)) {
> + daemon__signal(daemon, SIGKILL);
> + daemon__wait(daemon, 10);
> + }
> }
>
> static void __daemon__free(struct daemon *daemon)
> @@ -523,7 +632,7 @@ static int daemon__reconfig(struct daemon *daemon)
> /* Remove session. */
> if (session->state == SESSION_STATE__KILL) {
> if (session->pid > 0) {
> - session__kill(session);
> + session__kill(session, daemon);
> pr_info("reconfig: session '%s' killed\n", session->name);
> }
> session__remove(session);
> @@ -532,7 +641,7 @@ static int daemon__reconfig(struct daemon *daemon)
>
> /* Reconfig session. */
> if (session->pid > 0) {
> - session__kill(session);
> + session__kill(session, daemon);
> pr_info("reconfig: session '%s' killed\n", session->name);
> }
> if (session__run(session, daemon))
> @@ -690,6 +799,20 @@ static int go_background(struct daemon *daemon)
> return 0;
> }
>
> +static int setup_signalfd(struct daemon *daemon)
> +{
> + sigset_t mask;
> +
> + sigemptyset(&mask);
> + sigaddset(&mask, SIGCHLD);
> +
> + if (sigprocmask(SIG_BLOCK, &mask, NULL) == -1)
> + return -1;
> +
> + daemon->signal_fd = signalfd(-1, &mask, SFD_NONBLOCK|SFD_CLOEXEC);
> + return daemon->signal_fd;
> +}
> +
> static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> int argc, const char **argv)
> {
> @@ -699,8 +822,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> OPT_PARENT(parent_options),
> OPT_END()
> };
> - int sock_fd = -1, conf_fd = -1;
> - int sock_pos, file_pos;
> + int sock_fd = -1, conf_fd = -1, signal_fd = -1;
> + int sock_pos, file_pos, signal_pos;
> struct fdarray fda;
> int err = 0;
>
> @@ -732,7 +855,7 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
>
> pr_info("daemon started (pid %d)\n", getpid());
>
> - fdarray__init(&fda, 2);
> + fdarray__init(&fda, 3);
>
> sock_fd = setup_server_socket(daemon);
> if (sock_fd < 0)
> @@ -742,6 +865,10 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> if (conf_fd < 0)
> goto out;
>
> + signal_fd = setup_signalfd(daemon);
> + if (signal_fd < 0)
> + goto out;
> +
> sock_pos = fdarray__add(&fda, sock_fd, POLLIN|POLLERR|POLLHUP, 0);
> if (sock_pos < 0)
> goto out;
> @@ -750,6 +877,10 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> if (file_pos < 0)
> goto out;
>
> + signal_pos = fdarray__add(&fda, signal_fd, POLLIN|POLLERR|POLLHUP, 0);
> + if (signal_pos < 0)
> + goto out;
> +
> signal(SIGINT, sig_handler);
> signal(SIGTERM, sig_handler);
>
> @@ -763,6 +894,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> err = handle_server_socket(daemon, sock_fd);
> if (fda.entries[file_pos].revents & POLLIN)
> err = handle_config_changes(daemon, conf_fd, &reconfig);
> + if (fda.entries[signal_pos].revents & POLLIN)
> + err = handle_signalfd(daemon) < 0;
>
> if (reconfig)
> err = setup_server_config(daemon);
> @@ -778,6 +911,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> close(sock_fd);
> if (conf_fd != -1)
> close(conf_fd);
> + if (conf_fd != -1)
> + close(signal_fd);
>
> pr_info("daemon exited\n");
> fclose(daemon->out);
> --
> 2.29.2
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [PATCH 09/24] perf daemon: Add signalfd support
2021-02-03 21:24 ` Arnaldo Carvalho de Melo
@ 2021-02-04 15:06 ` Jiri Olsa
0 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-02-04 15:06 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jiri Olsa, Alexei Budankov, lkml, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Namhyung Kim, Alexander Shishkin, Michael Petlan,
Ian Rogers, Stephane Eranian
On Wed, Feb 03, 2021 at 06:24:09PM -0300, Arnaldo Carvalho de Melo wrote:
SNIP
> > @@ -351,6 +355,103 @@ static int session__run(struct session *session, struct daemon *daemon)
> > return -1;
> > }
> >
> > +static pid_t handle_signalfd(struct daemon *daemon)
> > +{
> > + struct signalfd_siginfo si;
> > + struct session *session;
> > + ssize_t err;
> > + int status;
> > + pid_t pid;
> > +
> > + err = read(daemon->signal_fd, &si, sizeof(struct signalfd_siginfo));
>
>
> I saw these and recalled we have a readn() function, shouldn't we be
> usint it in this series?
right, but the read call in here needs to succeed with the data
size otherwise it's an error
but perhaps we could use it later for the control descriptor
read/write, I'll check
>
> Its even in tools/lib/perf/lib.c
>
> > + if (err != sizeof(struct signalfd_siginfo))
> > + return -1;
> > +
> > + list_for_each_entry(session, &daemon->sessions, list) {
> > +
> > + if (session->pid != (int) si.ssi_pid)
> > + continue;
> > +
> > + pid = waitpid(session->pid, &status, 0);
> > + if (pid == session->pid) {
> > + if (WIFEXITED(status)) {
> > + pr_info("session '%s' exited, status=%d\n",
> > + session->name, WEXITSTATUS(status));
> > + } else if (WIFSIGNALED(status)) {
> > + pr_info("session '%s' killed (signal %d)\n",
> > + session->name, WTERMSIG(status));
> > + } else if (WIFSTOPPED(status)) {
> > + pr_info("session '%s' stopped (signal %d)\n",
> > + session->name, WSTOPSIG(status));
> > + } else {
> > + pr_info("session '%s' Unexpected status (0x%x)\n",
> > + session->name, status);
> > + }
> > + }
> > +
> > + session->state = SESSION_STATE__KILL;
> > + session->pid = -1;
> > + return pid;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int session__wait(struct session *session, struct daemon *daemon, int secs)
> > +{
> > + struct pollfd pollfd = {
> > + .fd = daemon->signal_fd,
> > + .events = POLLIN,
> > + };
> > + pid_t wpid = 0, pid = session->pid;
> > + time_t start;
> > +
> > + start = time(NULL);
>
>
> time_t start = time(NULL);
> > +
> > + do {
> > + if (poll(&pollfd, 1, 1000))
> > + wpid = handle_signalfd(daemon);
>
> Shouldn't we check for -1 and handle that differently?
ah right, check for error, will add
>
> > +
> > + if (start + secs < time(NULL))
> > + return -1;
> > + } while (wpid != pid);
> > +
> > + return 0;
> > +}
> > +
> > +static bool daemon__has_alive_session(struct daemon *daemon)
> > +{
> > + struct session *session;
> > +
> > + list_for_each_entry(session, &daemon->sessions, list) {
> > + if (session->pid != -1)
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +static int daemon__wait(struct daemon *daemon, int secs)
> > +{
> > + struct pollfd pollfd = {
> > + .fd = daemon->signal_fd,
> > + .events = POLLIN,
> > + };
> > + time_t start;
> > +
> > + start = time(NULL);
> > +
> > + do {
> > + if (poll(&pollfd, 1, 1000))
> > + handle_signalfd(daemon);
>
> ditto
ok
thanks,
jirka
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH 10/24] perf daemon: Add list command
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (8 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 09/24] perf daemon: Add signalfd support Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-01-30 23:48 ` [PATCH 11/24] perf daemon: Add signal command Jiri Olsa
` (13 subsequent siblings)
23 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Adding list command to display all running sessions.
It's the default command if no other command is specified.
Example:
# cat ~/.perfconfig
[daemon]
base=/opt/perfdata
[session-cycles]
run = -m 10M -e cycles --overwrite --switch-output -a
[session-sched]
run = -m 20M -e sched:* --overwrite --switch-output -a
Starting the daemon:
# perf daemon start
List sessions:
# perf daemon
[771394:daemon] base: /opt/perfdata
[771395:cycles] perf record -m 10M -e cycles --overwrite --switch-output -a
[771396:sched] perf record -m 20M -e sched:* --overwrite --switch-output -a
List sessions with more info:
# perf daemon -v
[771394:daemon] base: /opt/perfdata
output: /opt/perfdata/output
[771395:cycles] perf record -m 10M -e cycles --overwrite --switch-output -a
base: /opt/perfdata/session-cycles
output: /opt/perfdata/session-cycles/output
[771396:sched] perf record -m 20M -e sched:* --overwrite --switch-output -a
base: /opt/perfdata/session-sched
output: /opt/perfdata/session-sched/output
The 'output' file is perf record output for specific session.
Note you have to stop all running perf processes manually at
this point, stop command is coming in following patches.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 89 +++++++++++++++++++++++++++++++++++--
1 file changed, 86 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index 69d3af70b642..cdd1af4d672b 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -79,6 +79,7 @@ struct daemon {
const char *config;
char *config_real;
char *config_base;
+ const char *csv_sep;
const char *base_user;
char *base;
struct list_head sessions;
@@ -487,11 +488,78 @@ static int setup_server_socket(struct daemon *daemon)
return fd;
}
+enum {
+ CMD_LIST = 0,
+ CMD_MAX,
+};
+
union cmd {
int cmd;
+
+ /* CMD_LIST */
+ struct {
+ int cmd;
+ int verbose;
+ char csv_sep;
+ } list;
};
-static int handle_server_socket(struct daemon *daemon __maybe_unused, int sock_fd)
+static int cmd_session_list(struct daemon *daemon, union cmd *cmd, FILE *out)
+{
+ char csv_sep = cmd->list.csv_sep;
+ struct session *session;
+
+ if (csv_sep) {
+ fprintf(out, "%d%c%s%c%s%c%s/%s",
+ /* pid daemon */
+ getpid(), csv_sep, "daemon",
+ /* base */
+ csv_sep, daemon->base,
+ /* output */
+ csv_sep, daemon->base, SESSION_OUTPUT);
+
+ fprintf(out, "\n");
+ } else {
+ fprintf(out, "[%d:daemon] base: %s\n", getpid(), daemon->base);
+ if (cmd->list.verbose) {
+ fprintf(out, " output: %s/%s\n",
+ daemon->base, SESSION_OUTPUT);
+ }
+ }
+
+ list_for_each_entry(session, &daemon->sessions, list) {
+ if (csv_sep) {
+ fprintf(out, "%d%c%s%c%s",
+ /* pid */
+ session->pid,
+ /* name */
+ csv_sep, session->name,
+ /* base */
+ csv_sep, session->run);
+
+ fprintf(out, "%c%s%c%s/%s",
+ /* session dir */
+ csv_sep, session->base,
+ /* session output */
+ csv_sep, session->base, SESSION_OUTPUT);
+
+ fprintf(out, "\n");
+ } else {
+ fprintf(out, "[%d:%s] perf record %s\n",
+ session->pid, session->name, session->run);
+ if (!cmd->list.verbose)
+ continue;
+ fprintf(out, " base: %s\n",
+ session->base);
+ fprintf(out, " output: %s/%s\n",
+ session->base, SESSION_OUTPUT);
+ }
+ }
+
+ return 0;
+}
+
+static int handle_server_socket(struct daemon *daemon, int sock_fd)
{
int ret = -EINVAL, fd;
union cmd cmd;
@@ -515,6 +583,9 @@ static int handle_server_socket(struct daemon *daemon __maybe_unused, int sock_f
}
switch (cmd.cmd) {
+ case CMD_LIST:
+ ret = cmd_session_list(daemon, &cmd, out);
+ break;
default:
break;
}
@@ -919,7 +990,6 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
return err;
}
-__maybe_unused
static int send_cmd(struct daemon *daemon, union cmd *cmd)
{
char *line = NULL;
@@ -953,6 +1023,17 @@ static int send_cmd(struct daemon *daemon, union cmd *cmd)
return 0;
}
+static int send_cmd_list(struct daemon *daemon)
+{
+ union cmd cmd = {
+ .list.cmd = CMD_LIST,
+ .list.verbose = verbose,
+ };
+
+ cmd.list.csv_sep = daemon->csv_sep ? *daemon->csv_sep : 0;
+ return send_cmd(daemon, &cmd);
+}
+
int cmd_daemon(int argc, const char **argv)
{
struct option daemon_options[] = {
@@ -961,6 +1042,8 @@ int cmd_daemon(int argc, const char **argv)
"config file", "config file path"),
OPT_STRING(0, "base", &__daemon.base_user,
"directory", "base directory"),
+ OPT_STRING_OPTARG('x', "field-separator", &__daemon.csv_sep,
+ "field separator", "print counts with custom separator", ","),
OPT_END()
};
@@ -983,5 +1066,5 @@ int cmd_daemon(int argc, const char **argv)
return -1;
}
- return -1;
+ return send_cmd_list(&__daemon);
}
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH 11/24] perf daemon: Add signal command
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (9 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 10/24] perf daemon: Add list command Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-01-30 23:48 ` [PATCH 12/24] perf daemon: Add stop command Jiri Olsa
` (12 subsequent siblings)
23 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Allow perf daemon to send SIGUSR2 to all running sessions
or just to a specific session.
Example:
# cat ~/.perfconfig
[daemon]
base=/opt/perfdata
[session-cycles]
run = -m 10M -e cycles --overwrite --switch-output -a
[session-sched]
run = -m 20M -e sched:* --overwrite --switch-output -a
Starting the daemon:
# perf daemon start
Send signal to all running sessions:
# perf daemon signal
signal 12 sent to session 'cycles [773738]'
signal 12 sent to session 'sched [773739]'
Or to specific one:
# perf daemon signal --session sched
signal 12 sent to session 'sched [773739]'
And verify signals were delivered and perf.data dumped:
# cat /opt/perfdata/session-cycles/output
rounding mmap pages size to 32M (8192 pages)
[ perf record: dump data: Woken up 1 times ]
[ perf record: Dump perf.data.2021010220382490 ]
# car /opt/perfdata/session-sched/output
rounding mmap pages size to 32M (8192 pages)
[ perf record: dump data: Woken up 1 times ]
[ perf record: Dump perf.data.2021010220382489 ]
[ perf record: dump data: Woken up 1 times ]
[ perf record: Dump perf.data.2021010220393745 ]
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 75 +++++++++++++++++++++++++++++++++----
1 file changed, 68 insertions(+), 7 deletions(-)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index cdd1af4d672b..7581c5f296ad 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -490,9 +490,12 @@ static int setup_server_socket(struct daemon *daemon)
enum {
CMD_LIST = 0,
+ CMD_SIGNAL = 1,
CMD_MAX,
};
+#define SESSION_MAX 64
+
union cmd {
int cmd;
@@ -502,6 +505,13 @@ union cmd {
int verbose;
char csv_sep;
} list;
+
+ /* CMD_SIGNAL */
+ struct {
+ int cmd;
+ int sig;
+ char name[SESSION_MAX];
+ } signal;
};
static int cmd_session_list(struct daemon *daemon, union cmd *cmd, FILE *out)
@@ -559,6 +569,31 @@ static int cmd_session_list(struct daemon *daemon, union cmd *cmd, FILE *out)
return 0;
}
+static int session__signal(struct session *session, int sig)
+{
+ if (session->pid < 0)
+ return -1;
+ return kill(session->pid, sig);
+}
+
+static int cmd_session_kill(struct daemon *daemon, union cmd *cmd, FILE *out)
+{
+ struct session *session;
+ bool all = false;
+
+ all = !strcmp(cmd->signal.name, "all");
+
+ list_for_each_entry(session, &daemon->sessions, list) {
+ if (all || !strcmp(cmd->signal.name, session->name)) {
+ session__signal(session, cmd->signal.sig);
+ fprintf(out, "signal %d sent to session '%s [%d]'\n",
+ cmd->signal.sig, session->name, session->pid);
+ }
+ }
+
+ return 0;
+}
+
static int handle_server_socket(struct daemon *daemon, int sock_fd)
{
int ret = -EINVAL, fd;
@@ -586,6 +621,9 @@ static int handle_server_socket(struct daemon *daemon, int sock_fd)
case CMD_LIST:
ret = cmd_session_list(daemon, &cmd, out);
break;
+ case CMD_SIGNAL:
+ ret = cmd_session_kill(daemon, &cmd, out);
+ break;
default:
break;
}
@@ -621,13 +659,6 @@ static int setup_client_socket(struct daemon *daemon)
return fd;
}
-static int session__signal(struct session *session, int sig)
-{
- if (session->pid < 0)
- return -1;
- return kill(session->pid, sig);
-}
-
static void session__kill(struct session *session, struct daemon *daemon)
{
session__signal(session, SIGTERM);
@@ -1034,6 +1065,34 @@ static int send_cmd_list(struct daemon *daemon)
return send_cmd(daemon, &cmd);
}
+static int __cmd_signal(struct daemon *daemon, struct option parent_options[],
+ int argc, const char **argv)
+{
+ const char *name = "all";
+ struct option start_options[] = {
+ OPT_STRING(0, "session", &name, "session",
+ "Sent signal to specific session"),
+ OPT_PARENT(parent_options),
+ OPT_END()
+ };
+ union cmd cmd;
+
+ argc = parse_options(argc, argv, start_options, daemon_usage, 0);
+ if (argc)
+ usage_with_options(daemon_usage, start_options);
+
+ if (setup_config(daemon)) {
+ pr_err("failed: config not found\n");
+ return -1;
+ }
+
+ cmd.signal.cmd = CMD_SIGNAL,
+ cmd.signal.sig = SIGUSR2;
+ strncpy(cmd.signal.name, name, sizeof(cmd.signal.name) - 1);
+
+ return send_cmd(daemon, &cmd);
+}
+
int cmd_daemon(int argc, const char **argv)
{
struct option daemon_options[] = {
@@ -1056,6 +1115,8 @@ int cmd_daemon(int argc, const char **argv)
if (argc) {
if (!strcmp(argv[0], "start"))
return __cmd_start(&__daemon, daemon_options, argc, argv);
+ if (!strcmp(argv[0], "signal"))
+ return __cmd_signal(&__daemon, daemon_options, argc, argv);
pr_err("failed: unknown command '%s'\n", argv[0]);
return -1;
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH 12/24] perf daemon: Add stop command
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (10 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 11/24] perf daemon: Add signal command Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-01-30 23:48 ` [PATCH 13/24] perf daemon: Allow only one daemon over base directory Jiri Olsa
` (11 subsequent siblings)
23 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Add 'perf daemon stop' command to stop daemon process
and all running sessions.
Example:
# cat ~/.perfconfig
[daemon]
base=/opt/perfdata
[session-cycles]
run = -m 10M -e cycles --overwrite --switch-output -a
[session-sched]
run = -m 20M -e sched:* --overwrite --switch-output -a
Starting the daemon:
# perf daemon start
Stopping the daemon
# perf daemon stop
Daemon is not running, nothing to connect to:
# perf daemon
connect error: Connection refused
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index 7581c5f296ad..468ed2af8b3f 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -491,6 +491,7 @@ static int setup_server_socket(struct daemon *daemon)
enum {
CMD_LIST = 0,
CMD_SIGNAL = 1,
+ CMD_STOP = 2,
CMD_MAX,
};
@@ -624,6 +625,10 @@ static int handle_server_socket(struct daemon *daemon, int sock_fd)
case CMD_SIGNAL:
ret = cmd_session_kill(daemon, &cmd, out);
break;
+ case CMD_STOP:
+ done = 1;
+ pr_debug("perf daemon is exciting\n");
+ break;
default:
break;
}
@@ -1093,6 +1098,27 @@ static int __cmd_signal(struct daemon *daemon, struct option parent_options[],
return send_cmd(daemon, &cmd);
}
+static int __cmd_stop(struct daemon *daemon, struct option parent_options[],
+ int argc, const char **argv)
+{
+ struct option start_options[] = {
+ OPT_PARENT(parent_options),
+ OPT_END()
+ };
+ union cmd cmd = { .cmd = CMD_STOP, };
+
+ argc = parse_options(argc, argv, start_options, daemon_usage, 0);
+ if (argc)
+ usage_with_options(daemon_usage, start_options);
+
+ if (setup_config(daemon)) {
+ pr_err("failed: config not found\n");
+ return -1;
+ }
+
+ return send_cmd(daemon, &cmd);
+}
+
int cmd_daemon(int argc, const char **argv)
{
struct option daemon_options[] = {
@@ -1117,6 +1143,8 @@ int cmd_daemon(int argc, const char **argv)
return __cmd_start(&__daemon, daemon_options, argc, argv);
if (!strcmp(argv[0], "signal"))
return __cmd_signal(&__daemon, daemon_options, argc, argv);
+ else if (!strcmp(argv[0], "stop"))
+ return __cmd_stop(&__daemon, daemon_options, argc, argv);
pr_err("failed: unknown command '%s'\n", argv[0]);
return -1;
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH 13/24] perf daemon: Allow only one daemon over base directory
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (11 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 12/24] perf daemon: Add stop command Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-01-30 23:48 ` [PATCH 14/24] perf daemon: Set control fifo for session Jiri Olsa
` (10 subsequent siblings)
23 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Add 'lock' file under daemon base and flock it, so only one
perf daemon can run on top of it.
Each daemon tries to create and lock BASE/lock file, if it's
successful we are sure we're the only daemon running over
the BASE.
Once daemon is finished, file descriptor to lock file is
closed and lock is released.
Example:
# cat ~/.perfconfig
[daemon]
base=/opt/perfdata
[session-cycles]
run = -m 10M -e cycles --overwrite --switch-output -a
[session-sched]
run = -m 20M -e sched:* --overwrite --switch-output -a
Starting the daemon:
# perf daemon start
And try once more:
# perf daemon start
failed: another perf daemon (pid 775594) owns /opt/perfdata
will end up with an error, because there's already one running
on top of /opt/perfdata.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 58 +++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index 468ed2af8b3f..4ebeb524b16e 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -1,10 +1,12 @@
// SPDX-License-Identifier: GPL-2.0
#include <subcmd/parse-options.h>
#include <api/fd/array.h>
+#include <api/fs/fs.h>
#include <linux/zalloc.h>
#include <linux/string.h>
#include <linux/limits.h>
#include <string.h>
+#include <sys/file.h>
#include <signal.h>
#include <stdlib.h>
#include <time.h>
@@ -529,12 +531,18 @@ static int cmd_session_list(struct daemon *daemon, union cmd *cmd, FILE *out)
/* output */
csv_sep, daemon->base, SESSION_OUTPUT);
+ fprintf(out, "%c%s/%s",
+ /* lock */
+ csv_sep, daemon->base, "lock");
+
fprintf(out, "\n");
} else {
fprintf(out, "[%d:daemon] base: %s\n", getpid(), daemon->base);
if (cmd->list.verbose) {
fprintf(out, " output: %s/%s\n",
daemon->base, SESSION_OUTPUT);
+ fprintf(out, " lock: %s/lock\n",
+ daemon->base);
}
}
@@ -864,6 +872,50 @@ static int setup_config(struct daemon *daemon)
return daemon->config_real ? 0 : -1;
}
+/*
+ * Each daemon tries to create and lock BASE/lock file,
+ * if it's successful we are sure we're the only daemon
+ * running over the BASE.
+ *
+ * Once daemon is finished, file descriptor to lock file
+ * is closed and lock is released.
+ */
+static int check_lock(struct daemon *daemon)
+{
+ char path[PATH_MAX];
+ char buf[20];
+ int fd, pid;
+ ssize_t len;
+
+ scnprintf(path, sizeof(path), "%s/lock", daemon->base);
+
+ fd = open(path, O_RDWR|O_CREAT|O_CLOEXEC, 0640);
+ if (fd < 0)
+ return -1;
+
+ if (lockf(fd, F_TLOCK, 0) < 0) {
+ filename__read_int(path, &pid);
+ fprintf(stderr, "failed: another perf daemon (pid %d) owns %s\n",
+ pid, daemon->base);
+ return -1;
+ }
+
+ scnprintf(buf, sizeof(buf), "%d", getpid());
+ len = strlen(buf);
+
+ if (write(fd, buf, len) != len) {
+ perror("failed: write");
+ return -1;
+ }
+
+ if (ftruncate(fd, len)) {
+ perror("failed: ftruncate");
+ return -1;
+ }
+
+ return 0;
+}
+
static int go_background(struct daemon *daemon)
{
int pid, fd;
@@ -878,6 +930,9 @@ static int go_background(struct daemon *daemon)
if (setsid() < 0)
return -1;
+ if (check_lock(daemon))
+ return -1;
+
umask(0);
if (chdir(daemon->base)) {
@@ -946,6 +1001,9 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
if (setup_server_config(daemon))
return -1;
+ if (foreground && check_lock(daemon))
+ return -1;
+
if (!foreground) {
err = go_background(daemon);
if (err) {
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH 14/24] perf daemon: Set control fifo for session
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (12 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 13/24] perf daemon: Allow only one daemon over base directory Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-01-30 23:48 ` [PATCH 15/24] perf daemon: Add ping command Jiri Olsa
` (9 subsequent siblings)
23 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Setup control fifos for session and add --control
option to session arguments.
Example:
# cat ~/.perfconfig
[daemon]
base=/opt/perfdata
[session-cycles]
run = -m 10M -e cycles --overwrite --switch-output -a
[session-sched]
run = -m 20M -e sched:* --overwrite --switch-output -a
Starting the daemon:
# perf daemon start
Use can list control fifos with (control and ack files):
# perf daemon -v
[776459:daemon] base: /opt/perfdata
output: /opt/perfdata/output
lock: /opt/perfdata/lock
[776460:cycles] perf record -m 20M -e cycles --overwrite --switch-output -a
base: /opt/perfdata/session-cycles
output: /opt/perfdata/session-cycles/output
control: /opt/perfdata/session-cycles/control
ack: /opt/perfdata/session-cycles/ack
[776461:sched] perf record -m 20M -e sched:* --overwrite --switch-output -a
base: /opt/perfdata/session-sched
output: /opt/perfdata/session-sched/output
control: /opt/perfdata/session-sched/control
ack: /opt/perfdata/session-sched/ack
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index 4ebeb524b16e..d7b3dd1e96f9 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -30,6 +30,8 @@
#include "util.h"
#define SESSION_OUTPUT "output"
+#define SESSION_CONTROL "control"
+#define SESSION_ACK "ack"
/*
* Session states:
@@ -72,6 +74,7 @@ struct session {
char *base;
char *name;
char *run;
+ char *control;
int pid;
struct list_head list;
enum session_state state;
@@ -348,7 +351,18 @@ static int session__run(struct session *session, struct daemon *daemon)
dup2(fd, 2);
close(fd);
- scnprintf(buf, sizeof(buf), "%s record %s", daemon->perf, session->run);
+ if (mkfifo(SESSION_CONTROL, O_RDWR) && errno != EEXIST) {
+ perror("failed: create control fifo");
+ return -1;
+ }
+
+ if (mkfifo(SESSION_ACK, O_RDWR) && errno != EEXIST) {
+ perror("failed: create ack fifo");
+ return -1;
+ }
+
+ scnprintf(buf, sizeof(buf), "%s record --control=fifo:%s,%s %s",
+ daemon->perf, SESSION_CONTROL, SESSION_ACK, session->run);
argv = argv_split(buf, &argc);
if (!argv)
@@ -562,6 +576,12 @@ static int cmd_session_list(struct daemon *daemon, union cmd *cmd, FILE *out)
/* session output */
csv_sep, session->base, SESSION_OUTPUT);
+ fprintf(out, "%c%s/%s%c%s/%s",
+ /* session control */
+ csv_sep, session->base, SESSION_CONTROL,
+ /* session ack */
+ csv_sep, session->base, SESSION_ACK);
+
fprintf(out, "\n");
} else {
fprintf(out, "[%d:%s] perf record %s\n",
@@ -572,6 +592,10 @@ static int cmd_session_list(struct daemon *daemon, union cmd *cmd, FILE *out)
session->base);
fprintf(out, " output: %s/%s\n",
session->base, SESSION_OUTPUT);
+ fprintf(out, " control: %s/%s\n",
+ session->base, SESSION_CONTROL);
+ fprintf(out, " ack: %s/%s\n",
+ session->base, SESSION_ACK);
}
}
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH 15/24] perf daemon: Add ping command
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (13 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 14/24] perf daemon: Set control fifo for session Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-01-30 23:48 ` [PATCH 16/24] perf daemon: Use control to stop session Jiri Olsa
` (8 subsequent siblings)
23 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Adding ping command to verify the perf record session
is up and operational.
It's used in following patches via test code to make
sure perf record is ready to receive signals.
Example:
# cat ~/.perfconfig
[daemon]
base=/opt/perfdata
[session-cycles]
run = -m 10M -e cycles --overwrite --switch-output -a
[session-sched]
run = -m 20M -e sched:* --overwrite --switch-output -a
Starting the daemon:
# perf daemon start
Ping all sessions:
# perf daemon ping
OK cycles
OK sched
Ping specific session:
# perf daemon ping --session sched
OK sched
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 151 ++++++++++++++++++++++++++++++++++++
1 file changed, 151 insertions(+)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index d7b3dd1e96f9..a5a71e0a706c 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -469,6 +469,80 @@ static int daemon__wait(struct daemon *daemon, int secs)
return 0;
}
+static int
+session__control(struct session *session, const char *msg, bool do_ack)
+{
+ struct pollfd pollfd = { 0, };
+ char control_path[PATH_MAX];
+ char ack_path[PATH_MAX];
+ int control, ack = -1, len;
+ char buf[20];
+ int ret = -1;
+ ssize_t err;
+
+ /* open the control file */
+ scnprintf(control_path, sizeof(control_path), "%s/%s",
+ session->base, SESSION_CONTROL);
+
+ control = open(control_path, O_WRONLY|O_NONBLOCK);
+ if (!control)
+ return -1;
+
+ if (do_ack) {
+ /* open the ack file */
+ scnprintf(ack_path, sizeof(ack_path), "%s/%s",
+ session->base, SESSION_ACK);
+
+ ack = open(ack_path, O_RDONLY, O_NONBLOCK);
+ if (!ack) {
+ close(control);
+ return -1;
+ }
+ }
+
+ /* write the command */
+ len = strlen(msg);
+
+ do {
+ err = write(control, msg, len);
+ if (err == -1 && errno != EAGAIN) {
+ pr_err("failed: write to control pipe: %d (%s)\n",
+ errno, control_path);
+ goto out;
+ }
+ } while (err != len);
+
+ if (!do_ack)
+ goto out;
+
+ /* wait for an ack */
+ pollfd.fd = ack;
+ pollfd.events = POLLIN;
+
+ if (!poll(&pollfd, 1, 2000)) {
+ pr_err("failed: control ack timeout\n");
+ goto out;
+ }
+
+ if (!pollfd.revents & POLLIN) {
+ pr_err("failed: did not received an ack\n");
+ goto out;
+ }
+
+ err = read(ack, buf, sizeof(buf));
+ if (err > 0)
+ ret = strcmp(buf, "ack\n");
+ else
+ perror("failed: read ack %d\n");
+
+out:
+ if (ack != -1)
+ close(ack);
+
+ close(control);
+ return ret;
+}
+
static int setup_server_socket(struct daemon *daemon)
{
struct sockaddr_un addr;
@@ -508,6 +582,7 @@ enum {
CMD_LIST = 0,
CMD_SIGNAL = 1,
CMD_STOP = 2,
+ CMD_PING = 3,
CMD_MAX,
};
@@ -529,8 +604,25 @@ union cmd {
int sig;
char name[SESSION_MAX];
} signal;
+
+ /* CMD_PING */
+ struct {
+ int cmd;
+ char name[SESSION_MAX];
+ } ping;
+};
+
+enum {
+ PING_OK = 0,
+ PING_FAIL = 1,
+ PING_MAX,
};
+static int session__ping(struct session *session)
+{
+ return session__control(session, "ping", true) ? PING_FAIL : PING_OK;
+}
+
static int cmd_session_list(struct daemon *daemon, union cmd *cmd, FILE *out)
{
char csv_sep = cmd->list.csv_sep;
@@ -627,6 +719,34 @@ static int cmd_session_kill(struct daemon *daemon, union cmd *cmd, FILE *out)
return 0;
}
+static const char *ping_str[PING_MAX] = {
+ [PING_OK] = "OK",
+ [PING_FAIL] = "FAIL",
+};
+
+static int cmd_session_ping(struct daemon *daemon, union cmd *cmd, FILE *out)
+{
+ struct session *session;
+ bool all = false, found = false;
+
+ all = !strcmp(cmd->ping.name, "all");
+
+ list_for_each_entry(session, &daemon->sessions, list) {
+ if (all || !strcmp(cmd->ping.name, session->name)) {
+ int state = session__ping(session);
+
+ fprintf(out, "%-4s %s\n", ping_str[state], session->name);
+ found = true;
+ }
+ }
+
+ if (!found && !all) {
+ fprintf(out, "%-4s %s (not found)\n",
+ ping_str[PING_FAIL], cmd->ping.name);
+ }
+ return 0;
+}
+
static int handle_server_socket(struct daemon *daemon, int sock_fd)
{
int ret = -EINVAL, fd;
@@ -661,6 +781,9 @@ static int handle_server_socket(struct daemon *daemon, int sock_fd)
done = 1;
pr_debug("perf daemon is exciting\n");
break;
+ case CMD_PING:
+ ret = cmd_session_ping(daemon, &cmd, out);
+ break;
default:
break;
}
@@ -1072,6 +1195,7 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
signal(SIGINT, sig_handler);
signal(SIGTERM, sig_handler);
+ signal(SIGPIPE, SIG_IGN);
while (!done && !err) {
err = daemon__reconfig(daemon);
@@ -1201,6 +1325,31 @@ static int __cmd_stop(struct daemon *daemon, struct option parent_options[],
return send_cmd(daemon, &cmd);
}
+static int __cmd_ping(struct daemon *daemon, struct option parent_options[],
+ int argc, const char **argv)
+{
+ const char *name = "all";
+ struct option ping_options[] = {
+ OPT_STRING(0, "session", &name, "session",
+ "Ping to specific session"),
+ OPT_PARENT(parent_options),
+ OPT_END()
+ };
+ union cmd cmd = { .cmd = CMD_PING, };
+
+ argc = parse_options(argc, argv, ping_options, daemon_usage, 0);
+ if (argc)
+ usage_with_options(daemon_usage, ping_options);
+
+ if (setup_config(daemon)) {
+ pr_err("failed: config not found\n");
+ return -1;
+ }
+
+ scnprintf(cmd.ping.name, sizeof(cmd.ping.name), "%s", name);
+ return send_cmd(daemon, &cmd);
+}
+
int cmd_daemon(int argc, const char **argv)
{
struct option daemon_options[] = {
@@ -1227,6 +1376,8 @@ int cmd_daemon(int argc, const char **argv)
return __cmd_signal(&__daemon, daemon_options, argc, argv);
else if (!strcmp(argv[0], "stop"))
return __cmd_stop(&__daemon, daemon_options, argc, argv);
+ else if (!strcmp(argv[0], "ping"))
+ return __cmd_ping(&__daemon, daemon_options, argc, argv);
pr_err("failed: unknown command '%s'\n", argv[0]);
return -1;
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH 16/24] perf daemon: Use control to stop session
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (14 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 15/24] perf daemon: Add ping command Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-01-30 23:48 ` [PATCH 17/24] perf daemon: Add up time for daemon/session list Jiri Olsa
` (7 subsequent siblings)
23 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Using 'stop' control command to stop perf record session.
If that fails, falling back to current SIGTERM/SIGKILL pair.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 56 ++++++++++++++++++++++++++++++-------
1 file changed, 46 insertions(+), 10 deletions(-)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index a5a71e0a706c..92dba933027d 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -821,11 +821,25 @@ static int setup_client_socket(struct daemon *daemon)
static void session__kill(struct session *session, struct daemon *daemon)
{
- session__signal(session, SIGTERM);
- if (session__wait(session, daemon, 10)) {
- session__signal(session, SIGKILL);
- session__wait(session, daemon, 10);
- }
+ int how = 0;
+
+ do {
+ switch (how) {
+ case 0:
+ session__control(session, "stop", false);
+ break;
+ case 1:
+ session__signal(session, SIGTERM);
+ break;
+ case 2:
+ session__signal(session, SIGKILL);
+ break;
+ default:
+ break;
+ }
+ how++;
+
+ } while (session__wait(session, daemon, 10));
}
static void daemon__signal(struct daemon *daemon, int sig)
@@ -850,13 +864,35 @@ static void session__remove(struct session *session)
session__free(session);
}
+static void daemon__stop(struct daemon *daemon)
+{
+ struct session *session;
+
+ list_for_each_entry(session, &daemon->sessions, list)
+ session__control(session, "stop", false);
+}
+
static void daemon__kill(struct daemon *daemon)
{
- daemon__signal(daemon, SIGTERM);
- if (daemon__wait(daemon, 10)) {
- daemon__signal(daemon, SIGKILL);
- daemon__wait(daemon, 10);
- }
+ int how = 0;
+
+ do {
+ switch (how) {
+ case 0:
+ daemon__stop(daemon);
+ break;
+ case 1:
+ daemon__signal(daemon, SIGTERM);
+ break;
+ case 2:
+ daemon__signal(daemon, SIGKILL);
+ break;
+ default:
+ break;
+ }
+ how++;
+
+ } while (daemon__wait(daemon, 10));
}
static void __daemon__free(struct daemon *daemon)
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH 17/24] perf daemon: Add up time for daemon/session list
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (15 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 16/24] perf daemon: Use control to stop session Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-01-30 23:48 ` [PATCH 18/24] perf daemon: Add man page for perf-daemon Jiri Olsa
` (6 subsequent siblings)
23 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Display up time for both daemon and sessions.
Example:
# cat ~/.perfconfig
[daemon]
base=/opt/perfdata
[session-cycles]
run = -m 10M -e cycles --overwrite --switch-output -a
[session-sched]
run = -m 20M -e sched:* --overwrite --switch-output -a
Starting the daemon:
# perf daemon start
Get the details with up time:
# perf daemon -v
[778315:daemon] base: /opt/perfdata
output: /opt/perfdata/output
lock: /opt/perfdata/lock
up: 15 minutes
[778316:cycles] perf record -m 20M -e cycles --overwrite --switch-output -a
base: /opt/perfdata/session-cycles
output: /opt/perfdata/session-cycles/output
control: /opt/perfdata/session-cycles/control
ack: /opt/perfdata/session-cycles/ack
up: 10 minutes
[778317:sched] perf record -m 20M -e sched:* --overwrite --switch-output -a
base: /opt/perfdata/session-sched
output: /opt/perfdata/session-sched/output
control: /opt/perfdata/session-sched/control
ack: /opt/perfdata/session-sched/ack
up: 2 minutes
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index 92dba933027d..8ad58383630d 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -23,6 +23,7 @@
#include <sys/wait.h>
#include <poll.h>
#include <sys/stat.h>
+#include <time.h>
#include "builtin.h"
#include "perf.h"
#include "debug.h"
@@ -78,6 +79,7 @@ struct session {
int pid;
struct list_head list;
enum session_state state;
+ time_t start;
};
struct daemon {
@@ -91,6 +93,7 @@ struct daemon {
FILE *out;
char perf[PATH_MAX];
int signal_fd;
+ time_t start;
};
static struct daemon __daemon = {
@@ -318,6 +321,8 @@ static int session__run(struct session *session, struct daemon *daemon)
return -1;
}
+ session->start = time(NULL);
+
session->pid = fork();
if (session->pid < 0)
return -1;
@@ -627,6 +632,7 @@ static int cmd_session_list(struct daemon *daemon, union cmd *cmd, FILE *out)
{
char csv_sep = cmd->list.csv_sep;
struct session *session;
+ time_t curr = time(NULL);
if (csv_sep) {
fprintf(out, "%d%c%s%c%s%c%s/%s",
@@ -641,6 +647,10 @@ static int cmd_session_list(struct daemon *daemon, union cmd *cmd, FILE *out)
/* lock */
csv_sep, daemon->base, "lock");
+ fprintf(out, "%c%lu",
+ /* session up time */
+ csv_sep, (curr - daemon->start) / 60);
+
fprintf(out, "\n");
} else {
fprintf(out, "[%d:daemon] base: %s\n", getpid(), daemon->base);
@@ -649,6 +659,8 @@ static int cmd_session_list(struct daemon *daemon, union cmd *cmd, FILE *out)
daemon->base, SESSION_OUTPUT);
fprintf(out, " lock: %s/lock\n",
daemon->base);
+ fprintf(out, " up: %lu minutes\n",
+ (curr - daemon->start) / 60);
}
}
@@ -674,6 +686,10 @@ static int cmd_session_list(struct daemon *daemon, union cmd *cmd, FILE *out)
/* session ack */
csv_sep, session->base, SESSION_ACK);
+ fprintf(out, "%c%lu",
+ /* session up time */
+ csv_sep, (curr - session->start) / 60);
+
fprintf(out, "\n");
} else {
fprintf(out, "[%d:%s] perf record %s\n",
@@ -688,6 +704,8 @@ static int cmd_session_list(struct daemon *daemon, union cmd *cmd, FILE *out)
session->base, SESSION_CONTROL);
fprintf(out, " ack: %s/%s\n",
session->base, SESSION_ACK);
+ fprintf(out, " up: %lu minutes\n",
+ (curr - session->start) / 60);
}
}
@@ -1176,6 +1194,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
if (argc)
usage_with_options(daemon_usage, start_options);
+ daemon->start = time(NULL);
+
if (setup_config(daemon)) {
pr_err("failed: config not found\n");
return -1;
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH 18/24] perf daemon: Add man page for perf-daemon
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (16 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 17/24] perf daemon: Add up time for daemon/session list Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-02-03 21:17 ` Arnaldo Carvalho de Melo
2021-01-30 23:48 ` [PATCH 19/24] perf tests: Add daemon list command test Jiri Olsa
` (5 subsequent siblings)
23 siblings, 1 reply; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Adding man page for perf-daemon usage.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/Documentation/perf-config.txt | 14 ++
tools/perf/Documentation/perf-daemon.txt | 187 +++++++++++++++++++++++
2 files changed, 201 insertions(+)
diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
index c3ce48f1b379..153bde14bbe0 100644
--- a/tools/perf/Documentation/perf-config.txt
+++ b/tools/perf/Documentation/perf-config.txt
@@ -703,6 +703,20 @@ auxtrace.*::
If the directory does not exist or has the wrong file type,
the current directory is used.
+daemon.*::
+
+ daemon.base::
+ Base path for daemon data. All sessions data are stored under
+ this path.
+
+session-<NAME>.*::
+
+ session-<NAME>.run::
+
+ Defines new record session for daemon. The value is record's
+ command line without the 'record' keyword.
+
+
SEE ALSO
--------
linkperf:perf[1]
diff --git a/tools/perf/Documentation/perf-daemon.txt b/tools/perf/Documentation/perf-daemon.txt
index e69de29bb2d1..b0e1015476c2 100644
--- a/tools/perf/Documentation/perf-daemon.txt
+++ b/tools/perf/Documentation/perf-daemon.txt
@@ -0,0 +1,187 @@
+perf-daemon(1)
+==============
+
+NAME
+----
+perf-daemon - Run record sessions on background
+
+SYNOPSIS
+--------
+[verse]
+'perf daemon'
+'perf daemon' [<options>]
+'perf daemon start' [<options>]
+'perf daemon stop' [<options>]
+'perf daemon signal' [<options>]
+'perf daemon ping' [<options>]
+
+
+DESCRIPTION
+-----------
+This command allows to run simple daemon process that starts and
+monitors configured record sessions.
+
+Each session represents one perf record process started with
+control setup (with perf record --control.. options).
+
+These sessions are configured through config file, see CONFIG FILE
+section with EXAMPLES.
+
+
+OPTIONS
+-------
+--config=<PATH>::
+ Config file path, if not perf will check system and default
+ locations (/etc/perfconfig, $HOME/.perfconfig).
+
+-v::
+--verbose::
+ Be more verbose.
+
+
+All generic options are available also under commands.
+
+
+START COMMAND
+-------------
+The start command creates the daemon process.
+
+-f::
+--foreground::
+ Do not put the process in background.
+
+
+STOP COMMAND
+------------
+The stop command stops all the session and the daemon process.
+
+
+SIGNAL COMMAND
+--------------
+The signal command sends signal to configured sessions.
+
+--session::
+ Send signal to specific session.
+
+
+PING COMMAND
+------------
+The ping command sends control ping to configured sessions.
+
+--session::
+ Send ping to specific session.
+
+
+CONFIG FILE
+-----------
+The daemon is configured within standard perf config file by
+following new variables:
+
+daemon.base:
+ Base path for daemon data. All sessions data are
+ stored under this path.
+
+session-<NAME>.run:
+ Defines new record session. The value is record's command
+ line without the 'record' keyword.
+
+Each perf record session is run in daemon.base/<NAME> directory.
+
+
+EXAMPLES
+--------
+Example with 2 record sessions:
+
+ # cat ~/.perfconfig
+ [daemon]
+ base=/opt/perfdata
+
+ [session-cycles]
+ run = -m 10M -e cycles --overwrite --switch-output -a
+
+ [session-sched]
+ run = -m 20M -e sched:* --overwrite --switch-output -a
+
+
+Starting the daemon:
+
+ # perf daemon start
+
+
+Check sessions:
+
+ # perf daemon
+ [603349:daemon] base: /opt/perfdata
+ [603350:cycles] perf record -m 10M -e cycles --overwrite --switch-output -a
+ [603351:sched] perf record -m 20M -e sched:* --overwrite --switch-output -a
+
+First line is daemon process info with configured daemon base.
+
+
+Check sessions with more info:
+
+ # perf daemon -v
+ [603349:daemon] base: /opt/perfdata
+ output: /opt/perfdata/output
+ lock: /opt/perfdata/lock
+ up: 1 minutes
+ [603350:cycles] perf record -m 10M -e cycles --overwrite --switch-output -a
+ base: /opt/perfdata/session-cycles
+ output: /opt/perfdata/session-cycles/output
+ control: /opt/perfdata/session-cycles/control
+ ack: /opt/perfdata/session-cycles/ack
+ up: 1 minutes
+ [603351:sched] perf record -m 20M -e sched:* --overwrite --switch-output -a
+ base: /opt/perfdata/session-sched
+ output: /opt/perfdata/session-sched/output
+ control: /opt/perfdata/session-sched/control
+ ack: /opt/perfdata/session-sched/ack
+ up: 1 minutes
+
+The 'base' path is daemon/session base.
+The 'lock' file is daemon's lock file guarding that no other
+daemon is running on top of the base.
+The 'output' file is perf record output for specific session.
+The 'control' and 'ack' files are perf control files.
+The 'up' number shows minutes daemon/session is running.
+
+
+Make sure control session is online:
+
+ # perf daemon ping
+ OK cycles
+ OK sched
+
+
+Send USR2 signal to session 'cycles' to generate perf.data file:
+
+ # perf daemon signal --session cycles
+ signal 12 sent to session 'cycles [603452]'
+
+ # tail -2 /opt/perfdata/session-cycles/output
+ [ perf record: dump data: Woken up 1 times ]
+ [ perf record: Dump perf.data.2020123017013149 ]
+
+
+Send USR2 signal to all sessions:
+
+ # perf daemon signal
+ signal 12 sent to session 'cycles [603452]'
+ signal 12 sent to session 'sched [603453]'
+
+ # tail -2 /opt/perfdata/session-cycles/output
+ [ perf record: dump data: Woken up 1 times ]
+ [ perf record: Dump perf.data.2020123017024689 ]
+ # tail -2 /opt/perfdata/session-sched/output
+ [ perf record: dump data: Woken up 1 times ]
+ [ perf record: Dump perf.data.2020123017024713 ]
+
+
+Stop daemon:
+
+ # perf daemon stop
+
+
+SEE ALSO
+--------
+linkperf:perf-record[1], linkperf:perf-config[1]
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [PATCH 18/24] perf daemon: Add man page for perf-daemon
2021-01-30 23:48 ` [PATCH 18/24] perf daemon: Add man page for perf-daemon Jiri Olsa
@ 2021-02-03 21:17 ` Arnaldo Carvalho de Melo
2021-02-04 14:53 ` Jiri Olsa
0 siblings, 1 reply; 72+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-03 21:17 UTC (permalink / raw)
To: Jiri Olsa
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Em Sun, Jan 31, 2021 at 12:48:50AM +0100, Jiri Olsa escreveu:
> Adding man page for perf-daemon usage.
I see you decided to add it at the end, but for consistency, please
consider adding the bare minimum when adding
tools/perf/builtin-daemon.c, then go on adding the options and examples
as the features are being added.
- Arnaldo
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/perf/Documentation/perf-config.txt | 14 ++
> tools/perf/Documentation/perf-daemon.txt | 187 +++++++++++++++++++++++
> 2 files changed, 201 insertions(+)
>
> diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
> index c3ce48f1b379..153bde14bbe0 100644
> --- a/tools/perf/Documentation/perf-config.txt
> +++ b/tools/perf/Documentation/perf-config.txt
> @@ -703,6 +703,20 @@ auxtrace.*::
> If the directory does not exist or has the wrong file type,
> the current directory is used.
>
> +daemon.*::
> +
> + daemon.base::
> + Base path for daemon data. All sessions data are stored under
> + this path.
> +
> +session-<NAME>.*::
> +
> + session-<NAME>.run::
> +
> + Defines new record session for daemon. The value is record's
> + command line without the 'record' keyword.
> +
> +
> SEE ALSO
> --------
> linkperf:perf[1]
> diff --git a/tools/perf/Documentation/perf-daemon.txt b/tools/perf/Documentation/perf-daemon.txt
> index e69de29bb2d1..b0e1015476c2 100644
> --- a/tools/perf/Documentation/perf-daemon.txt
> +++ b/tools/perf/Documentation/perf-daemon.txt
> @@ -0,0 +1,187 @@
> +perf-daemon(1)
> +==============
> +
> +NAME
> +----
> +perf-daemon - Run record sessions on background
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'perf daemon'
> +'perf daemon' [<options>]
> +'perf daemon start' [<options>]
> +'perf daemon stop' [<options>]
> +'perf daemon signal' [<options>]
> +'perf daemon ping' [<options>]
> +
> +
> +DESCRIPTION
> +-----------
> +This command allows to run simple daemon process that starts and
> +monitors configured record sessions.
> +
> +Each session represents one perf record process started with
> +control setup (with perf record --control.. options).
> +
> +These sessions are configured through config file, see CONFIG FILE
> +section with EXAMPLES.
> +
> +
> +OPTIONS
> +-------
> +--config=<PATH>::
> + Config file path, if not perf will check system and default
> + locations (/etc/perfconfig, $HOME/.perfconfig).
> +
> +-v::
> +--verbose::
> + Be more verbose.
> +
> +
> +All generic options are available also under commands.
> +
> +
> +START COMMAND
> +-------------
> +The start command creates the daemon process.
> +
> +-f::
> +--foreground::
> + Do not put the process in background.
> +
> +
> +STOP COMMAND
> +------------
> +The stop command stops all the session and the daemon process.
> +
> +
> +SIGNAL COMMAND
> +--------------
> +The signal command sends signal to configured sessions.
> +
> +--session::
> + Send signal to specific session.
> +
> +
> +PING COMMAND
> +------------
> +The ping command sends control ping to configured sessions.
> +
> +--session::
> + Send ping to specific session.
> +
> +
> +CONFIG FILE
> +-----------
> +The daemon is configured within standard perf config file by
> +following new variables:
> +
> +daemon.base:
> + Base path for daemon data. All sessions data are
> + stored under this path.
> +
> +session-<NAME>.run:
> + Defines new record session. The value is record's command
> + line without the 'record' keyword.
> +
> +Each perf record session is run in daemon.base/<NAME> directory.
> +
> +
> +EXAMPLES
> +--------
> +Example with 2 record sessions:
> +
> + # cat ~/.perfconfig
> + [daemon]
> + base=/opt/perfdata
> +
> + [session-cycles]
> + run = -m 10M -e cycles --overwrite --switch-output -a
> +
> + [session-sched]
> + run = -m 20M -e sched:* --overwrite --switch-output -a
> +
> +
> +Starting the daemon:
> +
> + # perf daemon start
> +
> +
> +Check sessions:
> +
> + # perf daemon
> + [603349:daemon] base: /opt/perfdata
> + [603350:cycles] perf record -m 10M -e cycles --overwrite --switch-output -a
> + [603351:sched] perf record -m 20M -e sched:* --overwrite --switch-output -a
> +
> +First line is daemon process info with configured daemon base.
> +
> +
> +Check sessions with more info:
> +
> + # perf daemon -v
> + [603349:daemon] base: /opt/perfdata
> + output: /opt/perfdata/output
> + lock: /opt/perfdata/lock
> + up: 1 minutes
> + [603350:cycles] perf record -m 10M -e cycles --overwrite --switch-output -a
> + base: /opt/perfdata/session-cycles
> + output: /opt/perfdata/session-cycles/output
> + control: /opt/perfdata/session-cycles/control
> + ack: /opt/perfdata/session-cycles/ack
> + up: 1 minutes
> + [603351:sched] perf record -m 20M -e sched:* --overwrite --switch-output -a
> + base: /opt/perfdata/session-sched
> + output: /opt/perfdata/session-sched/output
> + control: /opt/perfdata/session-sched/control
> + ack: /opt/perfdata/session-sched/ack
> + up: 1 minutes
> +
> +The 'base' path is daemon/session base.
> +The 'lock' file is daemon's lock file guarding that no other
> +daemon is running on top of the base.
> +The 'output' file is perf record output for specific session.
> +The 'control' and 'ack' files are perf control files.
> +The 'up' number shows minutes daemon/session is running.
> +
> +
> +Make sure control session is online:
> +
> + # perf daemon ping
> + OK cycles
> + OK sched
> +
> +
> +Send USR2 signal to session 'cycles' to generate perf.data file:
> +
> + # perf daemon signal --session cycles
> + signal 12 sent to session 'cycles [603452]'
> +
> + # tail -2 /opt/perfdata/session-cycles/output
> + [ perf record: dump data: Woken up 1 times ]
> + [ perf record: Dump perf.data.2020123017013149 ]
> +
> +
> +Send USR2 signal to all sessions:
> +
> + # perf daemon signal
> + signal 12 sent to session 'cycles [603452]'
> + signal 12 sent to session 'sched [603453]'
> +
> + # tail -2 /opt/perfdata/session-cycles/output
> + [ perf record: dump data: Woken up 1 times ]
> + [ perf record: Dump perf.data.2020123017024689 ]
> + # tail -2 /opt/perfdata/session-sched/output
> + [ perf record: dump data: Woken up 1 times ]
> + [ perf record: Dump perf.data.2020123017024713 ]
> +
> +
> +Stop daemon:
> +
> + # perf daemon stop
> +
> +
> +SEE ALSO
> +--------
> +linkperf:perf-record[1], linkperf:perf-config[1]
> --
> 2.29.2
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 18/24] perf daemon: Add man page for perf-daemon
2021-02-03 21:17 ` Arnaldo Carvalho de Melo
@ 2021-02-04 14:53 ` Jiri Olsa
0 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-02-04 14:53 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jiri Olsa, lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan, Ian Rogers,
Stephane Eranian, Alexei Budankov
On Wed, Feb 03, 2021 at 06:17:15PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Jan 31, 2021 at 12:48:50AM +0100, Jiri Olsa escreveu:
> > Adding man page for perf-daemon usage.
>
> I see you decided to add it at the end, but for consistency, please
> consider adding the bare minimum when adding
> tools/perf/builtin-daemon.c, then go on adding the options and examples
> as the features are being added.
sure, will change
thanks,
jirka
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH 19/24] perf tests: Add daemon list command test
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (17 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 18/24] perf daemon: Add man page for perf-daemon Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-01-30 23:48 ` [PATCH 20/24] perf tests: Add daemon reconfig test Jiri Olsa
` (4 subsequent siblings)
23 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Adding test for basic perf daemon listing via the CSV
output mode (-x option).
Checking that configured sessions display expected values.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/tests/shell/daemon.sh | 184 +++++++++++++++++++++++++++++++
1 file changed, 184 insertions(+)
create mode 100755 tools/perf/tests/shell/daemon.sh
diff --git a/tools/perf/tests/shell/daemon.sh b/tools/perf/tests/shell/daemon.sh
new file mode 100755
index 000000000000..874e1fd77c7e
--- /dev/null
+++ b/tools/perf/tests/shell/daemon.sh
@@ -0,0 +1,184 @@
+#!/bin/sh
+# daemon operations
+# SPDX-License-Identifier: GPL-2.0
+
+check_line_first()
+{
+ local line=$1
+ local name=$2
+ local base=$3
+ local output=$4
+ local lock=$5
+ local up=$6
+
+ local line_name=`echo "${line}" | awk 'BEGIN { FS = ":" } ; { print $2 }'`
+ local line_base=`echo "${line}" | awk 'BEGIN { FS = ":" } ; { print $3 }'`
+ local line_output=`echo "${line}" | awk 'BEGIN { FS = ":" } ; { print $4 }'`
+ local line_lock=`echo "${line}" | awk 'BEGIN { FS = ":" } ; { print $5 }'`
+ local line_up=`echo "${line}" | awk 'BEGIN { FS = ":" } ; { print $6 }'`
+
+ if [ "${name}" != "${line_name}" ]; then
+ echo "FAILED: wrong name"
+ error=1
+ fi
+
+ if [ "${base}" != "${line_base}" ]; then
+ echo "FAILED: wrong base"
+ error=1
+ fi
+
+ if [ "${output}" != "${line_output}" ]; then
+ echo "FAILED: wrong output"
+ error=1
+ fi
+
+ if [ "${lock}" != "${line_lock}" ]; then
+ echo "FAILED: wrong lock"
+ error=1
+ fi
+
+ if [ "${up}" != "${line_up}" ]; then
+ echo "FAILED: wrong up"
+ error=1
+ fi
+}
+
+check_line_other()
+{
+ local line=$1
+ local name=$2
+ local run=$3
+ local base=$4
+ local output=$5
+ local control=$6
+ local ack=$7
+ local up=$8
+
+ local line_name=`echo "${line}" | awk 'BEGIN { FS = ":" } ; { print $2 }'`
+ local line_run=`echo "${line}" | awk 'BEGIN { FS = ":" } ; { print $3 }'`
+ local line_base=`echo "${line}" | awk 'BEGIN { FS = ":" } ; { print $4 }'`
+ local line_output=`echo "${line}" | awk 'BEGIN { FS = ":" } ; { print $5 }'`
+ local line_control=`echo "${line}" | awk 'BEGIN { FS = ":" } ; { print $6 }'`
+ local line_ack=`echo "${line}" | awk 'BEGIN { FS = ":" } ; { print $7 }'`
+ local line_up=`echo "${line}" | awk 'BEGIN { FS = ":" } ; { print $8 }'`
+
+ if [ "${name}" != "${line_name}" ]; then
+ echo "FAILED: wrong name"
+ error=1
+ fi
+
+ if [ "${run}" != "${line_run}" ]; then
+ echo "FAILED: wrong run"
+ error=1
+ fi
+
+ if [ "${base}" != "${line_base}" ]; then
+ echo "FAILED: wrong base"
+ error=1
+ fi
+
+ if [ "${output}" != "${line_output}" ]; then
+ echo "FAILED: wrong output"
+ error=1
+ fi
+
+ if [ "${control}" != "${line_control}" ]; then
+ echo "FAILED: wrong control"
+ error=1
+ fi
+
+ if [ "${ack}" != "${line_ack}" ]; then
+ echo "FAILED: wrong ack"
+ error=1
+ fi
+
+ if [ "${up}" != "${line_up}" ]; then
+ echo "FAILED: wrong up"
+ error=1
+ fi
+}
+
+daemon_start()
+{
+ local config=$1
+ local session=$2
+
+ perf daemon start --config ${config}
+
+ # wait for the session to ping
+ local state="FAIL"
+ while [ "${state}" != "OK" ]; do
+ state=`perf daemon ping --config ${config} --session ${session} | awk '{ print $1 }'`
+ sleep 0.05
+ done
+}
+
+daemon_exit()
+{
+ local base=$1
+ local config=$2
+
+ local line=`perf daemon --config ${config} -x: | head -1`
+ local pid=`echo "${line}" | awk 'BEGIN { FS = ":" } ; { print $1 }'`
+
+ # stop daemon
+ perf daemon stop --config ${config}
+
+ # ... and wait for the pid to go away
+ tail --pid=${pid} -f /dev/null
+}
+
+test_list()
+{
+ echo "test daemon list"
+
+ local config=$(mktemp /tmp/perf.daemon.config.XXX)
+ local base=$(mktemp -d /tmp/perf.daemon.base.XXX)
+
+ cat <<EOF > ${config}
+[daemon]
+base=BASE
+
+[session-size]
+run = -e cpu-clock
+
+[session-time]
+run = -e task-clock
+EOF
+
+ sed -i -e "s|BASE|${base}|" ${config}
+
+ # start daemon
+ daemon_start ${config} size
+
+ # check first line
+ # pid:daemon:base:base/output:base/lock
+ local line=`perf daemon --config ${config} -x: | head -1`
+ check_line_first ${line} daemon ${base} ${base}/output ${base}/lock "0"
+
+ # check 1st session
+ # pid:size:-e cpu-clock:base/size:base/size/output:base/size/control:base/size/ack:0
+ local line=`perf daemon --config ${config} -x: | head -2 | tail -1`
+ check_line_other "${line}" size "-e cpu-clock" ${base}/session-size \
+ ${base}/session-size/output ${base}/session-size/control \
+ ${base}/session-size/ack "0"
+
+ # check 2nd session
+ # pid:time:-e task-clock:base/time:base/time/output:base/time/control:base/time/ack:0
+ local line=`perf daemon --config ${config} -x: | head -3 | tail -1`
+ check_line_other "${line}" time "-e task-clock" ${base}/session-time \
+ ${base}/session-time/output ${base}/session-time/control \
+ ${base}/session-time/ack "0"
+
+ # stop daemon
+ daemon_exit ${base} ${config}
+
+ rm -rf ${base}
+ rm -f ${config}
+}
+
+error=0
+
+test_list
+
+exit ${error}
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH 20/24] perf tests: Add daemon reconfig test
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (18 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 19/24] perf tests: Add daemon list command test Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-01-30 23:48 ` [PATCH 21/24] perf tests: Add daemon stop command test Jiri Olsa
` (3 subsequent siblings)
23 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Adding test for daemon reconfiguration. The test changes
the configuration file and checks that the session is
changed properly.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/tests/shell/daemon.sh | 119 +++++++++++++++++++++++++++++++
1 file changed, 119 insertions(+)
diff --git a/tools/perf/tests/shell/daemon.sh b/tools/perf/tests/shell/daemon.sh
index 874e1fd77c7e..90a6e8caddfb 100755
--- a/tools/perf/tests/shell/daemon.sh
+++ b/tools/perf/tests/shell/daemon.sh
@@ -177,8 +177,127 @@ EOF
rm -f ${config}
}
+test_reconfig()
+{
+ echo "test daemon reconfig"
+
+ local config=$(mktemp /tmp/perf.daemon.config.XXX)
+ local base=$(mktemp -d /tmp/perf.daemon.base.XXX)
+
+ # prepare config
+ cat <<EOF > ${config}
+[daemon]
+base=BASE
+
+[session-size]
+run = -e cpu-clock
+
+[session-time]
+run = -e task-clock
+EOF
+
+ sed -i -e "s|BASE|${base}|" ${config}
+
+ # start daemon
+ daemon_start ${config} size
+
+ # check 2nd session
+ # pid:time:-e task-clock:base/time:base/time/output:base/time/control:base/time/ack:0
+ local line=`perf daemon --config ${config} -x: | head -3 | tail -1`
+ check_line_other "${line}" time "-e task-clock" ${base}/session-time \
+ ${base}/session-time/output ${base}/session-time/control ${base}/session-time/ack "0"
+ local pid=`echo "${line}" | awk 'BEGIN { FS = ":" } ; { print $1 }'`
+
+ # prepare new config
+ local config_new=${config}.new
+ cat <<EOF > ${config_new}
+[daemon]
+base=BASE
+
+[session-size]
+run = -e cpu-clock
+
+[session-time]
+run = -e cpu-clock
+EOF
+
+ # TEST 1 - change config
+
+ sed -i -e "s|BASE|${base}|" ${config_new}
+ cp ${config_new} ${config}
+
+ # wait for old session to finish
+ tail --pid=${pid} -f /dev/null
+
+ # wait for new one to start
+ local state="FAIL"
+ while [ "${state}" != "OK" ]; do
+ state=`perf daemon ping --config ${config} --session time | awk '{ print $1 }'`
+ done
+
+ # check reconfigured 2nd session
+ # pid:time:-e task-clock:base/time:base/time/output:base/time/control:base/time/ack:0
+ local line=`perf daemon --config ${config} -x: | head -3 | tail -1`
+ check_line_other "${line}" time "-e cpu-clock" ${base}/session-time \
+ ${base}/session-time/output ${base}/session-time/control ${base}/session-time/ack "0"
+
+ # TEST 2 - empty config
+
+ local config_empty=${config}.empty
+ cat <<EOF > ${config_empty}
+[daemon]
+base=BASE
+EOF
+
+ # change config
+ sed -i -e "s|BASE|${base}|" ${config_empty}
+ cp ${config_empty} ${config}
+
+ # wait for sessions to finish
+ local state="OK"
+ while [ "${state}" != "FAIL" ]; do
+ state=`perf daemon ping --config ${config} --session time | awk '{ print $1 }'`
+ done
+
+ local state="OK"
+ while [ "${state}" != "FAIL" ]; do
+ state=`perf daemon ping --config ${config} --session size | awk '{ print $1 }'`
+ done
+
+ local one=`perf daemon --config ${config} -x: | wc -l`
+
+ if [ ${one} -ne "1" ]; then
+ echo "FAILED: wrong list output"
+ error=1
+ fi
+
+ # TEST 3 - config again
+
+ cp ${config_new} ${config}
+
+ # wait for size to start
+ local state="FAIL"
+ while [ "${state}" != "OK" ]; do
+ state=`perf daemon ping --config ${config} --session size | awk '{ print $1 }'`
+ done
+
+ # wait for time to start
+ local state="FAIL"
+ while [ "${state}" != "OK" ]; do
+ state=`perf daemon ping --config ${config} --session time | awk '{ print $1 }'`
+ done
+
+ # stop daemon
+ daemon_exit ${base} ${config}
+
+ rm -rf ${base}
+ rm -f ${config}
+ rm -f ${config_new}
+ rm -f ${config_empty}
+}
error=0
test_list
+test_reconfig
exit ${error}
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH 21/24] perf tests: Add daemon stop command test
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (19 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 20/24] perf tests: Add daemon reconfig test Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-01-30 23:48 ` [PATCH 22/24] perf tests: Add daemon signal " Jiri Olsa
` (2 subsequent siblings)
23 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Adding test for perf daemon stop command. The test
stops the daemon and verifies all the configured
sessions are properly terminated.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/tests/shell/daemon.sh | 54 ++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/tools/perf/tests/shell/daemon.sh b/tools/perf/tests/shell/daemon.sh
index 90a6e8caddfb..3b6b5aa5587c 100755
--- a/tools/perf/tests/shell/daemon.sh
+++ b/tools/perf/tests/shell/daemon.sh
@@ -295,9 +295,63 @@ EOF
rm -f ${config_new}
rm -f ${config_empty}
}
+
+test_stop()
+{
+ echo "test daemon stop"
+
+ local config=$(mktemp /tmp/perf.daemon.config.XXX)
+ local base=$(mktemp -d /tmp/perf.daemon.base.XXX)
+
+ # prepare config
+ cat <<EOF > ${config}
+[daemon]
+base=BASE
+
+[session-size]
+run = -e cpu-clock
+
+[session-time]
+run = -e task-clock
+EOF
+
+ sed -i -e "s|BASE|${base}|" ${config}
+
+ # start daemon
+ daemon_start ${config} size
+
+ local pid_size=`perf daemon --config ${config} -x: | head -2 | tail -1 | awk 'BEGIN { FS = ":" } ; { print $1 }'`
+ local pid_time=`perf daemon --config ${config} -x: | head -3 | tail -1 | awk 'BEGIN { FS = ":" } ; { print $1 }'`
+
+ # check that sessions are running
+ if [ ! -d "/proc/${pid_size}" ]; then
+ echo "FAILED: session size not up"
+ fi
+
+ if [ ! -d "/proc/${pid_time}" ]; then
+ echo "FAILED: session time not up"
+ fi
+
+ # stop daemon
+ daemon_exit ${base} ${config}
+
+ # check that sessions are gone
+ if [ -d "/proc/${pid_size}" ]; then
+ echo "FAILED: session size still up"
+ fi
+
+ if [ -d "/proc/${pid_time}" ]; then
+ echo "FAILED: session time still up"
+ fi
+
+ rm -rf ${base}
+ rm -f ${config}
+}
+
error=0
test_list
test_reconfig
+test_stop
exit ${error}
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH 22/24] perf tests: Add daemon signal command test
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (20 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 21/24] perf tests: Add daemon stop command test Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-01-30 23:48 ` [PATCH 23/24] perf tests: Add daemon ping " Jiri Olsa
2021-01-30 23:48 ` [PATCH 24/24] perf tests: Add daemon lock test Jiri Olsa
23 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Adding test for perf daemon signal command. The test
sends a signal to configured sessions and verifies
the perf data files were generated accordingly.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/tests/shell/daemon.sh | 40 ++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/tools/perf/tests/shell/daemon.sh b/tools/perf/tests/shell/daemon.sh
index 3b6b5aa5587c..2e414f2c9251 100755
--- a/tools/perf/tests/shell/daemon.sh
+++ b/tools/perf/tests/shell/daemon.sh
@@ -348,10 +348,50 @@ EOF
rm -f ${config}
}
+test_signal()
+{
+ echo "test daemon signal"
+
+ local config=$(mktemp /tmp/perf.daemon.config.XXX)
+ local base=$(mktemp -d /tmp/perf.daemon.base.XXX)
+
+ # prepare config
+ cat <<EOF > ${config}
+[daemon]
+base=BASE
+
+[session-test]
+run = -e cpu-clock --switch-output
+EOF
+
+ sed -i -e "s|BASE|${base}|" ${config}
+
+ # start daemon
+ daemon_start ${config} test
+
+ # send 2 signals
+ perf daemon signal --config ${config} --session test
+ perf daemon signal --config ${config}
+
+ # stop daemon
+ daemon_exit ${base} ${config}
+
+ # count is 2 perf.data for signals and 1 for perf record finished
+ count=`ls ${base}/session-test/ | grep perf.data | wc -l`
+ if [ ${count} -ne 3 ]; then
+ error=1
+ echo "FAILED: perf data no generated"
+ fi
+
+ rm -rf ${base}
+ rm -f ${config}
+}
+
error=0
test_list
test_reconfig
test_stop
+test_signal
exit ${error}
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH 23/24] perf tests: Add daemon ping command test
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (21 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 22/24] perf tests: Add daemon signal " Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
2021-01-30 23:48 ` [PATCH 24/24] perf tests: Add daemon lock test Jiri Olsa
23 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Adding test for perf daemon ping command. The tests
verifies the ping command gets proper answer from
sessions.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/tests/shell/daemon.sh | 40 ++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/tools/perf/tests/shell/daemon.sh b/tools/perf/tests/shell/daemon.sh
index 2e414f2c9251..9fcb98768633 100755
--- a/tools/perf/tests/shell/daemon.sh
+++ b/tools/perf/tests/shell/daemon.sh
@@ -387,11 +387,51 @@ EOF
rm -f ${config}
}
+test_ping()
+{
+ echo "test daemon ping"
+
+ local config=$(mktemp /tmp/perf.daemon.config.XXX)
+ local base=$(mktemp -d /tmp/perf.daemon.base.XXX)
+
+ # prepare config
+ cat <<EOF > ${config}
+[daemon]
+base=BASE
+
+[session-size]
+run = -e cpu-clock
+
+[session-time]
+run = -e task-clock
+EOF
+
+ sed -i -e "s|BASE|${base}|" ${config}
+
+ # start daemon
+ daemon_start ${config} size
+
+ size=`perf daemon ping --config ${config} --session size | awk '{ print $1 }'`
+ type=`perf daemon ping --config ${config} --session time | awk '{ print $1 }'`
+
+ if [ ${size} != "OK" -o ${type} != "OK" ]; then
+ error=1
+ echo "FAILED: daemon ping failed"
+ fi
+
+ # stop daemon
+ daemon_exit ${base} ${config}
+
+ rm -rf ${base}
+ rm -f ${config}
+}
+
error=0
test_list
test_reconfig
test_stop
test_signal
+test_ping
exit ${error}
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread* [PATCH 24/24] perf tests: Add daemon lock test
2021-01-30 23:48 ` [PATCHv3 00/24] perf tools: Add daemon command Jiri Olsa
` (22 preceding siblings ...)
2021-01-30 23:48 ` [PATCH 23/24] perf tests: Add daemon ping " Jiri Olsa
@ 2021-01-30 23:48 ` Jiri Olsa
23 siblings, 0 replies; 72+ messages in thread
From: Jiri Olsa @ 2021-01-30 23:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers, Stephane Eranian,
Alexei Budankov
Adding test for perf daemon lock ensuring only
one instance of daemon can run over one base
directory.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/tests/shell/daemon.sh | 38 ++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/tools/perf/tests/shell/daemon.sh b/tools/perf/tests/shell/daemon.sh
index 9fcb98768633..e5b824dd08d9 100755
--- a/tools/perf/tests/shell/daemon.sh
+++ b/tools/perf/tests/shell/daemon.sh
@@ -426,6 +426,43 @@ EOF
rm -f ${config}
}
+test_lock()
+{
+ echo "test daemon lock"
+
+ local config=$(mktemp /tmp/perf.daemon.config.XXX)
+ local base=$(mktemp -d /tmp/perf.daemon.base.XXX)
+
+ # prepare config
+ cat <<EOF > ${config}
+[daemon]
+base=BASE
+
+[session-size]
+run = -e cpu-clock
+EOF
+
+ sed -i -e "s|BASE|${base}|" ${config}
+
+ # start daemon
+ daemon_start ${config} size
+
+ # start second daemon over the same config/base
+ failed=`perf daemon start --config ${config} 2>&1 | awk '{ print $1 }'`
+
+ # check that we failed properly
+ if [ ${failed} != "failed:" ]; then
+ error=1
+ echo "FAILED: daemon lock failed"
+ fi
+
+ # stop daemon
+ daemon_exit ${base} ${config}
+
+ rm -rf ${base}
+ rm -f ${config}
+}
+
error=0
test_list
@@ -433,5 +470,6 @@ test_reconfig
test_stop
test_signal
test_ping
+test_lock
exit ${error}
--
2.29.2
^ permalink raw reply related [flat|nested] 72+ messages in thread