Implement self-contained way of managing the request slab allocator

There's no real reason to use specific free lists.  We want all requests in a particular thread to be allocated in the same list, and passing around free list heads just makes things more complicated.

This seems to deal ok with child requests too.

diff --git a/src/lib/server/request.c b/src/lib/server/request.c
index 11f3e921d284997705538d8980ae57ef448769c2..dc0a63923b6e645b600f1c8d1e17eb5a08588a11 100644 (file)
--- a/src/lib/server/request.c
+++ b/src/lib/server/request.c
@@ -28,23 +28,125 @@ RCSID("$Id$")
 #include <freeradius-devel/server/rad_assert.h>
 #include <freeradius-devel/unlang/base.h>
 
+/** The thread local free list
+ *
+ * Any entries remaining in the list will be freed then the thread is joined
+ */
+fr_thread_local_setup(fr_dlist_head_t *, request_free_list)
+
+/** Setup logging and other fields for a request
+ *
+ * @param[in] request          to (re)-initialise.
+ */
+static void request_init(REQUEST *request)
+{
+#ifndef NDEBUG
+       request->magic = REQUEST_MAGIC;
+#endif
+
+       request->request_state = REQUEST_INIT;
+       request->master_state = REQUEST_ACTIVE;
+
+       /*
+        *      Initialise the stack
+        */
+       MEM(request->stack = unlang_interpret_stack_alloc(request));
+
+       /*
+        *      Initialise the request data list
+        */
+       request_data_list_init(&request->data);
+
+       /*
+        *      Initialise the state_ctx
+        */
+       if (!request->state_ctx) request->state_ctx = talloc_init("session-state");
+
+       /*
+        *      These may be changed later by request_pre_handler
+        */
+       request->log.lvl = fr_debug_lvl;        /* Default to global debug level */
+       if (!request->log.dst) {
+               request->log.dst = talloc_zero(request, log_dst_t);
+       } else {
+               memset(request->log.dst, 0, sizeof(*request->log.dst));
+       }
+       request->log.dst->func = vlog_request;
+       request->log.dst->uctx = &default_log;
+
+       request->module = NULL;
+       request->component = "<core>";
+
+       request->seq_start = 0;
+       request->runnable_id = -1;
+       request->time_order_id = -1;
+
+       fr_dlist_entry_init(&request->free_entry);      /* Needs to be initialised properly, else bad things happen */
+}
+
 /** Callback for freeing a request struct
  *
+ * @param[in] request          to free or return to the free list.
+ * @return
+ *     - 0 in the request was freed.
+ *     - -1 if the request was inserted into the free list.
  */
 static int _request_free(REQUEST *request)
 {
        rad_assert(!request->ev);
 
-#ifndef NDEBUG
-       request->magic = 0x01020304;    /* set the request to be nonsense */
-#endif
-       request->client = NULL;
-#ifdef WITH_PROXY
-       request->proxy = NULL;
-#endif
+       /*
+        *      Reinsert into the free list if it's not already
+        *      in the free list.
+        *
+        *      If it *IS* already in the free list, then free it.
+        */
+       if (unlikely(fr_dlist_entry_in_list(&request->free_entry))) {
+               fr_dlist_entry_unlink(&request->free_entry);    /* Don't trust the list head to be available */
+               talloc_free(request->state_ctx);
+               goto really_free;
+       }
 
        /*
-        *      This is parented separately.
+        *      We keep a buffer of <active> + N requests per
+        *      thread, to avoid spurious allocations.
+        */
+       if (fr_dlist_num_elements(request_free_list) <= 256) {
+               TALLOC_CTX              *state_ctx;
+               fr_dlist_head_t         *free_list;
+
+               /*
+                *      Ensure any data associated
+                *      with the state ctx is freed.
+                */
+               if (request->state_ctx) {
+                       rad_assert(!request->parent || (request->state_ctx != request->parent->state_ctx));
+                       talloc_free_children(request->state_ctx);
+                       state_ctx = request->state_ctx;
+               } else {
+                       state_ctx = NULL;
+               }
+               free_list = request_free_list;
+
+               /*
+                *      Reinitialise the request
+                */
+               talloc_free_children(request);
+               memset(request, 0, sizeof(*request));
+               request->component = "free_list";
+               request->state_ctx = state_ctx;         /* Use the old, now cleared, state_ctx */
+
+               /*
+                *      Reinsert into the free list
+                */
+               fr_dlist_insert_head(free_list, request);
+               request_free_list = free_list;
+
+               return -1;      /* Prevent free */
+       }
+
+       /*
+        *      state_ctx is parented separately.
         *
         *      The reason why it's OK to do this, is if the state attributes
         *      need to persist across requests, they will already have been
@@ -72,52 +174,94 @@ static int _request_free(REQUEST *request)
                talloc_free(request->state_ctx);
        }
 
+       /*
+        *      Ensure anything that might reference the request is
+        *      freed before it is.
+        */
        talloc_free_children(request);
 
+really_free:
+#ifndef NDEBUG
+       request->magic = 0x01020304;    /* set the request to be nonsense */
+#endif
+
        return 0;
 }
 
