Message-ID: <c3981c21-e970-6e27-af2a-364cbcdc6f2e@huawei.com>
Date:   Wed, 16 Dec 2020 10:54:43 +0300
From:   Alexei Budankov <abudankov@...wei.com>
To:     Jiri Olsa <jolsa@...hat.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 15.12.2020 22:43, Jiri Olsa wrote:
> 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?

This thing: https://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard
Since Perf is a part of OS it would better use some standardized locations.

> 
>>
>>>
>>>   [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?

Extending .perfconfig would look simpler for users, IHMO.
It looks like --base option actually implements --sandbox
or similar semantics.

> 
> 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?

Possibly, checking SIGCHLDs via signalfd [1] OR using pidfd [2] on kernel v5.3+

[1] https://man7.org/linux/man-pages/man2/signalfd.2.html
[2] https://man7.org/linux/man-pages/man2/pidfd_open.2.html

Thanks,
Alexei

> 
>>
>>> +		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
> 
> .
> 

Powered by blists - more mailing lists