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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 15 Dec 2020 20:43:54 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     Alexei Budankov <abudankov@...wei.com>
Cc:     Jiri Olsa <jolsa@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Ingo Molnar <mingo@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Namhyung Kim <namhyung@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Michael Petlan <mpetlan@...hat.com>,
        Ian Rogers <irogers@...gle.com>,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 4/8] perf daemon: Add daemon command

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