From f74f67540fb44d1ba1286278220635773c25f309 Mon Sep 17 00:00:00 2001 From: Mark Riddoch Date: Wed, 4 Sep 2013 12:24:59 +0200 Subject: [PATCH] Added spinlock protection for the session association to the DCB and reworked the reference count mechanism on the session. Introduced a FREE state for the session and alter the session destruction flow so that we only remove the session when all the DCB's have singled they have finished processing events for the DCB rather than when the first thread decides to clsoe the DCB. --- server/core/dcb.c | 52 +++++++++------------- server/core/session.c | 60 ++++++++++++++++++++++---- server/include/dcb.h | 2 +- server/include/session.h | 4 +- server/modules/protocol/mysql_client.c | 1 - 5 files changed, 76 insertions(+), 43 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 9a86f646f..cde3d9c1a 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -212,6 +212,19 @@ dcb_final_free(DCB *dcb) } spinlock_release(&dcbspin); + if (dcb->session) { + SESSION *local_session = dcb->session; + if (dcb_isclient(dcb)) + dcb->session->client = NULL; + dcb->session = NULL; + session_free(local_session); + skygw_log_write_flush( + LOGFILE_TRACE, + "%lu [dcb_final_free] DCB %p freed session %p", + pthread_self(), + dcb, + local_session); + } if (dcb->protocol) free(dcb->protocol); if (dcb->data) @@ -293,14 +306,13 @@ DCB *ptr, *lptr; * @param server The server to connect to * @param session The session this connection is being made for * @param protocol The protocol module to use - * @return The new allocated dcb + * @return The new allocated dcb or NULL if the DCB was not connected */ DCB * dcb_connect(SERVER *server, SESSION *session, const char *protocol) { DCB *dcb; GWPROTOCOL *funcs; -int val; if ((dcb = dcb_alloc()) == NULL) { @@ -315,24 +327,14 @@ int val; } memcpy(&(dcb->func), funcs, sizeof(GWPROTOCOL)); - /* Adding now the session refcount increase for the backend dcb. - * This operation is protected by the authspinlock. - * This spinlock could be used later in backend - * authentication. - */ - spinlock_acquire(&dcb->authlock); - val = atomic_add(&session->refcount, 1); - dcb->session = session; - spinlock_release(&dcb->authlock); + if (!session_link_dcb(session, dcb)) + { + skygw_log_write(LOGFILE_TRACE, + "dcb_connect: failed to link to session, the session has been removed."); + dcb_final_free(dcb); + return NULL; + } - skygw_log_write( - LOGFILE_TRACE, - "%lu [dcb_connect] Increased DCB %p session %p refcount to %d.", - pthread_self(), - dcb, - dcb->session, - val+1); - if ((dcb->fd = dcb->func.connect(dcb, server, session)) == -1) { dcb_final_free(dcb); @@ -670,18 +672,6 @@ dcb_close(DCB *dcb) pthread_self()); } } - dcb->session->client = NULL; - } - if (dcb->session) { - SESSION *local_session = dcb->session; - dcb->session = NULL; - session_free(local_session); - skygw_log_write_flush( - LOGFILE_TRACE, - "%lu [dcb_close] DCB %p freed session %p", - pthread_self(), - dcb, - local_session); } dcb_free(dcb); } diff --git a/server/core/session.c b/server/core/session.c index 2518369d0..83a036c86 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -78,7 +78,6 @@ session_alloc(SERVICE *service, DCB *client) return NULL; } - session->refcount = 0; session->ses_chk_top = CHK_NUM_SESSION; session->ses_chk_tail = CHK_NUM_SESSION; @@ -94,15 +93,26 @@ session_alloc(SERVICE *service, DCB *client) session->stats.connect = time(0); session->state = SESSION_STATE_ALLOC; /** - * If client has data pointer to authentication info, set it to session. + * Associate the session to the client DCB and set the reference count on + * the session to indicate that there is a single reference to the session. + * There is no need to protect this or use atomic add as the session has not + * been made available to the other threads at this point. */ session->data = client->data; client->session = session; + session->refcount = 1; + /** Release session lock */ + spinlock_release(&session->ses_lock); + /* * Only create a router session if we are not the listening * DCB. Creating a router session may create a connection to a * backend server, depending upon the router module implementation * and should be avoided for the listener session + * + * Router session creation may create other DCBs that link to the + * session, therefore it is important that the session lock is relinquished + * beforethe router call. */ if (client->state != DCB_STATE_LISTENING) { @@ -116,8 +126,6 @@ session_alloc(SERVICE *service, DCB *client) /** This indicates that session is ready to be shared with backend DCBs. */ session->state = SESSION_STATE_READY; - /** Release session lock */ - spinlock_release(&session->ses_lock); atomic_add(&service->stats.n_sessions, 1); atomic_add(&service->stats.n_current, 1); @@ -125,6 +133,28 @@ session_alloc(SERVICE *service, DCB *client) return session; } +/** + * Link a session to a DCB. + * + * @param session The session to link with the dcb + * @param dcb The DCB to be linked + * @return True if the session was sucessfully linked to the DCB + */ +bool +session_link_dcb(SESSION *session, DCB *dcb) +{ + spinlock_acquire(&session->ses_lock); + if (session->state == SESSION_STATE_FREE) + { + spinlock_release(&session->ses_lock); + return false; + } + atomic_add(&session->refcount, 1); + dcb->session = session; + spinlock_release(&session->ses_lock); + return true; +} + /** * Deallocate the specified session * @@ -134,9 +164,24 @@ void session_free(SESSION *session) { SESSION *ptr; + CHK_SESSION(session); + + spinlock_acquire(&session->ses_lock); + if (atomic_add(&session->refcount, -1) > 1) + { + /* + * There are still other references to the session + * so we simply return after decrementing the session + * count. + */ + spinlock_release(&session->ses_lock); + return; + } + session->state = SESSION_STATE_FREE; + spinlock_release(&session->ses_lock); + /* First of all remove from the linked list */ - spinlock_acquire(&session_spin); if (allSessions == session) { @@ -156,10 +201,7 @@ SESSION *ptr; atomic_add(&session->service->stats.n_current, -1); /* Clean up session and free the memory */ - if (atomic_add(&session->refcount, -1) == 1) - { - free(session); - } + free(session); } /** diff --git a/server/include/dcb.h b/server/include/dcb.h index 6c973d8d4..46e7e18e8 100644 --- a/server/include/dcb.h +++ b/server/include/dcb.h @@ -157,7 +157,7 @@ typedef struct dcb { char *remote; /**< Address of remote end */ void *protocol; /**< The protocol specific state */ struct session *session; /**< The owning session */ - GWPROTOCOL func; /**< The functions for this descrioptor */ + GWPROTOCOL func; /**< The functions for this descriptor */ SPINLOCK writeqlock; /**< Write Queue spinlock */ GWBUF *writeq; /**< Write Data Queue */ diff --git a/server/include/session.h b/server/include/session.h index f0224075b..cee13b683 100644 --- a/server/include/session.h +++ b/server/include/session.h @@ -53,7 +53,8 @@ typedef enum { SESSION_STATE_ALLOC, SESSION_STATE_READY, SESSION_STATE_LISTENER, - SESSION_STATE_LISTENER_STOPPED + SESSION_STATE_LISTENER_STOPPED, + SESSION_STATE_FREE } session_state_t; /** @@ -87,4 +88,5 @@ extern void printSession(SESSION *); extern void dprintAllSessions(struct dcb *); extern void dprintSession(struct dcb *, SESSION *); extern char *session_state(int); +extern bool session_link_dcb(SESSION *, struct dcb *); #endif diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index b2ae7e020..3fecce9d4 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -588,7 +588,6 @@ int gw_read_client_event(DCB* dcb) { //write to client mysql AUTH_OK packet, packet n. is 2 // start a new session, and connect to backends session = session_alloc(dcb->service, dcb); - atomic_add(&dcb->session->refcount, 1); CHK_SESSION(session); ss_dassert(session->state != SESSION_STATE_ALLOC); protocol->state = MYSQL_IDLE;