Re: pcp updates: libpcp connection unsharing

"Mark Goodwin" <[email protected]>
Newsgroups gmane.comp.sysutils.pcp
Message-ID <[email protected]> 
On Thu, Nov 3, 2016 at 7:07 AM, Frank Ch. Eigler <[email protected]> wrote:

> commit e257bb5e59dc4313c7ba32fbfdad46e04b175ae0
> Author: Frank Ch. Eigler <[email protected]>
> Date:   Wed Nov 2 14:44:10 2016 -0400
>
>     github issues/118: stop sharing sockets to same pmcd
>
>     More crashes & hangs have been observed that implicate outgoing tcp
>     connection sharing in libpcp.  It is time to turn off this
>     optimization (?) to squash any remaining latent bugs.  It should in
>     turn allow removal of the c_pmcd refcnt/mutex, but that's left for a
>     later patch.  QA impact is minimal.  Only one test case (024) actually
>     shows refcnt=X deltas, so its .out is recomputed here.
>

I'm unsure whether to merge this one. On the one hand it should squash
numerous bugs involving locking and connection sharing and simplify the
code (which sounds good), but on the other hand, how close are we to
understanding those bugs and what functionality do we lose? and what are
the performance or resource (maxfds?) implications? Do we need the c_pmcd
refcnt/mutex followup patch too? Any comments anyone? Ken?

