Re: [PATCH 4/8] perf daemon: Add daemon command

From: Jiri Olsa
Date: Tue Dec 15 2020 - 14:46:15 EST


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