-/** Create a new REQUEST data structure
+/** Free any free requests when the thread is joined
  *
  */
-REQUEST *request_alloc(TALLOC_CTX *ctx)
+static void _request_free_list_free_on_exit(void *arg)
 {
-       REQUEST *request;
-
-       request = talloc_zero(ctx, REQUEST);
-       if (!request) return NULL;
-       talloc_set_destructor(request, _request_free);
-#ifndef NDEBUG
-       request->magic = REQUEST_MAGIC;
-#endif
-#ifdef WITH_PROXY
-       request->proxy = NULL;
-#endif
-       request->reply = NULL;
-       request->control = NULL;
+       fr_dlist_head_t *list = talloc_get_type_abort(arg, fr_dlist_head_t);
+       REQUEST         *request;
 
        /*
-        *      These may be changed later by request_pre_handler
+        *      See the destructor for why this works
         */
-       request->log.lvl = fr_debug_lvl;        /* Default to global debug level */
-       request->log.dst = talloc_zero(request, log_dst_t);
-       request->log.dst->func = vlog_request;
-       request->log.dst->uctx = &default_log;
-
-       request->module = NULL;
-       request->component = "<core>";
-
-       MEM(request->stack = unlang_interpret_stack_alloc(request));
-
-       request->runnable_id = -1;
-       request->time_order_id = -1;
+       while ((request = fr_dlist_head(list))) talloc_free(request);
+       talloc_free(list);
+}
 
-       request->state_ctx = talloc_init("session-state");
+/** Create a new REQUEST data structure
+ *
+ */
+REQUEST *request_alloc(TALLOC_CTX *ctx)
+{
+       REQUEST                 *request;
+       fr_dlist_head_t         *free_list;
 
        /*
-        *      Initialise the request data list
+        *      Setup the free list, or return the free
+        *      list for this thread.
         */
-       request_data_list_init(&request->data);
+       if (unlikely(!request_free_list)) {
+               MEM(free_list = talloc(NULL, fr_dlist_head_t));
+               fr_dlist_init(free_list, REQUEST, free_entry);
+               fr_thread_local_set_destructor(request_free_list, _request_free_list_free_on_exit, free_list);
+       } else {
+               free_list = request_free_list;
+       }
+
+       request = fr_dlist_head(free_list);
+       if (!request) {
+               /*
+                *      Only allocate requests in the NULL
+                *      ctx.  There's no scenario where it's
+                *      appropriate to allocate them in a
+                *      pool, and using a strict talloc
+                *      hierarchy means that child requests
+                *      cannot be returned to a free list
+                *      and would have to be freed.
+                */
+               MEM(request = talloc_zero_pooled_object(NULL, REQUEST,
+                                                       1 +                             /* Stack pool */
+                                                       UNLANG_STACK_MAX +              /* Stack Frames */
+                                                       3 +                             /* packets */
+                                                       10,                             /* extra */
+                                                       (UNLANG_FRAME_PRE_ALLOC * UNLANG_STACK_MAX) +   /* Stack memory */
+                                                       (sizeof(RADIUS_PACKET) * 2) +   /* packets */
+                                                       128                             /* extra */
+                                                       ));
+               talloc_set_destructor(request, _request_free);
+
+               /*
+                *      Bind lifetime to a parent.
+                *
+                *      If the parent is freed the destructor
+                *      will fire, and return the request
+                *      to a "top level" free list.
+                */
+               if (ctx) talloc_link_ctx(ctx, request);
+       } else {
+               /*
+                *      Remove from the free list, as we're
+                *      about to use it!
+                */
+               fr_dlist_remove(free_list, request);
+       }
+
+       request_init(request);
 
        return request;
 }
