From 37c2d74c2c9c56690b984cc039ca6e1ed9dd341e Mon Sep 17 00:00:00 2001 From: Rich Megginson Date: Fri, 5 Apr 2013 14:29:16 -0600 Subject: [PATCH 4/4] Ticket #47320 - put conn on work_q not poll list if conn has buffered more_data https://fedorahosted.org/389/ticket/47320 Reviewed by: ??? Branch: master Fix Description: If we have a connection buffer (nsslapd-connection-buffer > 0) and we have data left over in the buffer after reading the op PDU, add the connection to the work_q so another thread can immediately begin on the next operation for the connection, rather than putting it back into the poll() array. There were some race conditions introduced into connection_read_operation so conn->c_mutex had to be locked in that function. Platforms tested: RHEL6 x86_64 Flag Day: no Doc impact: no --- ldap/servers/slapd/connection.c | 97 +++++++++++++++++++++++++++------------ 1 files changed, 67 insertions(+), 30 deletions(-) diff --git a/ldap/servers/slapd/connection.c b/ldap/servers/slapd/connection.c index 133c486..359dabe 100644 --- a/ldap/servers/slapd/connection.c +++ b/ldap/servers/slapd/connection.c @@ -1813,6 +1813,8 @@ int connection_wait_for_new_work(Slapi_PBlock *pb, PRIntervalTime interval) * Utility function called by connection_read_operation(). This is a * small wrapper on top of libldap's ber_get_next_buffer_ext(). * + * Caller must hold conn->c_mutex + * * Return value: * 0: Success * case 1) If there was not enough data in the buffer to complete the @@ -1872,7 +1874,7 @@ get_next_from_buffer( void *buffer, size_t buffer_size, ber_len_t *lenp, conn->c_private->c_buffer_bytes = conn->c_private->c_buffer_offset = 0; /* drop connection */ - disconnect_server( conn, conn->c_connid, -1, err, syserr ); + disconnect_server_nomutex( conn, conn->c_connid, -1, err, syserr ); return -1; } else if (CONNECTION_BUFFER_OFF == conn->c_private->use_buffer) { *lenp = bytes_scanned; @@ -1933,7 +1935,8 @@ int connection_read_operation(Connection *conn, Operation *op, ber_tag_t *tag, i char *buffer = conn->c_private->c_buffer; PRErrorCode err = 0; PRInt32 syserr = 0; - + + PR_Lock(conn->c_mutex); /* * if the socket is still valid, get the ber element * waiting for us on this connection. timeout is handled @@ -1941,7 +1944,8 @@ int connection_read_operation(Connection *conn, Operation *op, ber_tag_t *tag, i */ if ( (conn->c_sd == SLAPD_INVALID_SOCKET) || (conn->c_flags & CONN_FLAG_CLOSING) ) { - return CONN_DONE; + ret = CONN_DONE; + goto done; } *tag = LBER_DEFAULT; @@ -1953,7 +1957,8 @@ int connection_read_operation(Connection *conn, Operation *op, ber_tag_t *tag, i conn->c_private->c_buffer_bytes - conn->c_private->c_buffer_offset, &len, tag, op->o_ber, conn )) { - return CONN_DONE; + ret = CONN_DONE; + goto done; } new_operation = 0; } @@ -1968,7 +1973,8 @@ int connection_read_operation(Connection *conn, Operation *op, ber_tag_t *tag, i } else { ret = get_next_from_buffer( NULL, 0, &len, tag, op->o_ber, conn ); if (ret == -1) { - return CONN_DONE; /* get_next_from_buffer does the disconnect stuff */ + ret = CONN_DONE; + goto done; /* get_next_from_buffer does the disconnect stuff */ } else if (ret == 0) { ret = len; } @@ -1977,12 +1983,11 @@ int connection_read_operation(Connection *conn, Operation *op, ber_tag_t *tag, i if (ret <= 0) { if (0 == ret) { /* Connection is closed */ - PR_Lock( conn->c_mutex ); disconnect_server_nomutex( conn, conn->c_connid, -1, SLAPD_DISCONNECT_BAD_BER_TAG, 0 ); conn->c_gettingber = 0; - PR_Unlock( conn->c_mutex ); signal_listner(); - return CONN_DONE; + ret = CONN_DONE; + goto done; } /* err = PR_GetError(); */ /* If we would block, we need to poll for a while */ @@ -2000,19 +2005,22 @@ int connection_read_operation(Connection *conn, Operation *op, ber_tag_t *tag, i if (0 == ret) { /* We timed out, should the server shutdown ? */ if (op_shutdown) { - return CONN_SHUTDOWN; + ret = CONN_SHUTDOWN; + goto done; } /* We timed out, is this the first read in a PDU ? */ if (new_operation) { /* If so, we return */ - return CONN_TIMEDOUT; + ret = CONN_TIMEDOUT; + goto done; } else { /* Otherwise we loop, unless we exceeded the ioblock timeout */ if (waits_done > ioblocktimeout_waits) { LDAPDebug( LDAP_DEBUG_CONNS,"ioblock timeout expired on connection %" NSPRIu64 "\n", conn->c_connid, 0, 0 ); - disconnect_server( conn, conn->c_connid, -1, + disconnect_server_nomutex( conn, conn->c_connid, -1, SLAPD_DISCONNECT_IO_TIMEOUT, 0 ); - return CONN_DONE; + ret = CONN_DONE; + goto done; } else { /* The turbo mode may cause threads starvation. @@ -2031,8 +2039,9 @@ int connection_read_operation(Connection *conn, Operation *op, ber_tag_t *tag, i LDAPDebug( LDAP_DEBUG_ANY, "PR_Poll for connection %" NSPRIu64 " returns %d (%s)\n", conn->c_connid, err, slapd_pr_strerror( err ) ); /* If this happens we should close the connection */ - disconnect_server( conn, conn->c_connid, -1, err, syserr ); - return CONN_DONE; + disconnect_server_nomutex( conn, conn->c_connid, -1, err, syserr ); + ret = CONN_DONE; + goto done; } } else { /* Some other error, typically meaning bad stuff */ @@ -2040,8 +2049,9 @@ int connection_read_operation(Connection *conn, Operation *op, ber_tag_t *tag, i LDAPDebug( LDAP_DEBUG_CONNS, "PR_Recv for connection %" NSPRIu64 " returns %d (%s)\n", conn->c_connid, err, slapd_pr_strerror( err ) ); /* If this happens we should close the connection */ - disconnect_server( conn, conn->c_connid, -1, err, syserr ); - return CONN_DONE; + disconnect_server_nomutex( conn, conn->c_connid, -1, err, syserr ); + ret = CONN_DONE; + goto done; } } else { /* We read some data off the network, do something with it */ @@ -2053,7 +2063,8 @@ int connection_read_operation(Connection *conn, Operation *op, ber_tag_t *tag, i conn->c_private->c_buffer_bytes - conn->c_private->c_buffer_offset, &len, tag, op->o_ber, conn ) != 0 ) { - return CONN_DONE; + ret = CONN_DONE; + goto done; } } @@ -2074,9 +2085,10 @@ int connection_read_operation(Connection *conn, Operation *op, ber_tag_t *tag, i LDAPDebug( LDAP_DEBUG_ANY, "conn=%" NSPRIu64 " received a non-LDAP message (tag 0x%lx, expected 0x%lx)\n", conn->c_connid, *tag, LDAP_TAG_MESSAGE ); - disconnect_server( conn, conn->c_connid, -1, + disconnect_server_nomutex( conn, conn->c_connid, -1, SLAPD_DISCONNECT_BAD_BER_TAG, EPROTO ); - return CONN_DONE; + ret = CONN_DONE; + goto done; } if ( (*tag = ber_get_int( op->o_ber, &msgid )) @@ -2084,12 +2096,14 @@ int connection_read_operation(Connection *conn, Operation *op, ber_tag_t *tag, i /* log, close and send error */ LDAPDebug( LDAP_DEBUG_ANY, "conn=%" NSPRIu64 " unable to read tag for incoming request\n", conn->c_connid, 0, 0 ); - disconnect_server( conn, conn->c_connid, -1, SLAPD_DISCONNECT_BAD_BER_TAG, EPROTO ); - return CONN_DONE; + disconnect_server_nomutex( conn, conn->c_connid, -1, SLAPD_DISCONNECT_BAD_BER_TAG, EPROTO ); + ret = CONN_DONE; + goto done; } if(is_ber_too_big(conn,len)) { - disconnect_server( conn, conn->c_connid, -1, SLAPD_DISCONNECT_BER_TOO_BIG, 0 ); - return CONN_DONE; + disconnect_server_nomutex( conn, conn->c_connid, -1, SLAPD_DISCONNECT_BER_TOO_BIG, 0 ); + ret = CONN_DONE; + goto done; } op->o_msgid = msgid; @@ -2100,12 +2114,15 @@ int connection_read_operation(Connection *conn, Operation *op, ber_tag_t *tag, i /* log, close and send error */ LDAPDebug( LDAP_DEBUG_ANY, "conn=%" NSPRIu64 " ber_peek_tag returns 0x%lx\n", conn->c_connid, *tag, 0 ); - disconnect_server( conn, conn->c_connid, -1, SLAPD_DISCONNECT_BER_PEEK, EPROTO ); - return CONN_DONE; + disconnect_server_nomutex( conn, conn->c_connid, -1, SLAPD_DISCONNECT_BER_PEEK, EPROTO ); + ret = CONN_DONE; + goto done; default: break; } op->o_tag = *tag; +done: + PR_Unlock(conn->c_mutex); return ret; } @@ -2267,6 +2284,8 @@ connection_threadmain() int more_data = 0; int replication_connection = 0; /* If this connection is from a replication supplier, we want to ensure that operation processing is serialized */ int doshutdown = 0; + long bypasspollcnt = 0; + long maxconnhits = 0; #if defined( OSF1 ) || defined( hpux ) /* Arrange to ignore SIGPIPE signals. */ @@ -2404,11 +2423,29 @@ connection_threadmain() * they are received off the wire. */ replication_connection = conn->c_isreplication_session; - if ((tag != LDAP_REQ_UNBIND) && !thread_turbo_flag && !more_data && !replication_connection) { - connection_make_readable_nolock(conn); - /* once the connection is readable, another thread may access conn, - * so need locking from here on */ - signal_listner(); + if ((tag != LDAP_REQ_UNBIND) && !thread_turbo_flag && !replication_connection) { + if (!more_data) { + connection_make_readable_nolock(conn); + /* once the connection is readable, another thread may access conn, + * so need locking from here on */ + signal_listner(); + } else { /* more data in conn - just put back on work_q - bypass poll */ + bypasspollcnt++; + PR_Lock(conn->c_mutex); + /* don't do this if it would put us over the max threads per conn */ + if (conn->c_threadnumber < config_get_maxthreadsperconn()) { + /* for turbo, c_idlesince is set above - for !turbo and + * !more_data, we put the conn back in the poll loop and + * c_idlesince is set in handle_pr_read_ready - since we + * are bypassing both of those, we set idlesince here + */ + conn->c_idlesince = curtime; + connection_activity(conn); + } else { + maxconnhits++; + } + PR_Unlock(conn->c_mutex); + } } /* are we in referral-only mode? */ -- 1.7.1