<div dir="ltr">Thanks Ben for the answers.<div><br></div><div>Few more questions.<br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 6, 2013 at 4:27 PM, Ben Pfaff <span dir="ltr">&lt;<a href="mailto:blp@nicira.com" target="_blank">blp@nicira.com</a>&gt;</span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">On Thu, Jun 06, 2013 at 09:49:11AM -0700, Alex Wang wrote:<br>

&gt; The 2/17. 3/17 look good to me.<br>
<br>
</div>Thanks.<br>
<div class="im"><br>
&gt; For this patch, want to ask few questions:<br>
&gt;<br>
&gt; 1. why does the previous implementation cannot guarantee thread safety (An<br>
&gt; example?)? Is this related to the sigchld_ related functions?<br>
<br>
</div>A single-threaded process can ensure that a signal handler doesn&#39;t<br>
run during a section of code by blocking and unblocking the signal<br>
around that section of code.  A multithreaded process can&#39;t do that,<br>
because the signal handler might get invoked from any thread. </blockquote><div><br></div><div><br></div><div style>Seem to me that we make a rule here. We only want single threaded process to call &quot;process_init() and process_start()&quot;, right?</div>
<div style><br></div><div style><br></div><div style>Also, I want to ask why do you remove the sigchld_ related functions? Is that because the &quot;<span style="color:rgb(0,0,0)">xpthread_sigmask()</span>&quot; is not thread safe?</div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">
&gt; 2. For the &quot;process_register()&quot;, the comment still talks about blocking<br>
&gt; SIGCHLD signal, which can be confusing<br>
<br>
</div>Good catch.  I deleted that sentence from the comment, since it&#39;s no<br>
longer correct.  (It was also ungrammatical.)<br>
<div class=""><div class="h5"><br>
&gt; On Wed, Jun 5, 2013 at 1:05 PM, Ben Pfaff &lt;<a href="mailto:blp@nicira.com">blp@nicira.com</a>&gt; wrote:<br>
&gt;<br>
&gt; &gt; Signed-off-by: Ben Pfaff &lt;<a href="mailto:blp@nicira.com">blp@nicira.com</a>&gt;<br>
&gt; &gt; ---<br>
&gt; &gt;  lib/process.c        |  103<br>
&gt; &gt; ++++++++++++++++----------------------------------<br>
&gt; &gt;  lib/process.h        |    1 +<br>
&gt; &gt;  ovsdb/ovsdb-server.c |    7 ++-<br>
&gt; &gt;  3 files changed, 39 insertions(+), 72 deletions(-)<br>
&gt; &gt;<br>
&gt; &gt; diff --git a/lib/process.c b/lib/process.c<br>
&gt; &gt; index 4102308..22dd266 100644<br>
&gt; &gt; --- a/lib/process.c<br>
&gt; &gt; +++ b/lib/process.c<br>
&gt; &gt; @@ -44,9 +44,9 @@ struct process {<br>
&gt; &gt;      char *name;<br>
&gt; &gt;      pid_t pid;<br>
&gt; &gt;<br>
&gt; &gt; -    /* Modified by signal handler. */<br>
&gt; &gt; -    volatile bool exited;<br>
&gt; &gt; -    volatile int status;<br>
&gt; &gt; +    /* State. */<br>
&gt; &gt; +    bool exited;<br>
&gt; &gt; +    int status;<br>
&gt; &gt;  };<br>
&gt; &gt;<br>
&gt; &gt;  /* Pipe used to signal child termination. */<br>
&gt; &gt; @@ -55,9 +55,6 @@ static int fds[2];<br>
&gt; &gt;  /* All processes. */<br>
&gt; &gt;  static struct list all_processes = LIST_INITIALIZER(&amp;all_processes);<br>
&gt; &gt;<br>
&gt; &gt; -static bool sigchld_is_blocked(void);<br>
&gt; &gt; -static void block_sigchld(sigset_t *);<br>
&gt; &gt; -static void unblock_sigchld(const sigset_t *);<br>
&gt; &gt;  static void sigchld_handler(int signr OVS_UNUSED);<br>
&gt; &gt;<br>
&gt; &gt;  /* Initializes the process subsystem (if it is not already initialized).<br>
&gt; &gt;  Calls<br>
&gt; &gt; @@ -155,8 +152,6 @@ process_register(const char *name, pid_t pid)<br>
&gt; &gt;      struct process *p;<br>
&gt; &gt;      const char *slash;<br>
&gt; &gt;<br>
&gt; &gt; -    ovs_assert(sigchld_is_blocked());<br>
&gt; &gt; -<br>
&gt; &gt;      p = xzalloc(sizeof *p);<br>
&gt; &gt;      p-&gt;pid = pid;<br>
&gt; &gt;      slash = strrchr(name, &#39;/&#39;);<br>
&gt; &gt; @@ -181,7 +176,6 @@ process_register(const char *name, pid_t pid)<br>
&gt; &gt;  int<br>
&gt; &gt;  process_start(char **argv, struct process **pp)<br>
&gt; &gt;  {<br>
&gt; &gt; -    sigset_t oldsigs;<br>
&gt; &gt;      pid_t pid;<br>
&gt; &gt;      int error;<br>
&gt; &gt;<br>
&gt; &gt; @@ -192,16 +186,13 @@ process_start(char **argv, struct process **pp)<br>
&gt; &gt;          return error;<br>
&gt; &gt;      }<br>
&gt; &gt;<br>
&gt; &gt; -    block_sigchld(&amp;oldsigs);<br>
&gt; &gt;      pid = fork();<br>
&gt; &gt;      if (pid &lt; 0) {<br>
&gt; &gt; -        unblock_sigchld(&amp;oldsigs);<br>
&gt; &gt;          VLOG_WARN(&quot;fork failed: %s&quot;, strerror(errno));<br>
&gt; &gt;          return errno;<br>
&gt; &gt;      } else if (pid) {<br>
&gt; &gt;          /* Running in parent process. */<br>
&gt; &gt;          *pp = process_register(argv[0], pid);<br>
&gt; &gt; -        unblock_sigchld(&amp;oldsigs);<br>
&gt; &gt;          return 0;<br>
&gt; &gt;      } else {<br>
&gt; &gt;          /* Running in child process. */<br>
&gt; &gt; @@ -209,7 +200,6 @@ process_start(char **argv, struct process **pp)<br>
&gt; &gt;          int fd;<br>
&gt; &gt;<br>
&gt; &gt;          fatal_signal_fork();<br>
&gt; &gt; -        unblock_sigchld(&amp;oldsigs);<br>
&gt; &gt;          for (fd = 3; fd &lt; fd_max; fd++) {<br>
&gt; &gt;              close(fd);<br>
&gt; &gt;          }<br>
&gt; &gt; @@ -225,12 +215,7 @@ void<br>
&gt; &gt;  process_destroy(struct process *p)<br>
&gt; &gt;  {<br>
&gt; &gt;      if (p) {<br>
&gt; &gt; -        sigset_t oldsigs;<br>
&gt; &gt; -<br>
&gt; &gt; -        block_sigchld(&amp;oldsigs);<br>
&gt; &gt;          list_remove(&amp;p-&gt;node);<br>
&gt; &gt; -        unblock_sigchld(&amp;oldsigs);<br>
&gt; &gt; -<br>
&gt; &gt;          free(p-&gt;name);<br>
&gt; &gt;          free(p);<br>
&gt; &gt;      }<br>
&gt; &gt; @@ -265,13 +250,7 @@ process_name(const struct process *p)<br>
&gt; &gt;  bool<br>
&gt; &gt;  process_exited(struct process *p)<br>
&gt; &gt;  {<br>
&gt; &gt; -    if (p-&gt;exited) {<br>
&gt; &gt; -        return true;<br>
&gt; &gt; -    } else {<br>
&gt; &gt; -        char buf[_POSIX_PIPE_BUF];<br>
&gt; &gt; -        ignore(read(fds[0], buf, sizeof buf));<br>
&gt; &gt; -        return false;<br>
&gt; &gt; -    }<br>
&gt; &gt; +    return p-&gt;exited;<br>
&gt; &gt;  }<br>
&gt; &gt;<br>
&gt; &gt;  /* Returns process &#39;p&#39;&#39;s exit status, as reported by waitpid(2).<br>
&gt; &gt; @@ -313,6 +292,35 @@ process_status_msg(int status)<br>
&gt; &gt;      return ds_cstr(&amp;ds);<br>
&gt; &gt;  }<br>
&gt; &gt;<br>
&gt; &gt; +/* Executes periodic maintenance activities required by the process<br>
&gt; &gt; module. */<br>
&gt; &gt; +void<br>
&gt; &gt; +process_run(void)<br>
&gt; &gt; +{<br>
&gt; &gt; +    char buf[_POSIX_PIPE_BUF];<br>
&gt; &gt; +<br>
&gt; &gt; +    if (!list_is_empty(&amp;all_processes) &amp;&amp; read(fds[0], buf, sizeof buf) &gt;<br>
&gt; &gt; 0) {<br>
&gt; &gt; +        struct process *p;<br>
&gt; &gt; +<br>
&gt; &gt; +        LIST_FOR_EACH (p, node, &amp;all_processes) {<br>
&gt; &gt; +            if (!p-&gt;exited) {<br>
&gt; &gt; +                int retval, status;<br>
&gt; &gt; +                do {<br>
&gt; &gt; +                    retval = waitpid(p-&gt;pid, &amp;status, WNOHANG);<br>
&gt; &gt; +                } while (retval == -1 &amp;&amp; errno == EINTR);<br>
&gt; &gt; +                if (retval == p-&gt;pid) {<br>
&gt; &gt; +                    p-&gt;exited = true;<br>
&gt; &gt; +                    p-&gt;status = status;<br>
&gt; &gt; +                } else if (retval &lt; 0) {<br>
&gt; &gt; +                    VLOG_WARN(&quot;waitpid: %s&quot;, strerror(errno));<br>
&gt; &gt; +                    p-&gt;exited = true;<br>
&gt; &gt; +                    p-&gt;status = -1;<br>
&gt; &gt; +                }<br>
&gt; &gt; +            }<br>
&gt; &gt; +        }<br>
&gt; &gt; +    }<br>
&gt; &gt; +}<br>
&gt; &gt; +<br>
&gt; &gt; +<br>
&gt; &gt;  /* Causes the next call to poll_block() to wake up when process &#39;p&#39; has<br>
&gt; &gt;   * exited. */<br>
&gt; &gt;  void<br>
&gt; &gt; @@ -353,50 +361,5 @@ process_search_path(const char *name)<br>
&gt; &gt;  static void<br>
&gt; &gt;  sigchld_handler(int signr OVS_UNUSED)<br>
&gt; &gt;  {<br>
&gt; &gt; -    struct process *p;<br>
&gt; &gt; -<br>
&gt; &gt; -    COVERAGE_INC(process_sigchld);<br>
&gt; &gt; -    LIST_FOR_EACH (p, node, &amp;all_processes) {<br>
&gt; &gt; -        if (!p-&gt;exited) {<br>
&gt; &gt; -            int retval, status;<br>
&gt; &gt; -            do {<br>
&gt; &gt; -                retval = waitpid(p-&gt;pid, &amp;status, WNOHANG);<br>
&gt; &gt; -            } while (retval == -1 &amp;&amp; errno == EINTR);<br>
&gt; &gt; -            if (retval == p-&gt;pid) {<br>
&gt; &gt; -                p-&gt;exited = true;<br>
&gt; &gt; -                p-&gt;status = status;<br>
&gt; &gt; -            } else if (retval &lt; 0) {<br>
&gt; &gt; -                /* XXX We want to log something but we&#39;re in a signal<br>
&gt; &gt; -                 * handler. */<br>
&gt; &gt; -                p-&gt;exited = true;<br>
&gt; &gt; -                p-&gt;status = -1;<br>
&gt; &gt; -            }<br>
&gt; &gt; -        }<br>
&gt; &gt; -    }<br>
&gt; &gt;      ignore(write(fds[1], &quot;&quot;, 1));<br>
&gt; &gt;  }<br>
&gt; &gt; -<br>
&gt; &gt; -static bool<br>
&gt; &gt; -sigchld_is_blocked(void)<br>
&gt; &gt; -{<br>
&gt; &gt; -    sigset_t sigs;<br>
&gt; &gt; -<br>
&gt; &gt; -    xpthread_sigmask(SIG_SETMASK, NULL, &amp;sigs);<br>
&gt; &gt; -    return sigismember(&amp;sigs, SIGCHLD);<br>
&gt; &gt; -}<br>
&gt; &gt; -<br>
&gt; &gt; -static void<br>
&gt; &gt; -block_sigchld(sigset_t *oldsigs)<br>
&gt; &gt; -{<br>
&gt; &gt; -    sigset_t sigchld;<br>
&gt; &gt; -<br>
&gt; &gt; -    sigemptyset(&amp;sigchld);<br>
&gt; &gt; -    sigaddset(&amp;sigchld, SIGCHLD);<br>
&gt; &gt; -    xpthread_sigmask(SIG_BLOCK, &amp;sigchld, oldsigs);<br>
&gt; &gt; -}<br>
&gt; &gt; -<br>
&gt; &gt; -static void<br>
&gt; &gt; -unblock_sigchld(const sigset_t *oldsigs)<br>
&gt; &gt; -{<br>
&gt; &gt; -    xpthread_sigmask(SIG_SETMASK, oldsigs, NULL);<br>
&gt; &gt; -}<br>
&gt; &gt; diff --git a/lib/process.h b/lib/process.h<br>
&gt; &gt; index 6d1410b..d17737d 100644<br>
&gt; &gt; --- a/lib/process.h<br>
&gt; &gt; +++ b/lib/process.h<br>
&gt; &gt; @@ -33,6 +33,7 @@ bool process_exited(struct process *);<br>
&gt; &gt;  int process_status(const struct process *);<br>
&gt; &gt;  char *process_status_msg(int);<br>
&gt; &gt;<br>
&gt; &gt; +void process_run(void);<br>
&gt; &gt;  void process_wait(struct process *);<br>
&gt; &gt;<br>
&gt; &gt;  char *process_search_path(const char *);<br>
&gt; &gt; diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c<br>
&gt; &gt; index b9afd8c..20e1964 100644<br>
&gt; &gt; --- a/ovsdb/ovsdb-server.c<br>
&gt; &gt; +++ b/ovsdb/ovsdb-server.c<br>
&gt; &gt; @@ -230,8 +230,11 @@ main(int argc, char *argv[])<br>
&gt; &gt;          for (i = 0; i &lt; n_dbs; i++) {<br>
&gt; &gt;              ovsdb_trigger_run(dbs[i].db, time_msec());<br>
&gt; &gt;          }<br>
&gt; &gt; -        if (run_process &amp;&amp; process_exited(run_process)) {<br>
&gt; &gt; -            exiting = true;<br>
&gt; &gt; +        if (run_process) {<br>
&gt; &gt; +            process_run();<br>
&gt; &gt; +            if (process_exited(run_process)) {<br>
&gt; &gt; +                exiting = true;<br>
&gt; &gt; +            }<br>
&gt; &gt;          }<br>
&gt; &gt;<br>
&gt; &gt;          /* update Manager status(es) every 5 seconds */<br>
&gt; &gt; --<br>
&gt; &gt; 1.7.2.5<br>
&gt; &gt;<br>
&gt; &gt; _______________________________________________<br>
&gt; &gt; dev mailing list<br>
&gt; &gt; <a href="mailto:dev@openvswitch.org">dev@openvswitch.org</a><br>
&gt; &gt; <a href="http://openvswitch.org/mailman/listinfo/dev" target="_blank">http://openvswitch.org/mailman/listinfo/dev</a><br>
&gt; &gt;<br>
</div></div></blockquote></div><br></div></div></div>