@@ -201,7 +345,6 @@ static REQUEST *request_init_fake(REQUEST *request, REQUEST *fake)
        return fake;
 }
 
-
 /*
  *     Create a new REQUEST, based on an old one.
  *
@@ -222,6 +365,7 @@ REQUEST *request_alloc_fake(REQUEST *request, fr_dict_t const *namespace)
        return fake;
 }
 
+
 /** Allocate a fake request which is detachable from the parent.
  * i.e. if the parent goes away, sometimes the child MAY continue to
  * run.

diff --git a/src/lib/server/request.h b/src/lib/server/request.h
index cb5b2c40a37da1baa7c21d481ed3b4e7349dedd6..c23597c76d31e2e4cd911fe0b8ee337d50868069 100644 (file)
--- a/src/lib/server/request.h
+++ b/src/lib/server/request.h
@@ -150,6 +150,8 @@ struct fr_request_s {
        uint32_t                options;        //!< mainly for proxying EAP-MSCHAPv2.
 
        fr_async_t              *async;         //!< for new async listeners
+
+       fr_dlist_t              free_entry;     //!< Request's entry in the free list.
 };                             /* REQUEST typedef */
 
 #ifdef WITH_VERIFY_PTR

diff --git a/src/lib/unlang/base.h b/src/lib/unlang/base.h
index 6b53111458d2dcfc649325cf2c4bf65cbb6cc2bc..d2233fe76663bdb18ee65d7cd7ebedd6c42799f0 100644 (file)
--- a/src/lib/unlang/base.h
+++ b/src/lib/unlang/base.h
@@ -32,6 +32,9 @@
 extern "C" {
 #endif
 
+#define UNLANG_STACK_MAX (64)          //!< The maximum depth of the stack.
+#define UNLANG_FRAME_PRE_ALLOC (128)   //!< How much memory we pre-alloc for each frame.
+
 bool           unlang_section(CONF_SECTION *cs);
 
 void           unlang_register(int type, unlang_op_t *op);

diff --git a/src/lib/unlang/unlang_priv.h b/src/lib/unlang/unlang_priv.h
index 4429b9df54afe49fef90b8c0468f4238ecc8d18e..9e1e7ee955857badd529810e24ee3282117b5042 100644 (file)
--- a/src/lib/unlang/unlang_priv.h
+++ b/src/lib/unlang/unlang_priv.h
@@ -37,8 +37,6 @@
 extern "C" {
 #endif
 
-#define UNLANG_STACK_MAX (64)
-
 /* Actions may be a positive integer (the highest one returned in the group
  * will be returned), or the keyword "return", represented here by
  * MOD_ACTION_RETURN, to cause an immediate return.

diff --git a/src/lib/util/dlist.h b/src/lib/util/dlist.h
index abaaad02827b871a3db4ea782f60e0f483ad7711..b77410879921fd546b3b5ced72e3e7eb784e6feb 100644 (file)
--- a/src/lib/util/dlist.h
+++ b/src/lib/util/dlist.h
@@ -71,6 +71,24 @@ static inline void fr_dlist_entry_unlink(fr_dlist_t *entry)
 {
        entry->prev->next = entry->next;
        entry->next->prev = entry->prev;
+       entry->prev = entry->next = entry;
+}
+
+/** Check if a list entry is part of a list
+ *
+ * This works because the fr_dlist_head_t has an entry in the list.
+ * So if next and prev both point to the entry for the object being
+ * passed in, then it can't be part of a list with a fr_flist_head_t.
+ *
+ * @return
+ *     - True if in a list.
+ *     - False otherwise.
+ */
+static inline bool fr_dlist_entry_in_list(fr_dlist_t *entry)
+{
+       if ((entry->prev == entry) && (entry->next == entry)) return false;
+
+       return true;
 }
 
 /** Initialise the head structure of a doubly linked list