httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lu, Yingqi" <yingqi...@intel.com>
Subject RE: svn commit: r1599531 - in /httpd/httpd/trunk: CHANGES include/ap_listen.h server/listen.c server/mpm/event/event.c server/mpm/prefork/prefork.c server/mpm/worker/worker.c server/mpm_unix.c
Date Tue, 07 Oct 2014 01:28:58 GMT
Hi Yann,

Thank you very much for your help. Here is another update on the fix. In this update, I changed:

1. Address the restart/graceful restart issues with ap_daemons_limit changes (malloc/realloc/free
approach). Thank you for your help!

2. I still think we should use _SC_NPROCESSORS_CONF instead of _SC_NPROCESSORS_ONLN to calculate
num_buckets. The reason is: number of duplicated listener is calculated based on num_buckets.
Basically, one dedicated listener per bucket. Therefore, to keep the number of listener a
constant value via the restarts, I think we may want to use   _SC_NPROCESSORS_CONF. 

3. In addition to the restart issue, I guard the server_limit and ap_daemons_limit to be >=
 num_buckets.

I briefly run valgrind with --tool=memcheck on httpd start/stop/restart/graceful restart.
Summary says 0 errors. I am not sure if this is sufficient enough. 

Please let me know if this version works.

Thanks very much,
Yingqi

-----Original Message-----
From: Lu, Yingqi [mailto:yingqi.lu@intel.com] 
Sent: Monday, October 06, 2014 7:46 AM
To: dev@httpd.apache.org
Subject: RE: svn commit: r1599531 - in /httpd/httpd/trunk: CHANGES include/ap_listen.h server/listen.c
server/mpm/event/event.c server/mpm/prefork/prefork.c server/mpm/worker/worker.c server/mpm_unix.c

Hi Yann,

Thanks very much for your feedback.

I will send another update soon to address the restart issues.

Also, inactive CPUs will not be scheduled for httpd. I will change back _SC_NPROCESSORS_CONF
to _SC_NPROCESSORS_ONLN.

Thanks,
Lucy

-----Original Message-----
From: Yann Ylavic [mailto:ylavic.dev@gmail.com] 
Sent: Monday, October 06, 2014 1:12 AM
To: httpd
Subject: Re: svn commit: r1599531 - in /httpd/httpd/trunk: CHANGES include/ap_listen.h server/listen.c
server/mpm/event/event.c server/mpm/prefork/prefork.c server/mpm/worker/worker.c server/mpm_unix.c

Hi Yingqi,

On Sun, Oct 5, 2014 at 11:36 PM, Lu, Yingqi <yingqi.lu@intel.com> wrote:
> To address your first comment, the issue with pconf pool is that bucket array value needs
to be retained via restart and graceful restart. Based on your comments, I now put bucket
array into the retained_data struct for all the mpms. Hope this works.

The problem IMHO is that ap_daemons_limit (used to compute the size of the bucket array) may
not be constant accross restarts (depending on the new configuration).
Maybe you could use a retained bucket array to copy the current values before graceful restart
and restore them after in the pconf allocated array (the one really used by the parent process
and the new generation of children).
To address the memory leak, since the size may change, I think the retained array would have
to be malloc()ed instead, and possibly realloc()ed on restarts (cleared when non graceful)
if there is not enough space to handle the new generation (with a process pool cleanup registered
the first time to free() the whole thing on stop, and make valgrind happy).

Also, since the number of listenners (children) needs to remain constant (IIRC, or connections
may be reset), maybe you'll have to make sure on graceful restart that the previous generation
of children has really stopped listenning before starting new children. Maybe this is always
the case already, but the race condition seems more problematic when SO_REUSEPORT is used.

> Regarding to your second question, based on previous patch code, num_buckets is calculated
based on the active CPU threads count. I am thinking maybe it is better to do the calculation
based on total number of CPU threads instead. This keeps num_buckets to be a constant number
as long as the system is running. That is the reason I now change CPU thread count check from
_SC_NPROCESSORS_ONLN to _SC_NPROCESSORS_CONF.

I must have missed the point here, will inactive CPUs be scheduled for httpd?
Otherwise, I don't see why they should be taken into account for the number of buckets...

Regards,
Yann.
Mime
View raw message