From: Arran Cudbard-Bell Date: Tue, 10 Dec 2019 06:45:40 +0000 (+0700) Subject: Implement self-contained way of managing the request slab allocator X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bac4cb153b5b19fb9d002e0fc2bc0e6d14fd651e;p=thirdparty%2Ffreeradius-server.git 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 11f3e921d28..dc0a63923b6 100644 --- a/src/lib/server/request.c +++ b/src/lib/server/request.c @@ -28,23 +28,125 @@ RCSID("$Id$") #include #include +/** 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 = ""; + + 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 + 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 = ""; - - 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 cb5b2c40a37..c23597c76d3 100644 --- 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 6b53111458d..d2233fe7666 100644 --- 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 4429b9df54a..9e1e7ee9558 100644 --- 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 abaaad02827..b7741087992 100644 --- 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