You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "Paul J. Reder" <re...@raleigh.ibm.com> on 2001/08/03 15:28:17 UTC
[Fwd: [Patch]: Scoreboard as linked list.]
Unfortunately, the first version of the reply went directly to Ryan. Here is a copy...
Ryan Bloom wrote:
>
> -1. As I have stated multiple times, if this uses a mutex to lock the list whenever something
> walks the scoreboard, I can't accept it. It will kill the performance for modules that I have.
I'm very impressed that you were able to get the patch, apply it, build it and test it
in the 30 minutes between my post and yours :)
I do understand your concern. But I feel you are overreacting. When we had this discussion
you indicated this concern, but indicated a willingness to test the code if I wrote it.
It is now written, I would appreciate you actually taking a look at it before panning it
outright.
There are three times when locking can occur:
1. When workers or processes come and go.
2. When p_i_s_m checks for idleness.
3. When the scoreboard is walked for status.
1 occurs rarely (even in my pathological test scenario of MRPC = 3000 under extreme abuse).
In fact, 1 can be eliminated almost entirely if you set MRPC to 0. The exception being the
replacement of segfaulted processes. Due to its infrequency and the short duration of the
locking time this should not cause any problem, and in my testing did not seem to since my
solution performed at least as well as yours.
2 occurs once per second. This is a quick walk to check the status flag and tally some
counts. This could be optimized in several ways *if* it proves to be a problem.
A) It could avoid locking if MRPC = 0.
B) It could use a read lock to permit simultaneous status walks.
C) It could use finer granularity locking to allow multiple walkers withing the SB.
This code is already optimized, compared to the current code, since it only walks through
the workers in the active list. Workers on the free list aren't walked, therefore do not
incurr that overhead.
3 may occur frequently (in your case), or infrequently (in everyone elses ;) ). Locking
occurs in two parts. This can be optimized in a number of ways *if* it proves to be a
problem.
A-C) As above.
D) The walking code itself could be optimized to do less work during the locked phase.
For example, the mod_status code currently does all of its network writes in the
loop while holding a lock. The code could be rewritten to memcpy the parts into
a local buffer, then write the buffer to the network outside the locking region.
Obviously we have a difference of opinion. The only way to resolve this is to test it.
I have done my part in the testing process. Vetoing it without testing it is bad form.
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein
Re: [Fwd: [Patch]: Scoreboard as linked list.]
Posted by Ryan Bloom <rb...@covalent.net>.
On Monday 06 August 2001 08:07, Greg Ames wrote:
> Ryan Bloom wrote:
> > 2) I may be wrong, but I believe we have a race-condition.
> > find_child_by_pid walks the scoreboard without locking it. Since we call
> > that function from the parent whenever a child process dies, this looks
> > like a chance for a seg fault, but I am still trying to figure out what I
> > am missing. Rather than continue to look for my misunderstanding, can
> > somebody explain it to me?
>
> [disclosure: I haven't reviewed the patch, but think I understand the
> concept]
>
> Is this really a bug? I would think the list updates always happen in
> the single threaded parent process, and not in anything asychronous
> (like a signal handler). If that's true, why would any other code
> called synchronously from the parent need to hold the lock?
Yes, this is a bug. As I have tried to explain, anytime you want to walk the entire linked list,
you MUST lock it. You have no idea what the child processes are doing.
Based on my reading of the patch, we update the worker_score->next pointer in
ap_obtain_free_process, which is called from child_main. Therefore, updates happen
throughout the code, and all walks require a lock.
Ryan
_____________________________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
-----------------------------------------------------------------------------
Re: [Fwd: [Patch]: Scoreboard as linked list.]
Posted by Greg Ames <gr...@remulak.net>.
Ryan Bloom wrote:
> 2) I may be wrong, but I believe we have a race-condition. find_child_by_pid walks the
> scoreboard without locking it. Since we call that function from the parent whenever a child
> process dies, this looks like a chance for a seg fault, but I am still trying to figure out what
> I am missing. Rather than continue to look for my misunderstanding, can somebody explain it
> to me?
[disclosure: I haven't reviewed the patch, but think I understand the
concept]
Is this really a bug? I would think the list updates always happen in
the single threaded parent process, and not in anything asychronous
(like a signal handler). If that's true, why would any other code
called synchronously from the parent need to hold the lock?
Greg
Re: [Fwd: [Patch]: Scoreboard as linked list.]
Posted by "Paul J. Reder" <re...@raleigh.ibm.com>.
Ryan Bloom wrote:
> 2) I may be wrong, but I believe we have a race-condition. find_child_by_pid walks the
> scoreboard without locking it. Since we call that function from the parent whenever a child
> process dies, this looks like a chance for a seg fault, but I am still trying to figure out what I
> am missing. Rather than continue to look for my misunderstanding, can somebody explain it
> to me?
You are correct. If a process dies and ends up in the find_child_by_pid code at the same time
that another neighboring process is exiting cleanly, there could be a seg-fault. I'll fix
the patch appropriately.
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein
Re: [Fwd: [Patch]: Scoreboard as linked list.]
Posted by Ryan Bloom <rb...@covalent.net>.
> I have other concerns that I still haven't finished thinking through, so I
> haven't posted them yet, but I will continue to analyse the patch and post
> comments as I finish thinking them through.
As promised, the other concerns:
1) We are allocating and destroying shared memory segments. If I have a module that
creates an other_child, not an Apache child process, that module has access to the
scoreboard in the other_child process. If we start to create and delete segments of shared
memory that the module has access to, then we are asking for a seg-fault. If we are willing
to require modules to restart the other_child process whenever Apache restarts, then we can
avoid this problem, but this is a VERY large penalty.
2) I may be wrong, but I believe we have a race-condition. find_child_by_pid walks the
scoreboard without locking it. Since we call that function from the parent whenever a child
process dies, this looks like a chance for a seg fault, but I am still trying to figure out what I
am missing. Rather than continue to look for my misunderstanding, can somebody explain it
to me?
That's it for now. But I'm not done reviewing the code yet. :-)
Ryan
_____________________________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
-----------------------------------------------------------------------------
Re: [Fwd: [Patch]: Scoreboard as linked list.]
Posted by Ryan Bloom <rb...@covalent.net>.
On Friday 03 August 2001 06:28, Paul J. Reder wrote:
> Unfortunately, the first version of the reply went directly to Ryan. Here
> is a copy...
>
> Ryan Bloom wrote:
> > -1. As I have stated multiple times, if this uses a mutex to lock the
> > list whenever something walks the scoreboard, I can't accept it. It will
> > kill the performance for modules that I have.
>
> I'm very impressed that you were able to get the patch, apply it, build it
> and test it in the 30 minutes between my post and yours :)
That's easy to explain. I didn't apply it. I read it. I happen to be VERY well-
versed in how Apache 2.0 works. I tend to be able to read patches, and
determine most of the effects of the code. If my initial reading doesn't show me
any problems, then I apply and test it. It took me about 30 minutes to read through
the code. It helps that I was trying to add a feature to the configure script, and I
couldn't figure out how to do it, so I was checking e-mail every minute or so.
> I do understand your concern. But I feel you are overreacting. When we had
> this discussion you indicated this concern, but indicated a willingness to
> test the code if I wrote it. It is now written, I would appreciate you
> actually taking a look at it before panning it outright.
No. I specifically stated that if we had to lock the mutex every time we had to
walk the scoreboard, I would have to veto the patch. Since this patch very
obviously has that requirement, I have lived up to my word.
> There are three times when locking can occur:
> 1. When workers or processes come and go.
> 2. When p_i_s_m checks for idleness.
> 3. When the scoreboard is walked for status.
>
> 1 occurs rarely (even in my pathological test scenario of MRPC = 3000 under
> extreme abuse). In fact, 1 can be eliminated almost entirely if you set
> MRPC to 0. The exception being the replacement of segfaulted processes. Due
> to its infrequency and the short duration of the locking time this should
> not cause any problem, and in my testing did not seem to since my solution
> performed at least as well as yours.
>
> 2 occurs once per second. This is a quick walk to check the status flag and
> tally some counts. This could be optimized in several ways *if* it proves
> to be a problem. A) It could avoid locking if MRPC = 0.
> B) It could use a read lock to permit simultaneous status walks.
> C) It could use finer granularity locking to allow multiple walkers
> withing the SB. This code is already optimized, compared to the current
> code, since it only walks through the workers in the active list. Workers
> on the free list aren't walked, therefore do not incurr that overhead.
>
> 3 may occur frequently (in your case), or infrequently (in everyone elses
> ;) ). Locking occurs in two parts. This can be optimized in a number of
> ways *if* it proves to be a problem.
> A-C) As above.
> D) The walking code itself could be optimized to do less work during
> the locked phase. For example, the mod_status code currently does all of
> its network writes in the loop while holding a lock. The code could be
> rewritten to memcpy the parts into a local buffer, then write the buffer to
> the network outside the locking region.
> Obviously we have a difference of opinion. The only way to resolve this is
> to test it. I have done my part in the testing process. Vetoing it without
> testing it is bad form.
Nonsense, people veto without testing all the time. It can suck, and I have
complained about it in the past as well, but people do it all the time.
I have other concerns that I still haven't finished thinking through, so I haven't
posted them yet, but I will continue to analyse the patch and post comments as
I finish thinking them through.
Ryan
_____________________________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
-----------------------------------------------------------------------------