From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 9E4D86EC5D; Mon, 5 Apr 2021 18:52:50 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9E4D86EC5D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617637970; bh=ogkbgHYvmaKWEL1ANkT/+UMxqF6t58S0gZM8eTzsreg=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=ZfapUAqeXh+++Hc3zIIzgkqPG2QO8Ql72ZItVf5/wVRlIMWbPtywDzAKOcAIb44Kv uQEjnRMrVWvau7kAzJx3xqIaphjiufZhlzKQ1T7MW0EuOZoWNzBQcCePCyAahw3R+X Tb6C4+ylavEtasfEAEebVlJNUd8iGkB3zucFk1lA= Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C0A776EC5D for ; Mon, 5 Apr 2021 18:52:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C0A776EC5D Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1lTRWy-0008PD-4k; Mon, 05 Apr 2021 18:52:48 +0300 To: Cyrill Gorcunov , tml References: <20210402123420.885834-1-gorcunov@gmail.com> <20210402123420.885834-5-gorcunov@gmail.com> Message-ID: <14b1c3fa-da30-e7f1-6d17-32e575d71272@tarantool.org> Date: Mon, 5 Apr 2021 17:52:47 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: <20210402123420.885834-5-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9ED7173E37F4E32947287414FD1D04A09E656A5F3377C994A182A05F538085040CC46032C41F282E429C9EA4D2C46B2EA4EB5B09D8F0BE8DD688BA545FDD97302 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7CE4525FFB91B9BBCEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637D19071B5A26B4BDC8638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C16EE06F5A270FE6ACDF61032B7942A7E73945ED8B4B2C583A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE77A825AB47F0FC8649FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C362968DCAA3E4B45B117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637553F28B1F14F2446EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-B7AD71C0: 2623F767319EFA42AC98609FCEE262F9192335DD689A58EBAE0174B7F1092AFBF63472D85867295698F9586B16BE2562 X-C1DE0DAB: 0D63561A33F958A501F89EC4E3ACAC83BB05EEB400FA387C01756116D6C5026ED59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34B7CBFF60649FF2669093EEA1F01E78D4D7B0DA9178110A9E9EF00CC0E2D8418D621B918F5469ECA61D7E09C32AA3244C797EE800C22A8F079DB162B7B0BB5F0E259227199D06760A729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojM00ve/f+0okzfzsxh2gXIA== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822C777BC035E1A21802000A367479AB9E83841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v20 4/7] box/module_cache: introduce modules subsystem X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thanks for the patch! See 6 comments below. > diff --git a/src/box/module_cache.c b/src/box/module_cache.c > new file mode 100644 > index 000000000..2cd2f2e8b > --- /dev/null > +++ b/src/box/module_cache.c > @@ -0,0 +1,474 @@ > +/* > + * SPDX-License-Identifier: BSD-2-Clause > + * > + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file. > + */ > + > +#include > +#include > +#include > +#include > +#include > + > +#include "assoc.h" > +#include "diag.h" > +#include "fiber.h" > +#include "module_cache.h" > + > +#include "box/error.h" > +#include "box/port.h" > + > +#include "lua/utils.h" > +#include "libeio/eio.h" > + > +static struct mh_strnptr_t *module_cache = NULL; > + > +/** > + * Helpers for cache manipulations. > + */ > +static void * 1. It returns struct module, therefore the return type must be 'struct module *', not 'void *'. The same for cache_find() in box.lib implementation. > +cache_find(const char *str, size_t len) > +{ > + mh_int_t e = mh_strnptr_find_inp(module_cache, str, len); > + if (e == mh_end(module_cache)) > + return NULL; > + return mh_strnptr_node(module_cache, e)->val; > +} > + > +static void > +cache_update(struct module *m) > +{ > + const char *str = m->package; > + size_t len = m->package_len; > + > + mh_int_t e = mh_strnptr_find_inp(module_cache, str, len); > + if (e == mh_end(module_cache)) > + panic("module: failed to update cache: %s", str); > + > + mh_strnptr_node(module_cache, e)->str = m->package; > + mh_strnptr_node(module_cache, e)->val = m; > +} > + > +static int > +cache_put(struct module *m) > +{ > + const struct mh_strnptr_node_t nd = { > + .str = m->package, > + .len = m->package_len, > + .hash = mh_strn_hash(m->package, m->package_len), > + .val = m, > + }; > + > + mh_int_t e = mh_strnptr_put(module_cache, &nd, NULL, NULL); 2. Put() silently replaces the old value if it is present. I would recommend to use the next to the last argument to get the old value and ensure it is mh_end() via an assertion/panic. The same for the other new put() functions in the other commits. > + if (e == mh_end(module_cache)) { > + diag_set(OutOfMemory, sizeof(nd), "malloc", > + "module_cache node"); > + return -1; > + } > + return 0; > +} > + > +static void > +cache_del(struct module *m) > +{ > + const char *str = m->package; > + size_t len = m->package_len; > + > + mh_int_t e = mh_strnptr_find_inp(module_cache, str, len); > + if (e != mh_end(module_cache)) { 3. Maybe this must be an assertion/panic. I don't see a valid case when del() is called on an already deleted module. The same for the other new del() functions in the other commits. > + struct module *v = mh_strnptr_node(module_cache, e)->val; > + if (v == m) { > + /* > + * The module in cache might be updated > + * via force load and old instance is kept > + * by a reference only. > + */ > + mh_strnptr_del(module_cache, e, NULL); > + } > + } > +} <...> > + > +struct module * > +module_load(const char *package, size_t package_len) > +{ > + char path[PATH_MAX]; > + > + if (find_package(package, package_len, path, sizeof(path)) != 0) > + return NULL; > + > + struct module *m = cache_find(package, package_len); > + if (m != NULL) { > + struct module_attr attr; > + struct stat st; > + if (stat(path, &st) != 0) { > + diag_set(SystemError, "failed to stat() %s", path); > + return NULL; > + } > + > + /* > + * In case of cache hit we may reuse existing > + * module which speedup load procedure. > + */ > + module_attr_fill(&attr, &st); > + if (memcmp(&attr, &m->attr, sizeof(attr)) == 0) { 4. Please, add a static assertion, that sizeof(module_attr) == 40. Otherwise somebody might add a new attribute, which won't be uint64_t, and would break the comparison without noticing. Also you can make the attributes be stored as a byte array char[40] to make it impossible to add any padding into it. Also you can compare the attributes one by one. > + module_ref(m); > + return m; > + } > + > + /* > + * Module has been updated on a storage device, > + * so load a new instance and update the cache, > + * old entry get evicted but continue residing > + * in memory, fully functional, until last > + * function is unloaded. > + */ > + m = module_new(package, package_len, path); > + if (m != NULL) > + cache_update(m); > + } else { > + m = module_new(package, package_len, path); > + if (m != NULL && cache_put(m) != 0) { > + module_unload(m); > + return NULL; > + } > + } > + > + return m; > +} > + > +void > +module_unload(struct module *m) > +{ > + module_unref(m); > +} > + > +void > +module_free(void) > +{ > + mh_int_t e; > + > + mh_foreach(module_cache, e) { > + struct module *m = mh_strnptr_node(module_cache, e)->val; > + module_unload(m); 5. As I said in the previous review, it does not make much sense. If there are any not unloaded modules, and they try to unload later, they will see module_cache == NULL and will crash. Also you can't do unload here, because the module_cache itself does not keep any references. All the unloads must be done by the module objects owners. Not by module_cache on its own. For example, if there is a module having a single reference and used in some other subsystem, your unload will free it and make it memory invalid. That will crash in case the module owner will try to access it again. There should be a panic-check that the module cache is empty already. > + } > + > + mh_strnptr_delete(module_cache); > + module_cache = NULL; > +} > diff --git a/src/box/module_cache.h b/src/box/module_cache.h > new file mode 100644 > index 000000000..18eb3866a > --- /dev/null > +++ b/src/box/module_cache.h > @@ -0,0 +1,208 @@ > +/* > + * SPDX-License-Identifier: BSD-2-Clause > + * > + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file. > + */ > + > +#pragma once > + > +#include > + > +#include > +#include 6. You don't need these headers in module_cache.h. They are needed only in the .c file.