There is a follow up in r1629990 (fix min_spare_threads lower bound and check wrt num_buckets). In fact there are 2 with a minor fix in r1629916, but I guess this latter can be ignored for testing.
Regards, Yann. On Tue, Oct 7, 2014 at 5:28 PM, Lu, Yingqi <[email protected]> wrote: > Thanks very much for your quick help! > > I will test it today and let you know. > > Thanks, > Yingqi > > -----Original Message----- > From: Yann Ylavic [mailto:[email protected]] > Sent: Tuesday, October 07, 2014 8:21 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, > > thanks for your help. > > I finally commited a version (http://svn.apache.org/r1629909) where each > child's bucket number is stored in the process scoreboard, so there is no > need for each mpm to handle its own array. > > Can you please check that it works for you? > > Regards, > Yann. > > On Tue, Oct 7, 2014 at 11:01 AM, Lu, Yingqi <[email protected]> wrote: >> In the last version, I forgot to change _SC_NPROCESSORS_ONLN to >> _SC_NPROCESSORS_CONF for worker mpm. >> >> Please use this version to review. Sorry for the duplication. >> >> Thanks very much for your help! >> Yingqi >> >> -----Original Message----- >> From: Lu, Yingqi [mailto:[email protected]] >> Sent: Monday, October 06, 2014 6:29 PM >> 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, >> >> 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.
