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:[email protected]] Sent: Monday, October 06, 2014 7:46 AM To: [email protected] 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:[email protected]] 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 <[email protected]> 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.
httpd_trunk_SO_REUSEPORT_fix.patch
Description: httpd_trunk_SO_REUSEPORT_fix.patch
