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 B33616EC5D; Tue, 6 Apr 2021 17:33:26 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B33616EC5D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617719606; bh=vva9uIB+md7SupurAGZ5Hqz4IEsSHcCWok+apwDSmv4=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=YMFv59lP87bQXQV5N/KXwFa9PtDYPkprAQabUDlCCtJgHrG917hRUVwYzCTfQpaF3 Q9VEkaV+uvCAag2rrTDiijFz35NYBsjS5cRpv0bDTrszvd0+xUuuKtQ+zVbQsQ7538 Wy59mKN8awKJehpuClR+t/MC8XJ4HAu8GApYFdKE= Received: from mail-lf1-f45.google.com (mail-lf1-f45.google.com [209.85.167.45]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 9266A6EC5D for ; Tue, 6 Apr 2021 17:33:24 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9266A6EC5D Received: by mail-lf1-f45.google.com with SMTP id n138so23140007lfa.3 for ; Tue, 06 Apr 2021 07:33:24 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=beY5ak1mu7Je58CBtMxpWMmnCtRlOn/PyKPmN8w2MxQ=; b=HmER02FBlyg4HCjhafNpLWGVyZZCnm09bQh1InNMw8S7cipF51LSJpL873goaDCIiB fekYeCykJAE/HuDdQQ5uyX1b5Eg+KOqMPLWsjSutCFVVKBhVZvQaI41r8o+s34pJhQR3 HoI3TuiQ/Y0K1b4JG/1ozPoJj73/qKdc88UQDhDgHdFh5Xqw/zP/VGGTPYI59+QgJ/fu eimiu9wWcmXDerogFd1KRxlfEQjsZlP8khhiN8fu3q530kaPoDyg2+2q5vu3thtKf7z1 NBS7vSlFCNRAKOJYZaoI8vkpMksJVJMUJYOseVIAy34EKQSjQ2ZFdL7StrCFXC2HSrPF Xqrg== X-Gm-Message-State: AOAM532jX5nUcO0DFvDuCmUbnTJTyfZKVnf0FqgezQvpb+nmMxEuLGP9 P4oe6wumMblCeqcu255s+qGm7DvvR9Y= X-Google-Smtp-Source: ABdhPJzBLon5zIsPVoAL5j15apaldVbfb2NLkKxfnEI173CjL24sCpU+uXyQ0xl9SIcGAY1n2Cr9ag== X-Received: by 2002:ac2:569c:: with SMTP id 28mr20119509lfr.414.1617719603328; Tue, 06 Apr 2021 07:33:23 -0700 (PDT) Received: from grain.localdomain ([5.18.199.94]) by smtp.gmail.com with ESMTPSA id g5sm2247424ljn.82.2021.04.06.07.33.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 06 Apr 2021 07:33:22 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id D374F5601DD; Tue, 6 Apr 2021 17:33:19 +0300 (MSK) Date: Tue, 6 Apr 2021 17:33:19 +0300 To: Vladislav Shpilevoy Cc: tml Message-ID: References: <20210402123420.885834-1-gorcunov@gmail.com> <20210402123420.885834-5-gorcunov@gmail.com> <14b1c3fa-da30-e7f1-6d17-32e575d71272@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <14b1c3fa-da30-e7f1-6d17-32e575d71272@tarantool.org> User-Agent: Mutt/2.0.5 (2021-01-21) 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: Cyrill Gorcunov via Tarantool-patches Reply-To: Cyrill Gorcunov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On Mon, Apr 05, 2021 at 05:52:47PM +0200, Vladislav Shpilevoy wrote: > > +/** > > + * 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. OK > > +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. Sure > > > + 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. When we put the module in the cache and something is failed we call generic module_unload which in turn calls cache_del module_load ... m = module_new(package, package_len, path); if (m != NULL && cache_put(m) != 0) { module_unload(m); --> module_unref if (--m->refs == 0) { cache_del(m); this is done for simplicity. So calling cache_del with module which is not in cache is fine. > > + > > + /* > > + * 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. Not needed anymore. static void module_attr_fill(struct module_attr *attr, struct stat *st) { --> memset(attr, 0, sizeof(*attr)); any possible padding is explicitly cleared. Initially I though of using __packed attribue or something but at the end realised that using explicit cleanup is a way more robust. > > +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. Not at all. You can exit tarantool via Ctrl+D inside console and modules won't be empty and we should clean them up. So I can and I should unload modules here. Vlad, this is _exit_ path called when we're exiting tarantool. What I'm missing? > > + > > +#include > > +#include > > 6. You don't need these headers in module_cache.h. They are > needed only in the .c file. Yes, thanks for pointing.