Lon
Good work! Thanks for the original patch.
I merged Fabio's rev of the patch.
I had originally thought forked processes inherited a copy of the forked
threads, but this is not the case.
Regards
-steve
On Thu, 2008-04-17 at 08:34 +0200, Fabio M. Di Nitto wrote:
> Hi Lon
>
> On Wed, 16 Apr 2008, Lon Hohberger wrote:
>
> > Hi Fabio,
> >
> > I'd call this a 'pass 1' patch.
>
> 'pass 2' here :)
>
> >
> > * This makes your test program work.
> > * This does not cause a regression with the existing logsys test cases
> > in the openais trunk.
>
> confirmed. I was able to run ccsd without any problem.
>
> > It, however, dirties up the build system because it requires changing
> > the linking order (logsys now needs -lpthread after it. For some
> > reason, it didn't before (despite using other pthread APIs); someone
> > probably knows why - as well as the correct way to fix it within the
> > context of the openais build system.
>
> I have been looking into this because my original patch was very close to
> your but i don't have the definite answer other than linking with external
> libraries should happen at the end in order to resolve all symbols.
>
> So for example:
> $(CC) $(LDFLAGS) $(EXEC_OBJS) $(EXEC_LIBS) -o aisexec
> needs to turn into:
> $(CC) $(EXEC_OBJS) $(EXEC_LIBS) -o aisexec $(LDFLAGS)
>
> what still puzzles me is that we need to link all applications that uses
> liblogsys to pthread and this was not necessary before but i am sure i am
> overlooking at something. _maybe_ the fact that pthread_atfork interacts
> directly with the application fork() invokation needs this explicit
> linking.
>
> > It would be *potentially* cleaner to make the child "cleanup" function
> > an explicitly-called function, but probably less nice from a user API
> > perspective. Basically, pthread_atfork() makes it a pretty simple thing
> > since the user doesn't need to do anything in the child process (I think
> > pthread_atfork was primarily designed for use in threaded APIs, anyway).
> >
> > Minimally, we needed to zap the mutexes and the thread indicators (since
> > there's no threads in the child process... yet).
> >
> > Upon further investigation, I noticed a double-lock when doing
> > _log_printf around the config mutex (_log_printf does a lock and so does
> > log_printf_worker_fn, causing the child to hang on itself). To remedy
> > this, I added a check for logsys_wthread_active (which will be 0 in the
> > child process unless a full reinitialization is done) - but I'm not sure
> > if this is the right thing to do or not.
>
> It seems to work fine here. I guess in one way or another it is the right
> thing to do.
>
> Lon, thanks a lot for this patch.. Steven, can we please drive it into svn
> at somepoing?
>
> Thanks
> Fabio
>
>
> diff -urNad openais-0.82~/exec/Makefile openais-0.82/exec/Makefile
> --- openais-0.82~/exec/Makefile 2008-04-17 07:27:02.000000000 +0200
> +++ openais-0.82/exec/Makefile 2008-04-17 07:29:20.000000000 +0200
> @@ -175,7 +175,7 @@
> endif
>
> aisexec: $(EXEC_OBJS) $(EXEC_LIBS)
> - $(CC) $(LDFLAGS) $(EXEC_OBJS) $(EXEC_LIBS) -o aisexec
> + $(CC) $(EXEC_OBJS) $(EXEC_LIBS) -o aisexec $(LDFLAGS)
>
> libtotem_pg.a: $(TOTEM_OBJS)
> $(AR) -rc libtotem_pg.a $(TOTEM_OBJS)
> diff -urNad openais-0.82~/exec/logsys.c openais-0.82/exec/logsys.c
> --- openais-0.82~/exec/logsys.c 2008-04-17 07:27:02.000000000 +0200
> +++ openais-0.82/exec/logsys.c 2008-04-17 07:27:23.000000000 +0200
> @@ -255,7 +255,8 @@
> {
> struct log_data *log_data = (struct log_data *)work_item;
>
> - pthread_mutex_lock (&logsys_config_mutex);
> + if (logsys_wthread_active)
> + pthread_mutex_lock (&logsys_config_mutex);
> /*
> * Output the log data
> */
> @@ -273,7 +274,8 @@
> &log_data->log_string[log_data->syslog_pos]);
> }
> free (log_data->log_string);
> - pthread_mutex_unlock (&logsys_config_mutex);
> + if (logsys_wthread_active)
> + pthread_mutex_unlock (&logsys_config_mutex);
> }
>
> static void _log_printf (
> @@ -467,6 +469,14 @@
> pthread_mutex_unlock (&logsys_new_log_mutex);
> }
>
> +static void child_cleanup (void)
> +{
> + memset(&log_thread_group, 0, sizeof(log_thread_group));
> + logsys_wthread_active = 0;
> + pthread_mutex_init(&logsys_config_mutex, NULL);
> + pthread_mutex_init(&logsys_new_log_mutex, NULL);
> +}
> +
> int _logsys_wthread_create (void)
> {
> worker_thread_group_init (
> @@ -481,6 +491,7 @@
> logsys_flush();
>
> atexit (logsys_atexit);
> + pthread_atfork(NULL, NULL, child_cleanup);
>
> if (logsys_mode & LOG_MODE_OUTPUT_SYSLOG_THREADED && logsys_name !=
> NULL) {
> openlog (logsys_name, LOG_CONS|LOG_PID, logsys_facility);
> diff -urNad openais-0.82~/test/Makefile openais-0.82/test/Makefile
> --- openais-0.82~/test/Makefile 2008-04-17 07:27:02.000000000 +0200
> +++ openais-0.82/test/Makefile 2008-04-17 07:29:02.000000000 +0200
> @@ -161,13 +161,13 @@
> $(CC) $(LDFLAGS) -o openais-cfgtool openais-cfgtool.o $(LIBS)
>
> logsys_s: logsys_s.o logsys_s1.o logsys_s2.o ../exec/liblogsys.a
> - $(CC) $(LDFLAGS) -o logsys_s logsys_s.o logsys_s1.o logsys_s2.o
> ../exec/liblogsys.a
> + $(CC) -o logsys_s logsys_s.o logsys_s1.o logsys_s2.o
> ../exec/liblogsys.a $(LDFLAGS)
>
> logsys_t1: logsys_t1.o ../exec/liblogsys.a
> - $(CC) $(LDFLAGS) -o logsys_t1 logsys_t1.o ../exec/liblogsys.a
> + $(CC) -o logsys_t1 logsys_t1.o ../exec/liblogsys.a $(LDFLAGS)
>
> logsys_t2: logsys_t2.o ../exec/liblogsys.a
> - $(CC) $(LDFLAGS) -o logsys_t2 logsys_t2.o ../exec/liblogsys.a
> + $(CC) -o logsys_t2 logsys_t2.o ../exec/liblogsys.a $(LDFLAGS)
>
> clean:
> rm -f *.o $(LIBRARIES) $(BINARIES)
>
>
> --
> I'm going to make him an offer he can't refuse.
> _______________________________________________
> Openais mailing list
> Openais@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/openais
_______________________________________________
Openais mailing list
Openais@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/openais