Frank, the patch itself seems superficially OK, except for this bit:
    if (1 /* new->c_pmcd == NULL*/) {
which should be easily cleaned up.

I've attached the patch itself for reference and discussion.
pcp-fche-stop-socket-sharing.patch (text/x-patch, 5.2 KB) 
commit e257bb5e59dc4313c7ba32fbfdad46e04b175ae0
Author: Frank Ch. Eigler <[email protected]>
Date:   Wed Nov 2 14:44:10 2016 -0400

    github issues/118: stop sharing sockets to same pmcd
    
    More crashes & hangs have been observed that implicate outgoing tcp
    connection sharing in libpcp.  It is time to turn off this
    optimization (?) to squash any remaining latent bugs.  It should in
    turn allow removal of the c_pmcd refcnt/mutex, but that's left for a
    later patch.  QA impact is minimal.  Only one test case (024) actually
    shows refcnt=X deltas, so its .out is recomputed here.

diff --git a/qa/024.out b/qa/024.out
index d9de98d..745ed99 100644
--- a/qa/024.out
+++ b/qa/024.out
@@ -8,10 +8,13 @@ pmNewContext(1, <localhost>) -> 0
 Dump Contexts: current context = 0
 Context[0] host <localhost>: pmcd=CONNECTED profile=NOT_SENT fd=<n> refcnt=1 origin=0.000000 delta=0
 Dump Instance Profile state=INCLUDE, 0 profiles
-__pmDecodeError: got error PDU (code=-12358, fromversion=2)
+__pmDecodeXtendError: got error PDU (code=0, datum=DATA, version=2)
+__pmSetVersionIPC: fd=<n> version=2
+__pmSendCreds: #0 = 1020000
+__pmConnectPMCD(<localhost>): pmcd connection port=<n> fd=<n> PDU version=2
 pmNewContext(1, <localhost>) -> 1
 Dump Contexts: current context = 1
-Context[1] host <localhost>: pmcd=CONNECTED profile=NOT_SENT fd=<n> refcnt=2 origin=0.000000 delta=0
+Context[1] host <localhost>: pmcd=CONNECTED profile=NOT_SENT fd=<n> refcnt=1 origin=0.000000 delta=0
 Dump Instance Profile state=INCLUDE, 0 profiles
 pmUseContext(1) -> 0
 pmDestroyContext(1) -> 0, curcontext=-1
@@ -25,10 +28,13 @@ pmNewContext(1, <localhost>) -> 0
 Dump Contexts: current context = 0
 Context[0] host <localhost>: pmcd=CONNECTED profile=NOT_SENT fd=<n> refcnt=1 origin=0.000000 delta=0
 Dump Instance Profile state=INCLUDE, 0 profiles
-__pmDecodeError: got error PDU (code=-12358, fromversion=2)
+__pmDecodeXtendError: got error PDU (code=0, datum=DATA, version=2)
+__pmSetVersionIPC: fd=<n> version=2
+__pmSendCreds: #0 = 1020000
+__pmConnectPMCD(<localhost>): pmcd connection port=<n> fd=<n> PDU version=2
 pmNewContext(1, <localhost>) -> 1
 Dump Contexts: current context = 1
-Context[1] host <localhost>: pmcd=CONNECTED profile=NOT_SENT fd=<n> refcnt=2 origin=0.000000 delta=0
+Context[1] host <localhost>: pmcd=CONNECTED profile=NOT_SENT fd=<n> refcnt=1 origin=0.000000 delta=0
 Dump Instance Profile state=INCLUDE, 0 profiles
 pmUseContext(1) -> 0
 pmDestroyContext(1) -> 0, curcontext=-1
diff --git a/src/libpcp/src/context.c b/src/libpcp/src/context.c
index 75d18ef..28550af 100644
--- a/src/libpcp/src/context.c
+++ b/src/libpcp/src/context.c
@@ -1015,64 +1015,13 @@ INIT_CONTEXT:
 	}
 
 	/*
-	 * As an optimization, if there is already a connection to the
-	 * same PMCD, we try to reuse (share) it.  This is not viable
-	 * in several situations - when pmproxy is in use, or when any
-	 * connection attribute(s) are set, or when exclusion has been
-	 * explicitly requested (i.e. PM_CTXFLAG_EXCLUSIVE in c_flags).
-	 * A reference count greater than one indicates active sharing.
-	 *
-	 * Note the detection of connection-to-same-pmcd is flawed, as
-	 * hostname equality does not necessarily mean the connections
-	 * are equal; e.g., the IP address might have changed.
-	 *
-	 * It is the topic of some debate as to whether PMCD connection
-	 * sharing is of much value at all, especially considering the
-	 * number of subtle and nasty bugs it has caused over time.  Do
-	 * not rely on this behaviour, it may well be removed someday.
-	 *
-	 * NB: Take the libpcp lock while we search the contexts[].
-	 * This is not great, as the ping_pmcd() check can take some
-	 * milliseconds, but it is necessary to avoid races between
-	 * pmDestroyContext() and/or memory ordering.  For connections
-	 * being shared, the refcnt is incremented under libpcp lock.
-	 * Decrementing refcnt occurs in pmDestroyContext while holding
-	 * both the libpcp and context locks.
-	 */
-	PM_LOCK(__pmLock_libpcp);
-	if (nhosts == 1) { /* not proxied */
-	    for (i = 0; i < contexts_len; i++) {
-		__pmPMCDCtl *pmcd;
-
-		if (i == PM_TPD(curcontext))
-		    continue;
-		pmcd = contexts[i]->c_pmcd;
-		if (contexts[i]->c_type == new->c_type &&
-		    contexts[i]->c_flags == new->c_flags &&
-		    contexts[i]->c_flags == 0 &&
-		    strcmp(pmcd->pc_hosts[0].name, hosts[0].name) == 0 &&
-		    pmcd->pc_hosts[0].nports == hosts[0].nports) {
-		    int j, ports_same = 1;
-
-		    for (j = 0; j < hosts[0].nports; j++) {
-			if (pmcd->pc_hosts[0].ports[j] != hosts[0].ports[j]) {
-			    ports_same = 0;
-			    break;
-			}
-		    }
-
-		    /* ports match, check that pmcd is alive too */
-		    if (ports_same && ping_pmcd(i, pmcd)) {
-			new->c_pmcd = pmcd;
-			new->c_pmcd->pc_refcnt++;
-			break;
-		    }
-		}
-	    }
-	}
-	PM_UNLOCK(__pmLock_libpcp);
-
-	if (new->c_pmcd == NULL) {
+         * Previously, as an optimization, previous connections to the
+         * same PMCD were potentially reused.  (Concurrency was
+         * managed via c_pmcd->pc_refcnt and c_pmcd->pc_mutex.)
+         * We no longer do this, because of subtle and nasty bugs
+         * it has caused over time.
+         */
+	if (1 /* new->c_pmcd == NULL*/) {
 	    /*
 	     * Try to establish the connection.
 	     * If this fails, restore the original current context
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.