From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f52.google.com (mail-lf1-f52.google.com [209.85.167.52]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 2C4EB469719 for ; Thu, 5 Nov 2020 18:18:28 +0300 (MSK) Received: by mail-lf1-f52.google.com with SMTP id 126so2820619lfi.8 for ; Thu, 05 Nov 2020 07:18:28 -0800 (PST) From: Cyrill Gorcunov Date: Thu, 5 Nov 2020 18:18:05 +0300 Message-Id: <20201105151808.456573-2-gorcunov@gmail.com> In-Reply-To: <20201105151808.456573-1-gorcunov@gmail.com> References: <20201105151808.456573-1-gorcunov@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH v10 1/4] box/func: factor out c function entry structure List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tml Cc: Vladislav Shpilevoy Currently func_c structure is a wrapper over struct func which in turn handles function definition, credentials and etc, mostly to handle internal "_func" space. Such tight bound doesn't allow to reuse func_c in any other context. But we're going to support C function execution in read-only instances and for this we better reuse already existing structures as much as possible instead of breeding new ones. Thus lets extract module symbols into module_sym structure, this allows us to reuse module path cache in other code. While extracting the structure rename "func" member to "addr" since this exactly what the member represents: an address of symbol in memory. Same time to be completely independent of "_func" specific lets carry symbolic name inplace (ie member "name" is kind of redundant when module_sym is used for "_func" handling where we can retrieve the name via function definition but such definition won't be available for non-persistent C functions which we will support in next patches). The new structure allows us to load/unload general symbols via module_sym_load() and module_sym_unload() helpers. While been factoring out Vlad reported that we've an issue: if a module has been loaded for a first time but doesn't have symbol requested it will remain in memory keeping a reference to a shared library for nothing. Fixes #5475 Part-of #4642 Reported-by: Vladislav Shpilevoy Signed-off-by: Cyrill Gorcunov --- src/box/func.c | 172 +++++++++++++++++++++++++------------------------ src/box/func.h | 42 ++++++++++++ 2 files changed, 131 insertions(+), 83 deletions(-) diff --git a/src/box/func.c b/src/box/func.c index 8087c953f..36bfca33f 100644 --- a/src/box/func.c +++ b/src/box/func.c @@ -59,19 +59,8 @@ struct func_name { struct func_c { /** Function object base class. */ struct func base; - /** - * Anchor for module membership. - */ - struct rlist item; - /** - * For C functions, the body of the function. - */ - box_function_f func; - /** - * Each stored function keeps a handle to the - * dynamic library for the C callback. - */ - struct module *module; + /** C function module symbol. */ + struct module_sym mod_sym; }; /*** @@ -371,6 +360,70 @@ module_sym(struct module *module, const char *name) return f; } +int +module_sym_load(struct module_sym *mod_sym) +{ + assert(mod_sym->addr == NULL); + + struct func_name name; + func_split_name(mod_sym->name, &name); + + /* + * In case if module has been loaded already by + * some previous call we can eliminate redundant + * loading and take it from the cache. + */ + struct module *cached, *module; + cached = module_cache_find(name.package, name.package_end); + if (cached == NULL) { + module = module_load(name.package, name.package_end); + if (module == NULL) + return -1; + if (module_cache_put(module) != 0) { + module_delete(module); + return -1; + } + } else { + module = cached; + } + + mod_sym->addr = module_sym(module, name.sym); + if (mod_sym->addr == NULL) { + if (cached == NULL) { + /* + * In case if it was a first load we + * should clean the cache immediately otherwise + * the module may continue being referenced even + * if noone gonna use it. + */ + module_cache_del(name.package, name.package_end); + module_delete(module); + } + return -1; + } + mod_sym->module = module; + rlist_add(&module->funcs, &mod_sym->item); + return 0; +} + +void +module_sym_unload(struct module_sym *mod_sym) +{ + if (mod_sym->module == NULL) + return; + + rlist_del(&mod_sym->item); + if (rlist_empty(&mod_sym->module->funcs)) { + struct func_name name; + func_split_name(mod_sym->name, &name); + module_cache_del(name.package, name.package_end); + } + module_gc(mod_sym->module); + + mod_sym->module = NULL; + mod_sym->addr = NULL; +} + int module_reload(const char *package, const char *package_end, struct module **module) { @@ -385,15 +438,15 @@ module_reload(const char *package, const char *package_end, struct module **modu if (new_module == NULL) return -1; - struct func_c *func, *tmp_func; - rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) { + struct module_sym *mod_sym, *tmp; + rlist_foreach_entry_safe(mod_sym, &old_module->funcs, item, tmp) { struct func_name name; - func_split_name(func->base.def->name, &name); - func->func = module_sym(new_module, name.sym); - if (func->func == NULL) + func_split_name(mod_sym->name, &name); + mod_sym->addr = module_sym(new_module, name.sym); + if (mod_sym->addr == NULL) goto restore; - func->module = new_module; - rlist_move(&new_module->funcs, &func->item); + mod_sym->module = new_module; + rlist_move(&new_module->funcs, &mod_sym->item); } module_cache_del(package, package_end); if (module_cache_put(new_module) != 0) @@ -408,9 +461,9 @@ module_reload(const char *package, const char *package_end, struct module **modu */ do { struct func_name name; - func_split_name(func->base.def->name, &name); - func->func = module_sym(old_module, name.sym); - if (func->func == NULL) { + func_split_name(mod_sym->name, &name); + mod_sym->addr = module_sym(old_module, name.sym); + if (mod_sym->addr == NULL) { /* * Something strange was happen, an early loaden * function was not found in an old dso. @@ -418,10 +471,11 @@ module_reload(const char *package, const char *package_end, struct module **modu panic("Can't restore module function, " "server state is inconsistent"); } - func->module = old_module; - rlist_move(&old_module->funcs, &func->item); - } while (func != rlist_first_entry(&old_module->funcs, - struct func_c, item)); + mod_sym->module = old_module; + rlist_move(&old_module->funcs, &mod_sym->item); + } while (mod_sym != rlist_first_entry(&old_module->funcs, + struct module_sym, + item)); assert(rlist_empty(&new_module->funcs)); module_delete(new_module); return -1; @@ -484,79 +538,31 @@ func_c_new(MAYBE_UNUSED struct func_def *def) return NULL; } func->base.vtab = &func_c_vtab; - func->func = NULL; - func->module = NULL; + func->mod_sym.addr = NULL; + func->mod_sym.module = NULL; + func->mod_sym.name = def->name; return &func->base; } -static void -func_c_unload(struct func_c *func) -{ - if (func->module) { - rlist_del(&func->item); - if (rlist_empty(&func->module->funcs)) { - struct func_name name; - func_split_name(func->base.def->name, &name); - module_cache_del(name.package, name.package_end); - } - module_gc(func->module); - } - func->module = NULL; - func->func = NULL; -} - static void func_c_destroy(struct func *base) { assert(base->vtab == &func_c_vtab); assert(base != NULL && base->def->language == FUNC_LANGUAGE_C); struct func_c *func = (struct func_c *) base; - func_c_unload(func); + module_sym_unload(&func->mod_sym); TRASH(base); free(func); } -/** - * Resolve func->func (find the respective DLL and fetch the - * symbol from it). - */ -static int -func_c_load(struct func_c *func) -{ - assert(func->func == NULL); - - struct func_name name; - func_split_name(func->base.def->name, &name); - - struct module *module = module_cache_find(name.package, - name.package_end); - if (module == NULL) { - /* Try to find loaded module in the cache */ - module = module_load(name.package, name.package_end); - if (module == NULL) - return -1; - if (module_cache_put(module)) { - module_delete(module); - return -1; - } - } - - func->func = module_sym(module, name.sym); - if (func->func == NULL) - return -1; - func->module = module; - rlist_add(&module->funcs, &func->item); - return 0; -} - int func_c_call(struct func *base, struct port *args, struct port *ret) { assert(base->vtab == &func_c_vtab); assert(base != NULL && base->def->language == FUNC_LANGUAGE_C); struct func_c *func = (struct func_c *) base; - if (func->func == NULL) { - if (func_c_load(func) != 0) + if (func->mod_sym.addr == NULL) { + if (module_sym_load(&func->mod_sym) != 0) return -1; } @@ -571,10 +577,10 @@ func_c_call(struct func *base, struct port *args, struct port *ret) box_function_ctx_t ctx = { ret }; /* Module can be changed after function reload. */ - struct module *module = func->module; + struct module *module = func->mod_sym.module; assert(module != NULL); ++module->calls; - int rc = func->func(&ctx, data, data + data_sz); + int rc = func->mod_sym.addr(&ctx, data, data + data_sz); --module->calls; module_gc(module); region_truncate(region, region_svp); diff --git a/src/box/func.h b/src/box/func.h index 581e468cb..9a7f17446 100644 --- a/src/box/func.h +++ b/src/box/func.h @@ -58,6 +58,28 @@ struct module { char package[0]; }; +/** + * Callable symbol bound to a module. + */ +struct module_sym { + /** + * Anchor for module membership. + */ + struct rlist item; + /** + * For C functions, address of the function. + */ + box_function_f addr; + /** + * A module the symbol belongs to. + */ + struct module *module; + /** + * Function name definition. + */ + char *name; +}; + /** Virtual method table for func object. */ struct func_vtab { /** Call function with given arguments. */ @@ -108,6 +130,26 @@ func_delete(struct func *func); int func_call(struct func *func, struct port *args, struct port *ret); +/** + * Resolve C entry (find the respective DLL and fetch the + * symbol from it). + * + * @param mod_sym module symbol pointer. + * @retval -1 on error. + * @retval 0 on success. + */ +int +module_sym_load(struct module_sym *mod_sym); + +/** + * Unload module symbol and drop it from the package + * cache if there is no users left. + * + * @param mod_sym module symbol pointer. + */ +void +module_sym_unload(struct module_sym *mod_sym); + /** * Reload dynamically loadable module. * -- 2.26.2