* patch rfc: PR28661: debuginfod thread-pool @ 2021-12-09 3:55 Frank Ch. Eigler 2021-12-09 16:47 ` Mark Wielaard 0 siblings, 1 reply; 5+ messages in thread From: Frank Ch. Eigler @ 2021-12-09 3:55 UTC (permalink / raw) To: elfutils-devel Hi - While I think this patch itself is fine, and works around the libmicrohttpd bug that motivated it, I don't know how to test it seriously in the testsuite. (We can certainly try few -C options for parsing & operability.) The error edge cases only appear to occur under very high load and task limits such as those imposed by systemd cgroups. Author: Frank Ch. Eigler <fche@redhat.com> Date: Wed Dec 8 22:41:47 2021 -0500 PR28661: debuginfo connection thread pool support Add an option -C, which activates libmicrohttpd's thread-pool mode for handling incoming http connections. Add libmicrohttpd error-logging callback function so as to receive indication of its internal errors, and relay counts to our metrics. Some of these internal errors tipped us off to a microhttpd bug that thread pooling works around. Document in debuginfod.8 page. Hand-tested against "ulimit -u NNN" shells. Signed-off-by: Frank Ch. Eigler <fche@redhat.com> diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog index 21d0721ef080..125e7d816f51 100644 --- a/debuginfod/ChangeLog +++ b/debuginfod/ChangeLog @@ -1,3 +1,11 @@ +2021-12-08 Frank Ch. Eigler <fche@redhat.com> + + * debuginfod.cxx (connection_pool): New global. + (parse_opt): Parse & check -C option to set it. + (error_cb): New callback for libmicrohttpd error counting. + (main): Activate MHD_OPTION_THREAD_POOL_SIZE if appropriate. + Activate error_cb. + 2021-12-01 Mark Wielaard <mark@klomp.org> * debuginfod-client.c (debuginfod_query_server): Free tmp_url on diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx index 0d3f02978ee2..bb0c6cd65670 100644 --- a/debuginfod/debuginfod.cxx +++ b/debuginfod/debuginfod.cxx @@ -352,7 +352,9 @@ static const struct argp_option options[] = { "rescan-time", 't', "SECONDS", 0, "Number of seconds to wait between rescans, 0=disable.", 0 }, { "groom-time", 'g', "SECONDS", 0, "Number of seconds to wait between database grooming, 0=disable.", 0 }, { "maxigroom", 'G', NULL, 0, "Run a complete database groom/shrink pass at startup.", 0 }, - { "concurrency", 'c', "NUM", 0, "Limit scanning thread concurrency to NUM.", 0 }, + { "concurrency", 'c', "NUM", 0, "Limit scanning thread concurrency to NUM, default=#CPUs.", 0 }, + { "connection-pool", 'C', "NUM", OPTION_ARG_OPTIONAL, + "Use webapi connection pool with NUM threads, default=unlim.", 0 }, { "include", 'I', "REGEX", 0, "Include files matching REGEX, default=all.", 0 }, { "exclude", 'X', "REGEX", 0, "Exclude files matching REGEX, default=none.", 0 }, { "port", 'p', "NUM", 0, "HTTP port to listen on, default 8002.", 0 }, @@ -411,6 +413,7 @@ static unsigned rescan_s = 300; static unsigned groom_s = 86400; static bool maxigroom = false; static unsigned concurrency = std::thread::hardware_concurrency() ?: 1; +static int connection_pool = 0; static set<string> source_paths; static bool scan_files = false; static map<string,string> scan_archives; @@ -557,6 +560,16 @@ parse_opt (int key, char *arg, concurrency = (unsigned) atoi(arg); if (concurrency < 1) concurrency = 1; break; + case 'C': + if (arg) + { + connection_pool = atoi(arg); + if (connection_pool < 2) + argp_failure(state, 1, EINVAL, "-C NUM minimum 2"); + } + else // arg not given + connection_pool = std::thread::hardware_concurrency() * 2 ?: 2; + break; case 'I': // NB: no problem with unconditional free here - an earlier failed regcomp would exit program if (passive_p) @@ -3684,7 +3698,13 @@ sigusr2_handler (int /* sig */) } - +static void // error logging callback from libmicrohttpd internals +error_cb (void *arg, const char *fmt, va_list ap) +{ + (void) arg; + inc_metric("error_count","libmicrohttpd",fmt); + (void) vdprintf (STDERR_FILENO, fmt, ap); +} // A user-defined sqlite function, to score the sharedness of the @@ -3829,7 +3849,7 @@ main (int argc, char *argv[]) // Start httpd server threads. Separate pool for IPv4 and IPv6, in // case the host only has one protocol stack. - MHD_Daemon *d4 = MHD_start_daemon (MHD_USE_THREAD_PER_CONNECTION + MHD_Daemon *d4 = MHD_start_daemon ((connection_pool ? 0 : MHD_USE_THREAD_PER_CONNECTION) #if MHD_VERSION >= 0x00095300 | MHD_USE_INTERNAL_POLLING_THREAD #else @@ -3839,8 +3859,11 @@ main (int argc, char *argv[]) http_port, NULL, NULL, /* default accept policy */ handler_cb, NULL, /* handler callback */ + MHD_OPTION_EXTERNAL_LOGGER, error_cb, NULL, + (connection_pool ? MHD_OPTION_THREAD_POOL_SIZE : MHD_OPTION_END), + (connection_pool ? (int)connection_pool : MHD_OPTION_END), MHD_OPTION_END); - MHD_Daemon *d6 = MHD_start_daemon (MHD_USE_THREAD_PER_CONNECTION + MHD_Daemon *d6 = MHD_start_daemon ((connection_pool ? 0 : MHD_USE_THREAD_PER_CONNECTION) #if MHD_VERSION >= 0x00095300 | MHD_USE_INTERNAL_POLLING_THREAD #else @@ -3851,6 +3874,9 @@ main (int argc, char *argv[]) http_port, NULL, NULL, /* default accept policy */ handler_cb, NULL, /* handler callback */ + MHD_OPTION_EXTERNAL_LOGGER, error_cb, NULL, + (connection_pool ? MHD_OPTION_THREAD_POOL_SIZE : MHD_OPTION_END), + (connection_pool ? (int)connection_pool : MHD_OPTION_END), MHD_OPTION_END); if (d4 == NULL && d6 == NULL) // neither ipv4 nor ipv6? boo @@ -3904,6 +3930,8 @@ main (int argc, char *argv[]) if (! passive_p) obatched(clog) << "search concurrency " << concurrency << endl; + obatched(clog) << "webapi connection pool " << connection_pool + << (connection_pool ? "" : " (unlimited)") << endl; if (! passive_p) obatched(clog) << "rescan time " << rescan_s << endl; obatched(clog) << "fdcache fds " << fdcache_fds << endl; diff --git a/doc/ChangeLog b/doc/ChangeLog index 7a73fa107bf3..f25bcd2ee2b3 100644 --- a/doc/ChangeLog +++ b/doc/ChangeLog @@ -1,3 +1,8 @@ +2021-12-08 Frank Ch. Eigler <fche@redhat.com> + + PR28661 + * debuginfod.8 (-C): Document new flag. + 2021-11-05 Frank Ch. Eigler <fche@redhat.com> PR28430 diff --git a/doc/debuginfod.8 b/doc/debuginfod.8 index 1e56f6568322..ee8e4078e5b5 100644 --- a/doc/debuginfod.8 +++ b/doc/debuginfod.8 @@ -205,6 +205,23 @@ This important for controlling CPU-intensive operations like parsing an ELF file and especially decompressing archives. The default is the number of processors on the system; the minimum is 1. +.TP +.B "\-C" "\-C=NUM" "\-\-connection\-pool" "\-\-connection\-pool=NUM" +Set the size of the pool of threads serving webapi queries. The +following table summarizes the interpretaton of this option and its +optional NUM parameter. +.TS +l l. +no option clone new thread for every request, no fixed pool +\-C use a fixed thread pool sized automatically +\-C=NUM use a fixed thread pool sized NUM, minimum 2 +.TE + +The first mode is useful for friendly bursty traffic. The second mode +is a simple and safe configuration based on the number of processors. +The third mode is suitable for tuned load-limiting configurations +facing unruly traffic. + .TP .B "\-L" Traverse symbolic links encountered during traversal of the PATHs, ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: patch rfc: PR28661: debuginfod thread-pool 2021-12-09 3:55 patch rfc: PR28661: debuginfod thread-pool Frank Ch. Eigler @ 2021-12-09 16:47 ` Mark Wielaard 2021-12-09 16:54 ` Frank Ch. Eigler 2021-12-10 0:10 ` Frank Ch. Eigler 0 siblings, 2 replies; 5+ messages in thread From: Mark Wielaard @ 2021-12-09 16:47 UTC (permalink / raw) To: Frank Ch. Eigler, elfutils-devel Hi Frank, On Wed, 2021-12-08 at 22:55 -0500, Frank Ch. Eigler via Elfutils-devel wrote: > While I think this patch itself is fine, and works around the > libmicrohttpd bug that motivated it, I don't know how to test it > seriously in the testsuite. (We can certainly try few -C options for > parsing & operability.) The error edge cases only appear to occur > under very high load and task limits such as those imposed by systemd > cgroups. We have a couple of tests that try to seek the limit that they can still safely run under (e.g. run-large-elf-file.sh), but they were probably mistakes to include in the normal regression test suite. It is tricky to find the appropriate limit on a machine without hitting it and causing the test machine to fall over (briefly). If you can use ulimit -u or ulimit -T in the run-test.sh script then please use that, but that probably requires launching sub-shells and you quickly end up in shell-hell. So I would recommend to simply add a testcase that just uses no-option, -C and -C 256 (or take any arbitrary number, 1 might be an interesting corner case) and see that if you have 4 (8, 16, ...?) debuginfod-find (or curl metric) processes doing some parallel queries works as expected. > Author: Frank Ch. Eigler <fche@redhat.com> > Date: Wed Dec 8 22:41:47 2021 -0500 > > PR28661: debuginfo connection thread pool support > > Add an option -C, which activates libmicrohttpd's thread-pool mode for > handling incoming http connections. Add libmicrohttpd error-logging > callback function so as to receive indication of its internal errors, > and relay counts to our metrics. Some of these internal errors tipped > us off to a microhttpd bug that thread pooling works around. Document > in debuginfod.8 page. Hand-tested against "ulimit -u NNN" shells. > > Signed-off-by: Frank Ch. Eigler <fche@redhat.com> > > diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog > index 21d0721ef080..125e7d816f51 100644 > --- a/debuginfod/ChangeLog > +++ b/debuginfod/ChangeLog > @@ -1,3 +1,11 @@ > +2021-12-08 Frank Ch. Eigler <fche@redhat.com> > + > + * debuginfod.cxx (connection_pool): New global. > + (parse_opt): Parse & check -C option to set it. > + (error_cb): New callback for libmicrohttpd error counting. > + (main): Activate MHD_OPTION_THREAD_POOL_SIZE if appropriate. > + Activate error_cb. > + > 2021-12-01 Mark Wielaard <mark@klomp.org> > > * debuginfod-client.c (debuginfod_query_server): Free tmp_url on > diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx > index 0d3f02978ee2..bb0c6cd65670 100644 > --- a/debuginfod/debuginfod.cxx > +++ b/debuginfod/debuginfod.cxx > @@ -352,7 +352,9 @@ static const struct argp_option options[] = > { "rescan-time", 't', "SECONDS", 0, "Number of seconds to wait between rescans, 0=disable.", 0 }, > { "groom-time", 'g', "SECONDS", 0, "Number of seconds to wait between database grooming, 0=disable.", 0 }, > { "maxigroom", 'G', NULL, 0, "Run a complete database groom/shrink pass at startup.", 0 }, > - { "concurrency", 'c', "NUM", 0, "Limit scanning thread concurrency to NUM.", 0 }, > + { "concurrency", 'c', "NUM", 0, "Limit scanning thread concurrency to NUM, default=#CPUs.", 0 }, > + { "connection-pool", 'C', "NUM", OPTION_ARG_OPTIONAL, > + "Use webapi connection pool with NUM threads, default=unlim.", 0 }, > { "include", 'I', "REGEX", 0, "Include files matching REGEX, default=all.", 0 }, > { "exclude", 'X', "REGEX", 0, "Exclude files matching REGEX, default=none.", 0 }, > { "port", 'p', "NUM", 0, "HTTP port to listen on, default 8002.", 0 }, > @@ -411,6 +413,7 @@ static unsigned rescan_s = 300; > static unsigned groom_s = 86400; > static bool maxigroom = false; > static unsigned concurrency = std::thread::hardware_concurrency() ?: 1; > +static int connection_pool = 0; > static set<string> source_paths; > static bool scan_files = false; > static map<string,string> scan_archives; > @@ -557,6 +560,16 @@ parse_opt (int key, char *arg, > concurrency = (unsigned) atoi(arg); > if (concurrency < 1) concurrency = 1; > break; > + case 'C': > + if (arg) > + { > + connection_pool = atoi(arg); > + if (connection_pool < 2) > + argp_failure(state, 1, EINVAL, "-C NUM minimum 2"); > + } > + else // arg not given > + connection_pool = std::thread::hardware_concurrency() * 2 ?: 2; > + break; Aha, forget about my -C 1 corner case above then :) > case 'I': > // NB: no problem with unconditional free here - an earlier failed regcomp would exit program > if (passive_p) > @@ -3684,7 +3698,13 @@ sigusr2_handler (int /* sig */) > } > > > - > +static void // error logging callback from libmicrohttpd internals > +error_cb (void *arg, const char *fmt, va_list ap) > +{ > + (void) arg; > + inc_metric("error_count","libmicrohttpd",fmt); > + (void) vdprintf (STDERR_FILENO, fmt, ap); > +} > > > // A user-defined sqlite function, to score the sharedness of the > @@ -3829,7 +3849,7 @@ main (int argc, char *argv[]) > > // Start httpd server threads. Separate pool for IPv4 and IPv6, in > // case the host only has one protocol stack. > - MHD_Daemon *d4 = MHD_start_daemon (MHD_USE_THREAD_PER_CONNECTION > + MHD_Daemon *d4 = MHD_start_daemon ((connection_pool ? 0 : MHD_USE_THREAD_PER_CONNECTION) > #if MHD_VERSION >= 0x00095300 > | MHD_USE_INTERNAL_POLLING_THREAD > #else > @@ -3839,8 +3859,11 @@ main (int argc, char *argv[]) > http_port, > NULL, NULL, /* default accept policy */ > handler_cb, NULL, /* handler callback */ > + MHD_OPTION_EXTERNAL_LOGGER, error_cb, NULL, > + (connection_pool ? MHD_OPTION_THREAD_POOL_SIZE : MHD_OPTION_END), > + (connection_pool ? (int)connection_pool : MHD_OPTION_END), > MHD_OPTION_END); > - MHD_Daemon *d6 = MHD_start_daemon (MHD_USE_THREAD_PER_CONNECTION > + MHD_Daemon *d6 = MHD_start_daemon ((connection_pool ? 0 : MHD_USE_THREAD_PER_CONNECTION) > #if MHD_VERSION >= 0x00095300 > | MHD_USE_INTERNAL_POLLING_THREAD > #else > @@ -3851,6 +3874,9 @@ main (int argc, char *argv[]) > http_port, > NULL, NULL, /* default accept policy */ > handler_cb, NULL, /* handler callback */ > + MHD_OPTION_EXTERNAL_LOGGER, error_cb, NULL, > + (connection_pool ? MHD_OPTION_THREAD_POOL_SIZE : MHD_OPTION_END), > + (connection_pool ? (int)connection_pool : MHD_OPTION_END), > MHD_OPTION_END); Urgh, the use of varargs in MHD_start_daemon makes my head hurt. But it does look correct. > if (d4 == NULL && d6 == NULL) // neither ipv4 nor ipv6? boo > @@ -3904,6 +3930,8 @@ main (int argc, char *argv[]) > > if (! passive_p) > obatched(clog) << "search concurrency " << concurrency << endl; > + obatched(clog) << "webapi connection pool " << connection_pool > + << (connection_pool ? "" : " (unlimited)") << endl; > if (! passive_p) > obatched(clog) << "rescan time " << rescan_s << endl; > obatched(clog) << "fdcache fds " << fdcache_fds << endl; > diff --git a/doc/ChangeLog b/doc/ChangeLog > index 7a73fa107bf3..f25bcd2ee2b3 100644 > --- a/doc/ChangeLog > +++ b/doc/ChangeLog > @@ -1,3 +1,8 @@ > +2021-12-08 Frank Ch. Eigler <fche@redhat.com> > + > + PR28661 > + * debuginfod.8 (-C): Document new flag. > + > 2021-11-05 Frank Ch. Eigler <fche@redhat.com> > > PR28430 > diff --git a/doc/debuginfod.8 b/doc/debuginfod.8 > index 1e56f6568322..ee8e4078e5b5 100644 > --- a/doc/debuginfod.8 > +++ b/doc/debuginfod.8 > @@ -205,6 +205,23 @@ This important for controlling CPU-intensive operations like parsing > an ELF file and especially decompressing archives. The default is the > number of processors on the system; the minimum is 1. > > +.TP > +.B "\-C" "\-C=NUM" "\-\-connection\-pool" "\-\-connection\-pool=NUM" > +Set the size of the pool of threads serving webapi queries. The > +following table summarizes the interpretaton of this option and its > +optional NUM parameter. > +.TS > +l l. > +no option clone new thread for every request, no fixed pool > +\-C use a fixed thread pool sized automatically > +\-C=NUM use a fixed thread pool sized NUM, minimum 2 > +.TE > + > +The first mode is useful for friendly bursty traffic. The second mode > +is a simple and safe configuration based on the number of processors. > +The third mode is suitable for tuned load-limiting configurations > +facing unruly traffic. Thanks, the documentation looks pretty clear. Cheers, Mark ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: patch rfc: PR28661: debuginfod thread-pool 2021-12-09 16:47 ` Mark Wielaard @ 2021-12-09 16:54 ` Frank Ch. Eigler 2021-12-10 0:10 ` Frank Ch. Eigler 1 sibling, 0 replies; 5+ messages in thread From: Frank Ch. Eigler @ 2021-12-09 16:54 UTC (permalink / raw) To: Mark Wielaard; +Cc: elfutils-devel Hi - > [...] > If you can use ulimit -u or ulimit -T in the run-test.sh script then > please use that, but that probably requires launching sub-shells and > you quickly end up in shell-hell. A problem I found with that is that ulimit -u appears to be systemwide in the sense that a new process/thread will be prevented if the total systemwide processes of that user exceed this number. It's not just for accounting that particular processes' own children. (ulimit -T doesn't work on my f35 machine with bash or zsh or csh.) > So I would recommend to simply add a testcase that just uses no-option, > -C and -C 256 (or take any arbitrary number, 1 might be an interesting > corner case) and see that if you have 4 (8, 16, ...?) debuginfod-find > (or curl metric) processes doing some parallel queries works as > expected. Yeah, I can/will do that. Well, for any of these -C settings, even thousands of concurrently issued curl jobs should succeed, just not quite as quickly. - FChE ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: patch rfc: PR28661: debuginfod thread-pool 2021-12-09 16:47 ` Mark Wielaard 2021-12-09 16:54 ` Frank Ch. Eigler @ 2021-12-10 0:10 ` Frank Ch. Eigler 2021-12-10 17:44 ` Mark Wielaard 1 sibling, 1 reply; 5+ messages in thread From: Frank Ch. Eigler @ 2021-12-10 0:10 UTC (permalink / raw) To: Mark Wielaard; +Cc: elfutils-devel Hi, Mark - > So I would recommend to simply add a testcase that just uses no-option, > -C and -C 256 (or take any arbitrary number, 1 might be an interesting > corner case) and see that if you have 4 (8, 16, ...?) debuginfod-find > (or curl metric) processes doing some parallel queries works as > expected. OK, here's the current version, with a test, and with one more small improvement to the error message printer (to add timestamp & batching). Author: Frank Ch. Eigler <fche@redhat.com> Date: Wed Dec 8 22:41:47 2021 -0500 PR28661: debuginfo connection thread pool support Add an option -C, which activates libmicrohttpd's thread-pool mode for handling incoming http connections. Add libmicrohttpd error-logging callback function so as to receive indication of its internal errors, and relay counts to our metrics. Some of these internal errors tipped us off to a microhttpd bug that thread pooling works around. Document in debuginfod.8 page. Hand-tested against "ulimit -u NNN" shells, and with a less strenuous new test case. Signed-off-by: Frank Ch. Eigler <fche@redhat.com> diff --git a/NEWS b/NEWS index 490932ae4ef9..6be58866f100 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,8 @@ +Version 0.187 after 0.186 + +debuginfod: Support -C option for connection thread pooling. + + Version 0.186 debuginfod-client: Default $DEBUGINFOD_URLS is computed from drop-in files diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog index df373201d5c6..2642ef5eeaf1 100644 --- a/debuginfod/ChangeLog +++ b/debuginfod/ChangeLog @@ -3,6 +3,14 @@ * debuginfod.cxx (database_stats_report): Don't format clog using 'right' and 'setw(20)'. +2021-12-08 Frank Ch. Eigler <fche@redhat.com> + + * debuginfod.cxx (connection_pool): New global. + (parse_opt): Parse & check -C option to set it. + (error_cb): New callback for libmicrohttpd error counting. + (main): Activate MHD_OPTION_THREAD_POOL_SIZE if appropriate. + Activate error_cb. + 2021-12-04 Mark Wielaard <mark@klomp.org> * debuginfod.cxx (main): Call debuginfod_pool_groom before exit. diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx index 887e4f5abd44..bb8e8e102896 100644 --- a/debuginfod/debuginfod.cxx +++ b/debuginfod/debuginfod.cxx @@ -353,7 +353,9 @@ static const struct argp_option options[] = { "rescan-time", 't', "SECONDS", 0, "Number of seconds to wait between rescans, 0=disable.", 0 }, { "groom-time", 'g', "SECONDS", 0, "Number of seconds to wait between database grooming, 0=disable.", 0 }, { "maxigroom", 'G', NULL, 0, "Run a complete database groom/shrink pass at startup.", 0 }, - { "concurrency", 'c', "NUM", 0, "Limit scanning thread concurrency to NUM.", 0 }, + { "concurrency", 'c', "NUM", 0, "Limit scanning thread concurrency to NUM, default=#CPUs.", 0 }, + { "connection-pool", 'C', "NUM", OPTION_ARG_OPTIONAL, + "Use webapi connection pool with NUM threads, default=unlim.", 0 }, { "include", 'I', "REGEX", 0, "Include files matching REGEX, default=all.", 0 }, { "exclude", 'X', "REGEX", 0, "Exclude files matching REGEX, default=none.", 0 }, { "port", 'p', "NUM", 0, "HTTP port to listen on, default 8002.", 0 }, @@ -412,6 +414,7 @@ static unsigned rescan_s = 300; static unsigned groom_s = 86400; static bool maxigroom = false; static unsigned concurrency = std::thread::hardware_concurrency() ?: 1; +static int connection_pool = 0; static set<string> source_paths; static bool scan_files = false; static map<string,string> scan_archives; @@ -558,6 +561,16 @@ parse_opt (int key, char *arg, concurrency = (unsigned) atoi(arg); if (concurrency < 1) concurrency = 1; break; + case 'C': + if (arg) + { + connection_pool = atoi(arg); + if (connection_pool < 2) + argp_failure(state, 1, EINVAL, "-C NUM minimum 2"); + } + else // arg not given + connection_pool = std::thread::hardware_concurrency() * 2 ?: 2; + break; case 'I': // NB: no problem with unconditional free here - an earlier failed regcomp would exit program if (passive_p) @@ -1368,6 +1381,7 @@ class libarchive_fdcache if (verbose > 3) obatched(clog) << "fdcache interned a=" << a << " b=" << b << " fd=" << fd << " mb=" << mb << " front=" << front_p << endl; + set_metrics(); } @@ -3708,7 +3722,15 @@ sigusr2_handler (int /* sig */) } - +static void // error logging callback from libmicrohttpd internals +error_cb (void *arg, const char *fmt, va_list ap) +{ + (void) arg; + inc_metric("error_count","libmicrohttpd",fmt); + char errmsg[512]; + (void) vsnprintf (errmsg, sizeof(errmsg), fmt, ap); // ok if slightly truncated + obatched(cerr) << "libmicrohttpd error: " << errmsg; // MHD_DLOG calls already include \n +} // A user-defined sqlite function, to score the sharedness of the @@ -3853,7 +3875,7 @@ main (int argc, char *argv[]) // Start httpd server threads. Separate pool for IPv4 and IPv6, in // case the host only has one protocol stack. - MHD_Daemon *d4 = MHD_start_daemon (MHD_USE_THREAD_PER_CONNECTION + MHD_Daemon *d4 = MHD_start_daemon ((connection_pool ? 0 : MHD_USE_THREAD_PER_CONNECTION) #if MHD_VERSION >= 0x00095300 | MHD_USE_INTERNAL_POLLING_THREAD #else @@ -3863,8 +3885,11 @@ main (int argc, char *argv[]) http_port, NULL, NULL, /* default accept policy */ handler_cb, NULL, /* handler callback */ + MHD_OPTION_EXTERNAL_LOGGER, error_cb, NULL, + (connection_pool ? MHD_OPTION_THREAD_POOL_SIZE : MHD_OPTION_END), + (connection_pool ? (int)connection_pool : MHD_OPTION_END), MHD_OPTION_END); - MHD_Daemon *d6 = MHD_start_daemon (MHD_USE_THREAD_PER_CONNECTION + MHD_Daemon *d6 = MHD_start_daemon ((connection_pool ? 0 : MHD_USE_THREAD_PER_CONNECTION) #if MHD_VERSION >= 0x00095300 | MHD_USE_INTERNAL_POLLING_THREAD #else @@ -3875,6 +3900,9 @@ main (int argc, char *argv[]) http_port, NULL, NULL, /* default accept policy */ handler_cb, NULL, /* handler callback */ + MHD_OPTION_EXTERNAL_LOGGER, error_cb, NULL, + (connection_pool ? MHD_OPTION_THREAD_POOL_SIZE : MHD_OPTION_END), + (connection_pool ? (int)connection_pool : MHD_OPTION_END), MHD_OPTION_END); if (d4 == NULL && d6 == NULL) // neither ipv4 nor ipv6? boo @@ -3928,6 +3956,8 @@ main (int argc, char *argv[]) if (! passive_p) obatched(clog) << "search concurrency " << concurrency << endl; + obatched(clog) << "webapi connection pool " << connection_pool + << (connection_pool ? "" : " (unlimited)") << endl; if (! passive_p) obatched(clog) << "rescan time " << rescan_s << endl; obatched(clog) << "fdcache fds " << fdcache_fds << endl; diff --git a/doc/ChangeLog b/doc/ChangeLog index 7a73fa107bf3..f25bcd2ee2b3 100644 --- a/doc/ChangeLog +++ b/doc/ChangeLog @@ -1,3 +1,8 @@ +2021-12-08 Frank Ch. Eigler <fche@redhat.com> + + PR28661 + * debuginfod.8 (-C): Document new flag. + 2021-11-05 Frank Ch. Eigler <fche@redhat.com> PR28430 diff --git a/doc/debuginfod.8 b/doc/debuginfod.8 index 1e56f6568322..ee8e4078e5b5 100644 --- a/doc/debuginfod.8 +++ b/doc/debuginfod.8 @@ -205,6 +205,23 @@ This important for controlling CPU-intensive operations like parsing an ELF file and especially decompressing archives. The default is the number of processors on the system; the minimum is 1. +.TP +.B "\-C" "\-C=NUM" "\-\-connection\-pool" "\-\-connection\-pool=NUM" +Set the size of the pool of threads serving webapi queries. The +following table summarizes the interpretaton of this option and its +optional NUM parameter. +.TS +l l. +no option clone new thread for every request, no fixed pool +\-C use a fixed thread pool sized automatically +\-C=NUM use a fixed thread pool sized NUM, minimum 2 +.TE + +The first mode is useful for friendly bursty traffic. The second mode +is a simple and safe configuration based on the number of processors. +The third mode is suitable for tuned load-limiting configurations +facing unruly traffic. + .TP .B "\-L" Traverse symbolic links encountered during traversal of the PATHs, diff --git a/tests/ChangeLog b/tests/ChangeLog index 76cb974a43c1..82061c6e9a58 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,3 +1,9 @@ +2021-12-09 Frank Ch. Eigler <fche@redhat.com> + + * debuginfod-subr.sh (xfail): New proc. + * run-debuginfod-webapi-concurrency.sh: New test for -C. + * Makefile.am: List it. + 2021-12-04 Mark Wielaard <mark@klomp.org> * Makefile.am (EXTRA_NLIST_CFLAGS): New variable depends on diff --git a/tests/Makefile.am b/tests/Makefile.am index 8acaeaf2a3a6..b2da2c83e15a 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -242,7 +242,8 @@ TESTS += run-debuginfod-dlopen.sh \ run-debuginfod-percent-escape.sh \ run-debuginfod-x-forwarded-for.sh \ run-debuginfod-response-headers.sh \ - run-debuginfod-extraction-passive.sh + run-debuginfod-extraction-passive.sh \ + run-debuginfod-webapi-concurrency.sh endif endif @@ -539,6 +540,7 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \ run-debuginfod-percent-escape.sh \ run-debuginfod-response-headers.sh \ run-debuginfod-extraction-passive.sh \ + run-debuginfod-webapi-concurrency.sh \ debuginfod-rpms/fedora30/hello2-1.0-2.src.rpm \ debuginfod-rpms/fedora30/hello2-1.0-2.x86_64.rpm \ debuginfod-rpms/fedora30/hello2-debuginfo-1.0-2.x86_64.rpm \ diff --git a/tests/debuginfod-subr.sh b/tests/debuginfod-subr.sh index 59033f35980e..0b59b5b87e55 100755 --- a/tests/debuginfod-subr.sh +++ b/tests/debuginfod-subr.sh @@ -158,3 +158,13 @@ PORT1=0 PORT2=0 PID1=0 PID2=0 + + +# run $1 as a sh -c command, invert result code +xfail() { + if sh -c "$1"; then + false + else + true + fi +} diff --git a/tests/run-debuginfod-webapi-concurrency.sh b/tests/run-debuginfod-webapi-concurrency.sh new file mode 100755 index 000000000000..513f834f40d5 --- /dev/null +++ b/tests/run-debuginfod-webapi-concurrency.sh @@ -0,0 +1,60 @@ +#!/usr/bin/env bash +# +# Copyright (C) 2021 Red Hat, Inc. +# This file is part of elfutils. +# +# This file is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# elfutils is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +. $srcdir/debuginfod-subr.sh + +# for test case debugging, uncomment: +set -x + +mkdir Z +# This variable is essential and ensures no time-race for claiming ports occurs +# set base to a unique multiple of 100 not used in any other 'run-debuginfod-*' test +base=12000 +get_ports + +export DEBUGINFOD_CACHE_PATH=${PWD}/.client_cache + +cp -rvp ${abs_srcdir}/debuginfod-tars Z +tempfiles Z + + +for Cnum in "" "-C" "-C10" "-C100" +do + env LD_LIBRARY_PATH=$ldpath ${abs_builddir}/../debuginfod/debuginfod $VERBOSE "$Cnum" -d :memory: -Z .tar.xz -Z .tar.bz2=bzcat -p $PORT1 -t0 -g0 -v --fdcache-fds=0 --fdcache-prefetch-fds=0 Z >> vlog$PORT1 2>&1 & + PID1=$! + tempfiles vlog$PORT1 + errfiles vlog$PORT1 + + wait_ready $PORT1 'ready' 1 + # Wait for server to finish indexing + wait_ready $PORT1 'thread_work_total{role="traverse"}' 1 + wait_ready $PORT1 'thread_work_pending{role="scan"}' 0 + wait_ready $PORT1 'thread_busy{role="scan"}' 0 + + # Do a bunch of lookups in parallel + for jobs in `seq 400`; do + curl -s http://localhost:$PORT1/buildid/cee13b2ea505a7f37bd20d271c6bc7e5f8d2dfcb/debuginfo > /dev/null & + done + (sleep 5; curl -s http://localhost:$PORT1/metrics | egrep 'error|transfers'; kill $PID1) & + wait + PID1=0 +done + +xfail "grep Server.reached.connection vlog$PORT1" # PR18661 + +exit 0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: patch rfc: PR28661: debuginfod thread-pool 2021-12-10 0:10 ` Frank Ch. Eigler @ 2021-12-10 17:44 ` Mark Wielaard 0 siblings, 0 replies; 5+ messages in thread From: Mark Wielaard @ 2021-12-10 17:44 UTC (permalink / raw) To: Frank Ch. Eigler; +Cc: elfutils-devel Hi Frank, On Thu, 2021-12-09 at 19:10 -0500, Frank Ch. Eigler via Elfutils-devel wrote: > So I would recommend to simply add a testcase that just uses no- > > option, > > -C and -C 256 (or take any arbitrary number, 1 might be an interesting > > corner case) and see that if you have 4 (8, 16, ...?) debuginfod-find > > (or curl metric) processes doing some parallel queries works as > > expected. > > OK, here's the current version, with a test, and with one more small > improvement to the error message printer (to add timestamp & batching). Looks good, but two nitpicks. > - > +static void // error logging callback from libmicrohttpd internals > +error_cb (void *arg, const char *fmt, va_list ap) > +{ > + (void) arg; > + inc_metric("error_count","libmicrohttpd",fmt); > + char errmsg[512]; > + (void) vsnprintf (errmsg, sizeof(errmsg), fmt, ap); // ok if slightly truncated > + obatched(cerr) << "libmicrohttpd error: " << errmsg; // MHD_DLOG calls already include \n > +} That vsnprintf and truncation comment had me scared for a minute. But vsnprintf always terminates the string with a '\0' so that should be fine. diff --git a/tests/ChangeLog b/tests/ChangeLog > diff --git a/tests/debuginfod-subr.sh b/tests/debuginfod-subr.sh > index 59033f35980e..0b59b5b87e55 100755 > --- a/tests/debuginfod-subr.sh > +++ b/tests/debuginfod-subr.sh > @@ -158,3 +158,13 @@ PORT1=0 > PORT2=0 > PID1=0 > PID2=0 > + > + > +# run $1 as a sh -c command, invert result code > +xfail() { > + if sh -c "$1"; then > + false > + else > + true > + fi > +} Aha, nice. We should use that in other tests too. > diff --git a/tests/run-debuginfod-webapi-concurrency.sh b/tests/run- > debuginfod-webapi-concurrency.sh > new file mode 100755 > index 000000000000..513f834f40d5 > --- /dev/null > +++ b/tests/run-debuginfod-webapi-concurrency.sh > @@ -0,0 +1,60 @@ > +#!/usr/bin/env bash > +# > +# Copyright (C) 2021 Red Hat, Inc. > +# This file is part of elfutils. > +# > +# This file is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published > by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# elfutils is distributed in the hope that it will be useful, but > +# WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see < > http://www.gnu.org/licenses/>. > + > +. $srcdir/debuginfod-subr.sh > + > +# for test case debugging, uncomment: > +set -x > + > +mkdir Z > +# This variable is essential and ensures no time-race for claiming > ports occurs > +# set base to a unique multiple of 100 not used in any other 'run- > debuginfod-*' test > +base=12000 > +get_ports > + > +export DEBUGINFOD_CACHE_PATH=${PWD}/.client_cache > + > +cp -rvp ${abs_srcdir}/debuginfod-tars Z > +tempfiles Z > + > + > +for Cnum in "" "-C" "-C10" "-C100" > +do > + env LD_LIBRARY_PATH=$ldpath > ${abs_builddir}/../debuginfod/debuginfod $VERBOSE "$Cnum" -d :memory: > -Z .tar.xz -Z .tar.bz2=bzcat -p $PORT1 -t0 -g0 -v --fdcache-fds=0 -- > fdcache-prefetch-fds=0 Z >> vlog$PORT1 2>&1 & > + PID1=$! > + tempfiles vlog$PORT1 > + errfiles vlog$PORT1 > + > + wait_ready $PORT1 'ready' 1 > + # Wait for server to finish indexing > + wait_ready $PORT1 'thread_work_total{role="traverse"}' 1 > + wait_ready $PORT1 'thread_work_pending{role="scan"}' 0 > + wait_ready $PORT1 'thread_busy{role="scan"}' 0 > + > + # Do a bunch of lookups in parallel > + for jobs in `seq 400`; do > + curl -s > http://localhost:$PORT1/buildid/cee13b2ea505a7f37bd20d271c6bc7e5f8d2dfcb/debuginfo > > /dev/null & > + done > + (sleep 5; curl -s http://localhost:$PORT1/metrics | egrep > 'error|transfers'; kill $PID1) & > + wait > + PID1=0 > +done Launching 400 processes at once seems a lot. But maybe machines these days are fine with that. I would say that 50 should also do the trick. But maybe I just have wimpy machines :) Is there a more elegant way to check all requests have been processed than a sleep 5 followed by a metric scrape for errors? Is there a metric that gives the number of successful queries that we could use instead with wait_ready? > +xfail "grep Server.reached.connection vlog$PORT1" # PR18661 > + > +exit 0 Cheers, Mark ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-12-10 17:45 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-09 3:55 patch rfc: PR28661: debuginfod thread-pool Frank Ch. Eigler 2021-12-09 16:47 ` Mark Wielaard 2021-12-09 16:54 ` Frank Ch. Eigler 2021-12-10 0:10 ` Frank Ch. Eigler 2021-12-10 17:44 ` Mark Wielaard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).