From: Amos Jeffries Date: Fri, 5 Jun 2015 05:56:36 +0000 (-0700) Subject: SourceLayout: convert helper stats display to Packable API X-Git-Tag: merge-candidate-3-v1~89 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=bf3e8d5a0de0f6f59996d1a8fb88402ba2273ede;p=thirdparty%2Fsquid.git SourceLayout: convert helper stats display to Packable API Requires unifying the classes Helper::Request queues which incidentally also brings stateful helpers closer to concurrency support --- diff --git a/src/auth/basic/Config.cc b/src/auth/basic/Config.cc index ec43ab555f..04387ad786 100644 --- a/src/auth/basic/Config.cc +++ b/src/auth/basic/Config.cc @@ -147,7 +147,8 @@ Auth::Basic::Config::parse(Auth::Config * scheme, int n_configured, char *param_ static void authenticateBasicStats(StoreEntry * sentry) { - helperStats(sentry, basicauthenticators, "Basic Authenticator Statistics"); + if (basicauthenticators) + basicauthenticators->packStatsInto(sentry, "Basic Authenticator Statistics"); } char * diff --git a/src/auth/digest/Config.cc b/src/auth/digest/Config.cc index ef5b5b350f..49ca98201a 100644 --- a/src/auth/digest/Config.cc +++ b/src/auth/digest/Config.cc @@ -643,7 +643,8 @@ Auth::Digest::Config::type() const static void authenticateDigestStats(StoreEntry * sentry) { - helperStats(sentry, digestauthenticators, "Digest Authenticator Statistics"); + if (digestauthenticators) + digestauthenticators->packStatsInto(sentry, "Digest Authenticator Statistics"); } /* NonceUserUnlink: remove the reference to auth_user and unlink the node from the list */ diff --git a/src/auth/negotiate/Config.cc b/src/auth/negotiate/Config.cc index f3d8193a5d..956be3995f 100644 --- a/src/auth/negotiate/Config.cc +++ b/src/auth/negotiate/Config.cc @@ -244,7 +244,8 @@ Auth::Negotiate::Config::fixHeader(Auth::UserRequest::Pointer auth_user_request, static void authenticateNegotiateStats(StoreEntry * sentry) { - helperStatefulStats(sentry, negotiateauthenticators, "Negotiate Authenticator Statistics"); + if (negotiateauthenticators) + negotiateauthenticators->packStatsInto(sentry, "Negotiate Authenticator Statistics"); } /* diff --git a/src/auth/ntlm/Config.cc b/src/auth/ntlm/Config.cc index dbbd479273..0f9f7f5bf2 100644 --- a/src/auth/ntlm/Config.cc +++ b/src/auth/ntlm/Config.cc @@ -224,7 +224,8 @@ Auth::Ntlm::Config::fixHeader(Auth::UserRequest::Pointer auth_user_request, Http static void authenticateNTLMStats(StoreEntry * sentry) { - helperStatefulStats(sentry, ntlmauthenticators, "NTLM Authenticator Statistics"); + if (ntlmauthenticators) + ntlmauthenticators->packStatsInto(sentry, "NTLM Authenticator Statistics"); } /* diff --git a/src/external_acl.cc b/src/external_acl.cc index 6b2e06e67e..a2247db9ec 100644 --- a/src/external_acl.cc +++ b/src/external_acl.cc @@ -1464,7 +1464,8 @@ externalAclStats(StoreEntry * sentry) for (external_acl *p = Config.externalAclHelperList; p; p = p->next) { storeAppendPrintf(sentry, "External ACL Statistics: %s\n", p->name); storeAppendPrintf(sentry, "Cache size: %d\n", p->cache->count); - helperStats(sentry, p->theHelper); + assert(p->theHelper); + p->theHelper->packStatsInto(sentry); storeAppendPrintf(sentry, "\n"); } } diff --git a/src/helper.cc b/src/helper.cc index 94bbd2143f..47b1515e04 100644 --- a/src/helper.cc +++ b/src/helper.cc @@ -10,6 +10,7 @@ #include "squid.h" #include "base/AsyncCbdataCalls.h" +#include "base/Packable.h" #include "comm.h" #include "comm/Connection.h" #include "comm/Read.h" @@ -63,7 +64,6 @@ static void helperKickQueue(helper * hlp); static void helperStatefulKickQueue(statefulhelper * hlp); static void helperStatefulServerDone(helper_stateful_server * srv); static void StatefulEnqueue(statefulhelper * hlp, Helper::Request * r); -static bool helperStartStats(StoreEntry *sentry, void *hlp, const char *label); CBDATA_CLASS_INIT(helper); CBDATA_CLASS_INIT(helper_server); @@ -471,7 +471,7 @@ void statefulhelper::submit(const char *buf, HLPCB * callback, void *data, helpe if ((buf != NULL) && lastserver) { debugs(84, 5, "StatefulSubmit with lastserver " << lastserver); assert(lastserver->flags.reserved); - assert(!(lastserver->request)); + assert(!lastserver->requests.size()); debugs(84, 5, "StatefulSubmit dispatching"); helperStatefulDispatch(lastserver, r); @@ -523,31 +523,21 @@ helperStatefulServerGetData(helper_stateful_server * srv) return srv->data; } -/** - * Dump some stats about the helper states to a StoreEntry - */ void -helperStats(StoreEntry * sentry, helper * hlp, const char *label) +helper::packStatsInto(Packable *p, const char *label) const { - if (!helperStartStats(sentry, hlp, label)) - return; - - storeAppendPrintf(sentry, "program: %s\n", - hlp->cmdline->key); - storeAppendPrintf(sentry, "number active: %d of %d (%d shutting down)\n", - hlp->childs.n_active, hlp->childs.n_max, (hlp->childs.n_running - hlp->childs.n_active) ); - storeAppendPrintf(sentry, "requests sent: %d\n", - hlp->stats.requests); - storeAppendPrintf(sentry, "replies received: %d\n", - hlp->stats.replies); - storeAppendPrintf(sentry, "requests timedout: %d\n", - hlp->stats.timedout); - storeAppendPrintf(sentry, "queue length: %d\n", - hlp->stats.queue_size); - storeAppendPrintf(sentry, "avg service time: %d msec\n", - hlp->stats.avg_svc_time); - storeAppendPrintf(sentry, "\n"); - storeAppendPrintf(sentry, "%7s\t%7s\t%7s\t%11s\t%11s\t%11s\t%s\t%7s\t%7s\t%7s\n", + if (label) + p->appendf("%s:\n", label); + + p->appendf(" program: %s\n", cmdline->key); + p->appendf(" number active: %d of %d (%d shutting down)\n", childs.n_active, childs.n_max, (childs.n_running - childs.n_active)); + p->appendf(" requests sent: %d\n", stats.requests); + p->appendf(" replies received: %d\n", stats.replies); + p->appendf(" requests timedout: %d\n", stats.timedout); + p->appendf(" queue length: %d\n", stats.queue_size); + p->appendf(" avg service time: %d msec\n", stats.avg_svc_time); + p->append("\n",1); + p->appendf("%7s\t%7s\t%7s\t%11s\t%11s\t%11s\t%6s\t%7s\t%7s\t%7s\n", "ID #", "FD", "PID", @@ -559,11 +549,12 @@ helperStats(StoreEntry * sentry, helper * hlp, const char *label) "Offset", "Request"); - for (dlink_node *link = hlp->servers.head; link; link = link->next) { - helper_server *srv = (helper_server*)link->data; + for (dlink_node *link = servers.head; link; link = link->next) { + HelperServerBase *srv = static_cast(link->data); + assert(srv); Helper::Request *request = srv->requests.empty() ? NULL : srv->requests.front(); double tt = 0.001 * (request ? tvSubMsec(request->dispatch_time, current_time) : tvSubMsec(srv->dispatch_time, srv->answer_time)); - storeAppendPrintf(sentry, "%7u\t%7d\t%7d\t%11" PRIu64 "\t%11" PRIu64 "\t%11" PRIu64 "\t%c%c%c%c\t%7.3f\t%7d\t%s\n", + p->appendf("%7u\t%7d\t%7d\t%11" PRIu64 "\t%11" PRIu64 "\t%11" PRIu64 "\t%c%c%c%c%c%c\t%7.3f\t%7d\t%s\n", srv->index.value, srv->readPipe->fd, srv->pid, @@ -573,74 +564,21 @@ helperStats(StoreEntry * sentry, helper * hlp, const char *label) srv->stats.pending ? 'B' : ' ', srv->flags.writing ? 'W' : ' ', srv->flags.closing ? 'C' : ' ', - srv->flags.shutdown ? 'S' : ' ', - tt < 0.0 ? 0.0 : tt, - (int) srv->roffset, - request ? Format::QuoteMimeBlob(request->buf) : "(none)"); - } - - storeAppendPrintf(sentry, "\nFlags key:\n\n"); - storeAppendPrintf(sentry, " B = BUSY\n"); - storeAppendPrintf(sentry, " W = WRITING\n"); - storeAppendPrintf(sentry, " C = CLOSING\n"); - storeAppendPrintf(sentry, " S = SHUTDOWN PENDING\n"); -} - -void -helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label) -{ - if (!helperStartStats(sentry, hlp, label)) - return; - - storeAppendPrintf(sentry, "program: %s\n", - hlp->cmdline->key); - storeAppendPrintf(sentry, "number active: %d of %d (%d shutting down)\n", - hlp->childs.n_active, hlp->childs.n_max, (hlp->childs.n_running - hlp->childs.n_active) ); - storeAppendPrintf(sentry, "requests sent: %d\n", - hlp->stats.requests); - storeAppendPrintf(sentry, "replies received: %d\n", - hlp->stats.replies); - storeAppendPrintf(sentry, "queue length: %d\n", - hlp->stats.queue_size); - storeAppendPrintf(sentry, "avg service time: %d msec\n", - hlp->stats.avg_svc_time); - storeAppendPrintf(sentry, "\n"); - storeAppendPrintf(sentry, "%7s\t%7s\t%7s\t%11s\t%11s\t%6s\t%7s\t%7s\t%7s\n", - "ID #", - "FD", - "PID", - "# Requests", - "# Replies", - "Flags", - "Time", - "Offset", - "Request"); - - for (dlink_node *link = hlp->servers.head; link; link = link->next) { - helper_stateful_server *srv = (helper_stateful_server *)link->data; - double tt = 0.001 * tvSubMsec(srv->dispatch_time, srv->stats.pending ? current_time : srv->answer_time); - storeAppendPrintf(sentry, "%7u\t%7d\t%7d\t%11" PRIu64 "\t%11" PRIu64 "\t%c%c%c%c%c\t%7.3f\t%7d\t%s\n", - srv->index.value, - srv->readPipe->fd, - srv->pid, - srv->stats.uses, - srv->stats.replies, - srv->stats.pending ? 'B' : ' ', - srv->flags.closing ? 'C' : ' ', srv->flags.reserved ? 'R' : ' ', srv->flags.shutdown ? 'S' : ' ', - srv->request ? (srv->request->placeholder ? 'P' : ' ') : ' ', + request && request->placeholder ? 'P' : ' ', tt < 0.0 ? 0.0 : tt, (int) srv->roffset, - srv->request ? Format::QuoteMimeBlob(srv->request->buf) : "(none)"); + request ? Format::QuoteMimeBlob(request->buf) : "(none)"); } - storeAppendPrintf(sentry, "\nFlags key:\n\n"); - storeAppendPrintf(sentry, " B = BUSY\n"); - storeAppendPrintf(sentry, " C = CLOSING\n"); - storeAppendPrintf(sentry, " R = RESERVED\n"); - storeAppendPrintf(sentry, " S = SHUTDOWN PENDING\n"); - storeAppendPrintf(sentry, " P = PLACEHOLDER\n"); + p->append("\nFlags key:\n" + " B\tBUSY\n" + " W\tWRITING\n" + " C\tCLOSING\n" + " R\tRESERVED\n" + " S\tSHUTDOWN PENDING\n" + " P\tPLACEHOLDER\n", 101); } void @@ -743,7 +681,6 @@ static void helperServerFree(helper_server *srv) { helper *hlp = srv->parent; - Helper::Request *r; int concurrency = hlp->childs.concurrency; if (!concurrency) @@ -793,7 +730,7 @@ helperServerFree(helper_server *srv) while (!srv->requests.empty()) { // XXX: re-schedule these on another helper? - r = srv->requests.front(); + Helper::Request *r = srv->requests.front(); srv->requests.pop_front(); void *cbdata; @@ -814,7 +751,6 @@ static void helperStatefulServerFree(helper_stateful_server *srv) { statefulhelper *hlp = srv->parent; - Helper::Request *r; if (srv->rbuf) { memFreeBuf(srv->rbuf_sz, srv->rbuf); @@ -857,18 +793,18 @@ helperStatefulServerFree(helper_stateful_server *srv) } } - if ((r = srv->request)) { + while (!srv->requests.empty()) { + // XXX: re-schedule these on another helper? + Helper::Request *r = srv->requests.front(); + srv->requests.pop_front(); void *cbdata; if (cbdataReferenceValidDone(r->data, &cbdata)) { Helper::Reply nilReply; - nilReply.whichServer = srv; r->callback(cbdata, nilReply); } delete r; - - srv->request = NULL; } if (srv->data != NULL) @@ -1051,7 +987,6 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len { char *t = NULL; helper_stateful_server *srv = (helper_stateful_server *)data; - Helper::Request *r; statefulhelper *hlp = srv->parent; assert(cbdataReferenceValid(data)); @@ -1073,7 +1008,7 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len srv->roffset += len; srv->rbuf[srv->roffset] = '\0'; - r = srv->request; + Helper::Request *r = srv->requests.front(); debugs(84, DBG_DATA, Raw("accumulated", srv->rbuf, srv->roffset)); if (r == NULL) { @@ -1087,6 +1022,7 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len if ((t = strchr(srv->rbuf, hlp->eom))) { /* end of reply found */ + srv->requests.pop_front(); // we already have it in 'r' int called = 1; int skip = 1; debugs(84, 3, "helperStatefulHandleRead: end of reply found"); @@ -1120,7 +1056,6 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len */ srv->roffset = 0; delete r; - srv->request = NULL; -- srv->stats.pending; ++ srv->stats.replies; @@ -1437,14 +1372,14 @@ helperStatefulDispatch(helper_stateful_server * srv, Helper::Request * r) /* and push the queue. Note that the callback may have submitted a new * request to the helper which is why we test for the request */ - if (srv->request == NULL) + if (!srv->requests.size()) helperStatefulServerDone(srv); return; } srv->flags.reserved = true; - srv->request = r; + srv->requests.push_back(r); srv->dispatch_time = current_time; AsyncCall::Pointer call = commCbCall(5,5, "helperStatefulDispatchWriteDone", CommIoCbPtrFun(helperStatefulDispatchWriteDone, hlp)); @@ -1489,22 +1424,6 @@ helperStatefulServerDone(helper_stateful_server * srv) } } -// TODO: should helper_ and helper_stateful_ have a common parent? -static bool -helperStartStats(StoreEntry *sentry, void *hlp, const char *label) -{ - if (!hlp) { - if (label) - storeAppendPrintf(sentry, "%s: unavailable\n", label); - return false; - } - - if (label) - storeAppendPrintf(sentry, "%s:\n", label); - - return true; -} - void helper_server::checkForTimedOutRequests(bool const retry) { diff --git a/src/helper.h b/src/helper.h index 4991811e1e..2b3dcc95c1 100644 --- a/src/helper.h +++ b/src/helper.h @@ -24,6 +24,8 @@ #include #include +class Packable; + /** * Managers a set of individual helper processes with a common queue of requests. * @@ -68,6 +70,10 @@ public: /// Submits a request to the helper or add it to the queue if none of /// the servers is available. void submitRequest(Helper::Request *r); + + /// Dump some stats about the helper state to a Packable object + void packStatsInto(Packable *p, const char *label = NULL) const; + public: wordlist *cmdline; dlink_list servers; @@ -164,6 +170,9 @@ public: bool reserved; } flags; + typedef std::list Requests; + Requests requests; ///< requests in order of submission/expiration + struct { uint64_t uses; //< requests sent to this helper uint64_t replies; //< replies received from this helper @@ -189,9 +198,6 @@ public: helper *parent; - typedef std::list Requests; - Requests requests; ///< requests in order of submission/expiration - // STL says storing std::list iterators is safe when changing the list typedef std::map RequestIndex; RequestIndex requestsIndex; ///< maps request IDs to requests @@ -213,7 +219,6 @@ public: /* MemBuf writebuf; */ statefulhelper *parent; - Helper::Request *request; void *data; /* State data used by the calling routines */ }; @@ -223,8 +228,6 @@ void helperOpenServers(helper * hlp); void helperStatefulOpenServers(statefulhelper * hlp); void helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data); void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPCB * callback, void *data, helper_stateful_server * lastserver); -void helperStats(StoreEntry * sentry, helper * hlp, const char *label = NULL); -void helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label = NULL); void helperShutdown(helper * hlp); void helperStatefulShutdown(statefulhelper * hlp); void helperStatefulReleaseServer(helper_stateful_server * srv); diff --git a/src/redirect.cc b/src/redirect.cc index 80beba17bc..1fac3fc09e 100644 --- a/src/redirect.cc +++ b/src/redirect.cc @@ -200,7 +200,7 @@ redirectStats(StoreEntry * sentry) return; } - helperStats(sentry, redirectors, "Redirector Statistics"); + redirectors->packStatsInto(sentry, "Redirector Statistics"); if (Config.onoff.redirector_bypass) storeAppendPrintf(sentry, "\nNumber of requests bypassed " @@ -215,7 +215,7 @@ storeIdStats(StoreEntry * sentry) return; } - helperStats(sentry, storeIds, "StoreId helper Statistics"); + storeIds->packStatsInto(sentry, "StoreId helper Statistics"); if (Config.onoff.store_id_bypass) storeAppendPrintf(sentry, "\nNumber of requests bypassed " diff --git a/src/tests/stub_helper.cc b/src/tests/stub_helper.cc index 56825f2635..b6608ed702 100644 --- a/src/tests/stub_helper.cc +++ b/src/tests/stub_helper.cc @@ -16,9 +16,8 @@ void helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data) S void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPCB * callback, void *data, helper_stateful_server * lastserver) STUB helper::~helper() STUB CBDATA_CLASS_INIT(helper); +void helper::packStatsInto(Packable *p, const char *label) const STUB -void helperStats(StoreEntry * sentry, helper * hlp, const char *label) STUB -void helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label) STUB void helperShutdown(helper * hlp) STUB void helperStatefulShutdown(statefulhelper * hlp) STUB void helperOpenServers(helper * hlp) STUB