Date:   Tue, 19 Jan 2021 19:31:19 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     Namhyung Kim <namhyung@...nel.org>
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>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Michael Petlan <mpetlan@...hat.com>,
        Ian Rogers <irogers@...gle.com>,
        Stephane Eranian <eranian@...gle.com>,
        Alexei Budankov <abudankov@...wei.com>
Subject: Re: [PATCH 07/22] perf daemon: Add daemon command

On Tue, Jan 19, 2021 at 01:08:17PM +0900, Namhyung Kim wrote:

SNIP

> > +               if (!session)
> > +                       return -ENOMEM;
> > +
> > +               pr_debug("reconfig: found new session %s\n", name);
> > +               /* This is new session, trigger reconfig to start it. */
> > +               session->state = SESSION_STATE__RECONFIG;
> > +       } else if (session->state == SESSION_STATE__KILL) {
> > +               /*
> > +                * The session was marked to kill and we still
> > +                * found it in config file.
> > +                */
> > +               pr_debug("reconfig: found current session %s\n", name);
> > +               session->state = SESSION_STATE__OK;
> > +       }
> > +
> > +       if (!strcmp(var, "run")) {
> > +               if (session->run && strcmp(session->run, value)) {
> > +                       free(session->run);
> > +                       pr_debug("reconfig: session %s is changed\n", name);
> > +                       session->state = SESSION_STATE__RECONFIG;
> > +               }
> > +               session->run = strdup(value);
> 
> Please check the result.

right, will add

> 
> > +       }
> > +
> > +       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 = strdup(value);
> 
> It seems these config items are mandatory.  Is there a check
> for their presence?

base needs to be there, I'll add the check

missing session should just put daemon to sleep and it
should be woken up by config file change

that seems like a good test actualy, will try to add it

> 
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int client_config(const char *var, const char *value, void *cb)
> > +{
> > +       struct daemon *daemon = cb;
> > +
> > +       if (!strcmp(var, "daemon.base"))
> > +               daemon->base = strdup(value);
> > +
> > +       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;
> > +}
> > +
> > +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 session for kill, the server config will
> > +        * set proper state for found sessions.
> > +        */
> > +       list_for_each_entry(session, &daemon->sessions, list)
> > +               session->state = SESSION_STATE__KILL;
> 
> Probably we can put them in a different state like INIT or READY?

it's convenient, because all session we find will be changed
to OK and the rest will be killed

SNIP

> > +               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("chdir failed");
> > +               return -1;
> > +       }
> > +
> > +       fd = open("/dev/null", O_RDONLY);
> > +       if (fd < 0) {
> > +               perror("failed to open /dev/null");
> > +               return -1;
> > +       }
> > +
> > +       close(0);
> > +       dup2(fd, 0);
> 
> The man page says dup2() will close the second file descriptor.

right, I overlooekd that, close calls are not needed then

SNIP

> > +               /* Reconfig session. */
> > +               pr_debug2("reconfig: session '%s' start\n", session->name);
> > +               if (session->pid > 0) {
> > +                       session__kill(session);
> > +                       pr_info("reconfig: session '%s' killed\n", session->name);
> > +               }
> > +               if (session__run(session, daemon))
> 
> Does it call a config function?  Or is it called already?

daemon__reconfig kills and starts sessions based
on the config data read by setup_server_config

> 
> > +                       return -1;
> > +               pr_debug2("reconfig: session '%s' done\n", session->name);
> > +               session->state = SESSION_STATE__OK;
> 
> I think RUNNING is a better name.

I'll check, I'll put the state graph in comment,
so we can discuss the changes

thanks,
jirka

Powered by blists - more mailing lists