On Tue, Dec 15, 2020 at 06:40:26PM +0300, Alexei Budankov wrote:
> Hi,
>
> On 12.12.2020 13:43, Jiri Olsa wrote:
> > Adding daemon command that allows to run record sessions
> > on background. Each session represents one perf record
> > process and is configured in config file.
> >
> > Example:
> >
> > # cat config.daemon
> > [daemon]
> > base=/opt/perfdata
>
> It could probably make sense to consider using locations at /var/
> directory, similar to other already existing daemon processes in
> system so admin and user experience would be easily reusabe for
> performance monitoring daemon (service).
hm, you can specify any /var path in there if you like,
do you suggest to hardcode it?
>
> >
> > [session-1]
> > run = -m 10M -e cycles -o /opt/perfdata/1/perf.data --overwrite
> > --switch-output -a
> >
> > [session-2]
> > run = -m 20M -e sched:* -o /opt/perfdata/2/perf.data --overwrite
> > --switch-output -a
> >
> > Default perf config has the same daemon base:
> >
> > # cat ~/.perfconfig
> > [daemon]
> > base=/opt/perfdata
> >
> > Starting the daemon:
> >
> > # perf daemon --config config.daemon
>
> It could make sense to name daemon config file similar to .perfconfig
> e.g. like .perfconfig.daemon. perf daemon command would then assume, by
> default, usage of .perfconfig.daemon config or the one specified on the
> command line via --config option. It also would be helpfull have loaded
> config file path printed into console:
> # perf daemon
> Daemon process <pid> started with config /path/to/.perfconfig.daemon
so the current way is, that following creates daemon:
# perf daemon --config <CONFIG>
and any other 'non --config' option' is used to 'query/control' daemon:
# perf daemon
# perf daemon --signal
# perf daemon --stop
...
I'd like to keep short way checking on daemon, without too many
options, like:
# perf daemon
[690174:daemon] base: /opt/perfdata
[690175:top] perf record -e cycles --switch-output=1m --switch-max-files=6 -a
I think maybe we don't need any other .perfconfig, we could have
all in standard .perfconfig, like:
# cat .perfconfig:
[daemon]
base=/opt/perfdata
[session-1]
run = -m 1M -e cycles --overwrite --switch-output -a
[session-2]
run = -m 1M -e sched:* --overwrite --switch-output -a
and to run daemon on top of it:
# perf daemon --start
to run daemon with alternate config:
# perf daemon --start=<CONFIGFILE>
or:
# perf daemon --start --config=<CONFIGFILE>
and checking on daemon with default .perfconfig setup:
# perf daemon
checking on daemon with different base or config:
# perf daemon --base=<BASE>
# perf daemon --config=<CONFIGFILE>
# perf daemon --base=<BASE> --stop
# perf daemon --base=<BASE> --signal
# perf daemon --config=<CONFIGFILE> --stop
# perf daemon --config=<CONFIGFILE> --signal
how about that?
SNIP
> > +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 session_name(const char *var, char *session, int len)
>
> should possibly name it get_session_name.
ok
>
> > +{
> > + const char *p = var + sizeof("session-") - 1;
>
> should possibly check that p still points inside [var, var+len).
ok
SNIP
> > +static int session__wait(struct session *session, struct daemon *daemon,
> > + int secs)
> > +{
> > + time_t current, start = 0;
> > + int cnt;
> > +
> > + start = current = time(NULL);
> > +
> > + do {
> > + usleep(500);
>
> This polling design is actually sub-optimal because it induces redundant
> noise in a system. Ideally it should be implemented in async fashion so
> kernel would atomically notify daemon process on event happened in some
> of record processes e.g. using of poll-like() system call.
ok, any suggestion?
>
> > + cnt = session__check(session, daemon);
> > + if (cnt)
> > + break;
> > +
> > + current = time(NULL);
> > + } while ((start + secs > current));
> > +
> > + return cnt;
> > +}
> > +
> > +static int session__signal(struct session *session, int sig)
> > +{
> > + if (session->pid < 0)
> > + return -1;
> > + return kill(session->pid, sig);
>
> "Better" alternative could possibly be sending of some 'stop' command
> via --control=fd.
true, nice idea.. seems more clean and we already have control fd open
will add it to next version
thanks,
jirka
