From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (gnu.wildebeest.org [45.83.234.184]) by sourceware.org (Postfix) with ESMTPS id 4ADAD3857C6B for ; Fri, 10 Dec 2021 17:45:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4ADAD3857C6B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: from tarox.wildebeest.org (83-87-18-245.cable.dynamic.v4.ziggo.nl [83.87.18.245]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 14D0D3000ED0; Fri, 10 Dec 2021 18:44:57 +0100 (CET) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id 91EB8400027B; Fri, 10 Dec 2021 18:44:57 +0100 (CET) Message-ID: <69c6009f046b2cee82f271ada847cb4752e09eab.camel@klomp.org> Subject: Re: patch rfc: PR28661: debuginfod thread-pool From: Mark Wielaard To: "Frank Ch. Eigler" Cc: elfutils-devel@sourceware.org Date: Fri, 10 Dec 2021 18:44:57 +0100 In-Reply-To: <20211210001052.GA25363@redhat.com> References: <20211209035541.GA21366@redhat.com> <20211210001052.GA25363@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.28.5 (3.28.5-10.el7) Mime-Version: 1.0 X-Spam-Status: No, score=-9.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Dec 2021 17:45:04 -0000 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,=20 > > -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. >=20 > 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. =20 > - > +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=3D0 > PORT2=3D0 > PID1=3D0 > PID2=3D0 > + > + > +# 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=3D12000 > +get_ports > + > +export DEBUGINFOD_CACHE_PATH=3D${PWD}/.client_cache > + > +cp -rvp ${abs_srcdir}/debuginfod-tars Z > +tempfiles Z > + > + > +for Cnum in "" "-C" "-C10" "-C100" > +do > + env LD_LIBRARY_PATH=3D$ldpath > ${abs_builddir}/../debuginfod/debuginfod $VERBOSE "$Cnum" -d :memory: > -Z .tar.xz -Z .tar.bz2=3Dbzcat -p $PORT1 -t0 -g0 -v --fdcache-fds=3D0 -- > fdcache-prefetch-fds=3D0 Z >> vlog$PORT1 2>&1 & > + PID1=3D$! > + tempfiles vlog$PORT1 > + errfiles vlog$PORT1 > + =20 > + wait_ready $PORT1 'ready' 1 > + # Wait for server to finish indexing > + wait_ready $PORT1 'thread_work_total{role=3D"traverse"}' 1 > + wait_ready $PORT1 'thread_work_pending{role=3D"scan"}' 0 > + wait_ready $PORT1 'thread_busy{role=3D"scan"}' 0 > + > + # Do a bunch of lookups in parallel > + for jobs in `seq 400`; do > + curl -s=20 > http://localhost:$PORT1/buildid/cee13b2ea505a7f37bd20d271c6bc7e5f8d2dfcb/= debuginfo > > /dev/null & > + done > + (sleep 5; curl -s http://localhost:$PORT1/metrics | egrep > 'error|transfers'; kill $PID1) & > + wait > + PID1=3D0 > +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