You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ben Hyde <bh...@gensym.com> on 1997/12/15 18:10:00 UTC
[PATCH] Serialize the update to pool.sub_* in destroy_pool
Destroy_pool is called as frequently as per request, (e.g. in child_main and
mod_example). It mutates the subpools list of the parent. Those mutations must
be serialized.
Remove an unnecessary assignment.
> cvs diff -u -b alloc.c
Index: alloc.c
===================================================================
RCS file: /cvs/apachen/src/main/alloc.c,v
retrieving revision 1.60
diff -u -b -r1.60 alloc.c
--- alloc.c 1997/12/07 21:33:18 1.60
+++ alloc.c 1997/12/15 17:50:16
@@ -378,9 +378,6 @@
while (a->sub_pools)
destroy_pool(a->sub_pools);
-
- a->sub_pools = NULL;
-
run_cleanups(a->cleanups);
a->cleanups = NULL;
free_proc_chain(a->subprocesses);
@@ -413,6 +410,8 @@
block_alarms();
clear_pool(a);
+ (void) acquire_mutex(alloc_mutex);
+ {
if (a->parent) {
if (a->parent->sub_pools == a)
a->parent->sub_pools = a->sub_next;
@@ -421,6 +420,8 @@
if (a->sub_next)
a->sub_next->sub_prev = a->sub_prev;
}
+ }
+ (void) release_mutex(alloc_mutex);
free_blocks(a->first);
unblock_alarms();
Re: [PATCH] Serialize the update to pool.sub_* in destroy_pool
Posted by Ben Laurie <be...@algroup.co.uk>.
Dean Gaudet wrote:
>
> On Mon, 15 Dec 1997, Ben Laurie wrote:
>
> > Dean Gaudet wrote:
> > > This breaks down though on timeouts when the longjmp happens to clean
> > > up everything. In that case the three subthreads need to be cleaned
> > > up before their pools can be. Unfortunately, clear_pool() recurses to
> > > the deepest pool first before doing a run_cleanups(), and so it would
> > > try to clean up other_sub_pool before it had run the cleanup that would
> > > kill off the three subthreads.
> > >
> > > Aha. Do other_sub_pool = make_sub_pool(NULL), and then take care of
> > > that with a cleanup registered in pool1.
> >
> > Glurp. And then if you destroy the subpool you need to also destroy the
> > cleanup. Presumably you need to serialise this cleanup, too.
>
> Right, but the sub pool is only destroyed by the cleanup, and that cleanup
> also destroys the thread(s) that were using the sub pool. Serialization
> is implicit because the other threads are gone.
I meant serialise the destruction of the cleanup. Not quite sure about
that, though. Guess it depends on circumstances.
> > Perhaps it would be more elegant to have "top down" cleanups which get
> > executed before the (existing) "bottom up" cleanups? Thread cleanups
> > would then be "top down" and, well, presto.
>
> Yeah I've wondered about this myself at one point too -- but I've been
> able to come up with solutions using the current stuff so far. But I've
> had to assume that the current implementation of alloc.c is the documented
> API. That is, cleanups occur after sub pools are destroyed, and cleanups
> occur in the reverse order they're registered in.
Let's document it and introduce top-down cleanups. Simplicate and add
lightness, I say.
The order of cleanups is important. People shouldn't have to assume
things about them.
Cheers,
Ben.
--
Ben Laurie |Phone: +44 (181) 735 0686|Apache Group member
Freelance Consultant |Fax: +44 (181) 735 0689|http://www.apache.org
and Technical Director|Email: ben@algroup.co.uk |Apache-SSL author
A.L. Digital Ltd, |http://www.algroup.co.uk/Apache-SSL
London, England. |"Apache: TDG" http://www.ora.com/catalog/apache
Re: [PATCH] Serialize the update to pool.sub_* in destroy_pool
Posted by Dean Gaudet <dg...@arctic.org>.
On Mon, 15 Dec 1997, Ben Laurie wrote:
> Dean Gaudet wrote:
> > This breaks down though on timeouts when the longjmp happens to clean
> > up everything. In that case the three subthreads need to be cleaned
> > up before their pools can be. Unfortunately, clear_pool() recurses to
> > the deepest pool first before doing a run_cleanups(), and so it would
> > try to clean up other_sub_pool before it had run the cleanup that would
> > kill off the three subthreads.
> >
> > Aha. Do other_sub_pool = make_sub_pool(NULL), and then take care of
> > that with a cleanup registered in pool1.
>
> Glurp. And then if you destroy the subpool you need to also destroy the
> cleanup. Presumably you need to serialise this cleanup, too.
Right, but the sub pool is only destroyed by the cleanup, and that cleanup
also destroys the thread(s) that were using the sub pool. Serialization
is implicit because the other threads are gone.
> Perhaps it would be more elegant to have "top down" cleanups which get
> executed before the (existing) "bottom up" cleanups? Thread cleanups
> would then be "top down" and, well, presto.
Yeah I've wondered about this myself at one point too -- but I've been
able to come up with solutions using the current stuff so far. But I've
had to assume that the current implementation of alloc.c is the documented
API. That is, cleanups occur after sub pools are destroyed, and cleanups
occur in the reverse order they're registered in.
Dean
Re: [PATCH] Serialize the update to pool.sub_* in destroy_pool
Posted by Ben Laurie <be...@algroup.co.uk>.
Dean Gaudet wrote:
> This breaks down though on timeouts when the longjmp happens to clean
> up everything. In that case the three subthreads need to be cleaned
> up before their pools can be. Unfortunately, clear_pool() recurses to
> the deepest pool first before doing a run_cleanups(), and so it would
> try to clean up other_sub_pool before it had run the cleanup that would
> kill off the three subthreads.
>
> Aha. Do other_sub_pool = make_sub_pool(NULL), and then take care of
> that with a cleanup registered in pool1.
Glurp. And then if you destroy the subpool you need to also destroy the
cleanup. Presumably you need to serialise this cleanup, too.
Perhaps it would be more elegant to have "top down" cleanups which get
executed before the (existing) "bottom up" cleanups? Thread cleanups
would then be "top down" and, well, presto.
Cheers,
Ben.
--
Ben Laurie |Phone: +44 (181) 735 0686|Apache Group member
Freelance Consultant |Fax: +44 (181) 735 0689|http://www.apache.org
and Technical Director|Email: ben@algroup.co.uk |Apache-SSL author
A.L. Digital Ltd, |http://www.algroup.co.uk/Apache-SSL
London, England. |"Apache: TDG" http://www.ora.com/catalog/apache
Re: [PATCH] Serialize the update to pool.sub_* in destroy_pool
Posted by Dean Gaudet <dg...@arctic.org>.
Yuck. All accesses to foo->sub_pools, sub_next, and sub_prev need to
be serialized.
Can we get anywhere by having each thread do a make_sub_pool(NULL) so that
their pools are not sub pools of the shared pools? I think then we just
revisit your earlier complaint about the lack of serialization in pools
period. Or maybe not. My example where mod_cgi uses three "subthreads"
to do work and has three sub_pools didn't clean up the sub pools until
all three threads had completed. So we'd have something like this:
thread private pool
|
r->pool
|
+----------+-----------+
| | |
pool1 pool2 pool3 one for each subthread
|
other_sub_pool
As long as the subthreads don't destroy pool1, 2, or 3, they're safe.
They're still free to make sub pools like other_sub_pool, provided pool1
itself isn't being used by any other thread. They can even destroy
other_sub_pool.
This breaks down though on timeouts when the longjmp happens to clean
up everything. In that case the three subthreads need to be cleaned
up before their pools can be. Unfortunately, clear_pool() recurses to
the deepest pool first before doing a run_cleanups(), and so it would
try to clean up other_sub_pool before it had run the cleanup that would
kill off the three subthreads.
Aha. Do other_sub_pool = make_sub_pool(NULL), and then take care of
that with a cleanup registered in pool1.
Ok I've convinced myself that we don't need to serialize the sub_pools
thing at all... we just need to have a few examples of how to do
multithreaded stuff "right". But have I convinced anyone else?
Dean
On Mon, 15 Dec 1997, Ben Hyde wrote:
> Destroy_pool is called as frequently as per request, (e.g. in child_main and
> mod_example). It mutates the subpools list of the parent. Those mutations must
> be serialized.
>
> Remove an unnecessary assignment.
>
> > cvs diff -u -b alloc.c
> Index: alloc.c
> ===================================================================
> RCS file: /cvs/apachen/src/main/alloc.c,v
> retrieving revision 1.60
> diff -u -b -r1.60 alloc.c
> --- alloc.c 1997/12/07 21:33:18 1.60
> +++ alloc.c 1997/12/15 17:50:16
> @@ -378,9 +378,6 @@
>
> while (a->sub_pools)
> destroy_pool(a->sub_pools);
> -
> - a->sub_pools = NULL;
> -
> run_cleanups(a->cleanups);
> a->cleanups = NULL;
> free_proc_chain(a->subprocesses);
> @@ -413,6 +410,8 @@
> block_alarms();
> clear_pool(a);
>
> + (void) acquire_mutex(alloc_mutex);
> + {
> if (a->parent) {
> if (a->parent->sub_pools == a)
> a->parent->sub_pools = a->sub_next;
> @@ -421,6 +420,8 @@
> if (a->sub_next)
> a->sub_next->sub_prev = a->sub_prev;
> }
> + }
> + (void) release_mutex(alloc_mutex);
>
> free_blocks(a->first);
> unblock_alarms();
>