move the cleanups of request_done to a free list

so that we can call request_done() with the thread mutex unlocked

Author: alandekok

src/include/dlist.h

@@ -45,6 +45,11 @@ static inline void fr_dlist_entry_init(fr_dlist_t *entry)
 	entry->prev = entry->next = entry;
 }
 
+static inline bool fr_dlist_empty(fr_dlist_t *head)
+{
+	return ((head->prev == head) && (head->next == head));
+}
+
 static inline CC_HINT(nonnull) void fr_dlist_entry_unlink(fr_dlist_t *entry)
 {
 	entry->prev->next = entry->next;
@@ -60,4 +65,14 @@ static inline CC_HINT(nonnull) void fr_dlist_insert_tail(fr_dlist_t *head, fr_dl
 	head->prev = entry;
 }
 
+static inline CC_HINT(nonnull) fr_dlist_t *fr_dlist_pop_head(fr_dlist_t *head)
+{
+	fr_dlist_t *entry = head->next;
+
+	if (entry == head) return NULL;
+
+	fr_dlist_entry_unlink(entry);
+	return entry;
+}
+
 #endif	/* RADIUS_DLIST_H */

src/include/radiusd.h

@@ -32,6 +32,7 @@ RCSIDH(radiusd_h, "$Id$")
 #include <freeradius-devel/conffile.h>
 #include <freeradius-devel/event.h>
 #include <freeradius-devel/connection.h>
+#include <freeradius-devel/dlist.h>
 
 typedef struct rad_request REQUEST;
 
@@ -321,6 +322,7 @@ struct rad_request {
 	uint32_t		num_coa_requests;//!< Counter for number of requests sent including
 						//!< retransmits.
 #endif
+	fr_dlist_t		entry;		//!< for the free list
 };				/* REQUEST typedef */
 
 #define RAD_REQUEST_LVL_NONE	(0)		//!< No debug messages should be printed.

src/main/threads.c

@@ -505,7 +505,7 @@ int request_enqueue(REQUEST *request)
 
 	/*
 	 *	If there are too many packets _overall_, OR the
-	 *	destinatio fifo is full, then try to delete a lower
+	 *	destination fifo is full, then try to delete a lower
 	 *	priority one.
 	 */
 	if (((thread_pool.num_queued >= thread_pool.max_queue_size) &&
@@ -547,24 +547,73 @@ int request_enqueue(REQUEST *request)
 	return 1;
 }
 
+/*
+ *	Free a list of requests, WITHOUT holding the thread mutex.
+ */
+static void request_list_free(fr_dlist_t *head)
+{
+	fr_dlist_t *entry;
+
+	while ((entry = fr_dlist_pop_head(head)) != NULL) {
+		REQUEST *request;
+
+		request = (REQUEST *) (((uint8_t *) entry) - offsetof(REQUEST, entry));
+		rad_assert(request->magic == REQUEST_MAGIC);
+
+		/*
+		 *	This entry was marked to be stopped.  Acknowledge it.
+		 *
+		 *	If we own the request, we delete it. Otherwise
+		 *	we run the "done" callback now, which will
+		 *	stop timers, remove it from the request hash,
+		 *	update listener counts, etc.
+		 *
+		 *	Running request_done() here means that old
+		 *	requests are cleaned up immediately, which
+		 *	frees up more resources for new requests.  It
+		 *	also means that we don't need to rely on
+		 *	timers to free up the old requests, as those
+		 *	timers will run much much later.
+		 *
+		 *	Catching this corner case doesn't change the
+		 *	normal operation of the server.  Most requests
+		 *	should NOT be marked "stop processing" when
+		 *	they're in the queue.  This situation
+		 *	generally happens when the server is blocked,
+		 *	due to a slow back-end database.
+		 */
+		request->child_state = REQUEST_DONE;
+		if (request->master_state == REQUEST_TO_FREE) {
+			request_free(request);
+		} else {
+			request_done(request, REQUEST_DONE);
+		}
+	}
+}
+
 #ifndef HAVE_STDATOMIC_H
 /*
  *	Try to free up requests by discarding requests of lower priority.
  */
 static int request_fifo_discard(int priority, bool lower, time_t now)
 {
-	int i, rcode;
+	int i, rcode = 0;
 	REQUEST *request;
+	fr_dlist_t free_list;
+
+	fr_dlist_entry_init(&free_list);
 
 	if (lower) {
 		for (i = NUM_FIFOS - 1; i < priority; i--) {
 			request = fr_fifo_pop(thread_pool.fifo[i]);
 			if (!request) continue;
 
 			fr_assert(request->child_state == REQUEST_QUEUED);
-			request->child_state = REQUEST_DONE;
-			request_done(request, FR_ACTION_DONE);
-			return 1;
+
+			request->master_state = REQUEST_STOP_PROCESSING;
+			fr_dlist_insert_tail(&free_list, &request->entry);
+			rcode = 1;
+			break;
 		}
 
 		/*
@@ -576,47 +625,48 @@ static int request_fifo_discard(int priority, bool lower, time_t now)
 	}
 
 	/*
-	 *	The time stamp for proxied requests may be 5-10
-	 *	seconds in the past, because the home server hasn't
-	 *	responded.  We therefore can't discard "old" requests,
-	 *	as they have just received a reply, or they have just
-	 *	timed out.
+	 *	We didn't free a lower priority one, try to free one from the current priority.
+	 *
+	 *	We don't free proxied requests based on time, as the time stamp for proxied requests may be
+	 *	5-10 seconds in the past, because the home server hasn't responded.  We therefore can't
+	 *	discard "old" requests, as they have just received a reply, or they have just timed out.
 	 */
-	if (priority <= RAD_LISTEN_PROXY) return 0;
+	if (!rcode && (priority > RAD_LISTEN_PROXY)) {
+		while (true) {
+			request = fr_fifo_peek(thread_pool.fifo[priority]);
+			if (!request) break;
 
-	/*
-	 *	Peek at the first entry in the fifo.  Note that there
-	 *	is not always a first entry.  This is because we're
-	 *	called if there are too many _total_ requests.
-	 */
-	rcode = 0;
 
-retry:
-	request = fr_fifo_peek(thread_pool.fifo[priority]);
-	if (!request) return rcode;
+			/*
+			 *	This request expires in the future.  We can't do anything.
+			 */
+			if ((request->timestamp + main_config.max_request_time) > now) break;
 
-	/*
-	 *	This request expires in the future.  We can't do anything.
-	 */
-	if ((request->timestamp + main_config.max_request_time) > now) return rcode;
+			request = fr_fifo_pop(thread_pool.fifo[priority]);
+			rad_assert(request != NULL);
+			VERIFY_REQUEST(request);
 
-	request = fr_fifo_pop(thread_pool.fifo[priority]);
-	rad_assert(request != NULL);
-	VERIFY_REQUEST(request);
+			request->master_state = REQUEST_STOP_PROCESSING;
+			fr_dlist_insert_tail(&free_list, &request->entry);
+			thread_pool.num_queued--;
 
-	request->child_state = REQUEST_DONE;
-	if (request->master_state == REQUEST_TO_FREE) {
-		request_free(request);
-	} else {
-		request_done(request, REQUEST_DONE);
+			/*
+			 *	We might as well delete as many old requests as possible.
+			 */
+			rcode = 1;
+		}
 	}
-	thread_pool.num_queued--;
 
 	/*
-	 *	We might as well delete as many old requests as possible.
+	 *	request_done() grabs a mutex.  So we unlock this one, to avoid nested mutexes.
 	 */
-	rcode = 1;
-	goto retry;
+	if (!fr_dlist_empty(&free_list)) {
+		pthread_mutex_unlock(&thread_pool.queue_mutex);
+		request_list_free(&free_list);
+		pthread_mutex_lock(&thread_pool.queue_mutex);
+	}
+
+	return rcode;
 }
 #endif
 
@@ -634,12 +684,15 @@ static int request_dequeue(REQUEST **prequest)
 #endif
 	RAD_LISTEN_TYPE i;
 	REQUEST *request = NULL;
+	fr_dlist_t	free_list;
+
+	fr_dlist_entry_init(&free_list);
+
 	reap_children();
 
 	rad_assert(pool_initialized == true);
 
 #ifdef HAVE_STDATOMIC_H
-retry:
 	for (i = 0; i < NUM_FIFOS; i++) {
 		if (!fr_atomic_queue_pop(thread_pool.queue[i], (void **) &request)) continue;
 
@@ -651,42 +704,9 @@ static int request_dequeue(REQUEST **prequest)
 			break;
 		}
 
-		/*
-		 *	This entry was marked to be stopped.  Acknowledge it.
-		 *
-		 *	If we own the request, we delete it. Otherwise
-		 *	we run the "done" callback now, which will
-		 *	stop timers, remove it from the request hash,
-		 *	update listener counts, etc.
-		 *
-		 *	Running request_done() here means that old
-		 *	requests are cleaned up immediately, which
-		 *	frees up more resources for new requests.  It
-		 *	also means that we don't need to rely on
-		 *	timers to free up the old requests, as those
-		 *	timers will run much much later.
-		 *
-		 *	Catching this corner case doesn't change the
-		 *	normal operation of the server.  Most requests
-		 *	should NOT be marked "stop processing" when
-		 *	they're in the queue.  This situation
-		 *	generally happens when the server is blocked,
-		 *	due to a slow back-end database.
-		 */
-		request->child_state = REQUEST_DONE;
-		if (request->master_state == REQUEST_TO_FREE) {
-			request_free(request);
-		} else {
-			request_done(request, REQUEST_DONE);
-		}
+		fr_dlist_insert_tail(&free_list, &request->entry);
 		request = NULL;
 	}
-
-	/*
-	 *	Popping might fail.  If so, return.
-	 */
-	if (!request) return 0;
-
 #else
 	pthread_mutex_lock(&thread_pool.queue_mutex);
 
@@ -735,58 +755,59 @@ static int request_dequeue(REQUEST **prequest)
 		rad_assert(request != NULL);
 		VERIFY_REQUEST(request);
 
-		request->child_state = REQUEST_DONE;
-		if (request->master_state == REQUEST_TO_FREE) {
-			request_free(request);
-		} else {
-			request_done(request, REQUEST_DONE);
-		}
+		fr_dlist_insert_tail(&free_list, &request->entry);
 		thread_pool.num_queued--;
+		request = NULL;
 	}
 
 	start = 0;
- retry:
+
 	/*
-	 *	Pop results from the top of the queue
+	 *	Pop the first request by priority.
 	 */
 	for (i = start; i < NUM_FIFOS; i++) {
 		request = fr_fifo_pop(thread_pool.fifo[i]);
-		if (request) {
-			VERIFY_REQUEST(request);
-			start = i;
-			break;
+		if (!request) continue;
+
+		VERIFY_REQUEST(request);
+
+		rad_assert(thread_pool.num_queued > 0);
+		thread_pool.num_queued--;
+
+		if (request->master_state == REQUEST_STOP_PROCESSING) {
+			fr_dlist_insert_tail(&free_list, &request->entry);
+			request = NULL;
+			continue;
 		}
-	}
 
-	if (!request) {
-		pthread_mutex_unlock(&thread_pool.queue_mutex);
-		*prequest = NULL;
-		return 0;
+		start = i;
+		break;
 	}
 
-	rad_assert(thread_pool.num_queued > 0);
-	thread_pool.num_queued--;
+	if (request) thread_pool.active_threads++;
+
+	pthread_mutex_unlock(&thread_pool.queue_mutex);
 #endif	/* HAVE_STD_ATOMIC_H */
 
 	*prequest = request;
 
-	rad_assert(*prequest != NULL);
-	rad_assert(request->magic == REQUEST_MAGIC);
+	/*
+	 *	Clean up the free list, with all of the mutexes unlocked.  request_done() will grab another
+	 *	mutex to do its work, and we don't want to have nested mutexes.
+	 */
+	if (!fr_dlist_empty(&free_list)) {
+		request_list_free(&free_list);
+	}
 
 	/*
-	 *	If the request has sat in the queue for too long,
-	 *	kill it.
-	 *
-	 *	The main clean-up code can't delete the request from
-	 *	the queue, and therefore won't clean it up until we
-	 *	have acknowledged it as "done".
+	 *	We didn't find a live request in the list.  Return that.
 	 */
-	if (request->master_state == REQUEST_STOP_PROCESSING) {
-		request->module = "<done>";
-		request->child_state = REQUEST_DONE;
-		goto retry;
+	if (!request) {
+		return 0;
 	}
 
+	rad_assert(*prequest != NULL);
+	rad_assert(request->magic == REQUEST_MAGIC);
 	rad_assert(request->child_state == REQUEST_QUEUED);
 
 	request->component = "<core>";
@@ -798,8 +819,6 @@ static int request_dequeue(REQUEST **prequest)
 	 */
 #ifdef HAVE_STDATOMIC_H
 	CAS_INCR(thread_pool.active_threads);
-#else
-	thread_pool.active_threads++;
 #endif
 
 	blocked = time(NULL);
@@ -817,10 +836,6 @@ static int request_dequeue(REQUEST **prequest)
 		blocked = 0;
 	}
 
-#ifndef HAVE_STDATOMIC_H
-	pthread_mutex_unlock(&thread_pool.queue_mutex);
-#endif
-
 	if (blocked) {
 		ERROR("%d requests have been waiting in the processing queue for %d seconds.  Check that all databases are running properly!",
 		      num_blocked, (int) blocked);